Merge lp://staging/~gary/launchpad/bug723999-2c into lp://staging/launchpad

Proposed by Gary Poster
Status: Merged
Merged at revision: 12548
Proposed branch: lp://staging/~gary/launchpad/bug723999-2c
Merge into: lp://staging/launchpad
Prerequisite: lp://staging/~gary/launchpad/bug723999-2b
Diff against target: 685 lines (+235/-218)
5 files modified
lib/lp/bugs/doc/bugsubscription.txt (+9/-2)
lib/lp/bugs/interfaces/bug.py (+5/-5)
lib/lp/bugs/model/bug.py (+68/-45)
lib/lp/bugs/model/structuralsubscription.py (+151/-120)
lib/lp/bugs/subscribers/bug.py (+2/-46)
To merge this branch: bzr merge lp://staging/~gary/launchpad/bug723999-2c
Reviewer Review Type Date Requested Status
Benji York (community) code Approve
Review via email: mp+52448@code.staging.launchpad.net

Description of the change

This branch is slightly meatier than than the previous two in the series, and we're starting to see the light at the end of the tunnel.

This branch makes the following changes.

- I factored the code of bug.getAlsoNotifiedSubscribers into a function, get_also_notified_subscribers. The method now calls the function.
- I made the function accept a bugtask or a bug, rather than only a bug. This meant that I could eliminate an essentially identical function of lp.bugs.subscribers.bug.get_bugtask_indirect_subscribers and reuse the function. I changed the only test of the duplicate function to use the newly shared one.
- I made the get_also_notified_subscribers function proxied so that it returns proxied objects. This maintains the behavior of calling utility methods that are available to everyone, so it is a pattern I'm following in the cours eof this work. You'll see I actually verify the expected behavior of the proxy in some small test changes of the code formerly for get_bugtask_indirect_subscribers.
- I made the get_also_notified_subscribers function handle direct subscriptions more carefully.
- I made the get_also_notified_subscribers function use the relatively new structuralsubscription function get_structural_subscribers directly, rather than go through the middleman of the IBugTaskSet method. As you might guess, one of my upcoming branches will remove that IBugTaskSet method and repoint the pertinent tests.
- I made get_structural_subscribers and friends accept a direct_subscribers argument. If they have already been calculated, as they have in get_also_notified_subscribers, let's use them, and make our queries faster!
- I pulled out two functions from the _get_structural_subscription_filter_id_query function, _calculate_bugtask_condition and _calculate_tag_query. The only point of the refactoring at this time is to try to make the code more understandable by dividing up responsibilities a bit. This is my attempt to respond to jcsackett's review of my original bug723999 branch. If you don't find these changes more comprehensible, I've failed.

Note that I tried to make lint completely happy (particularly notable is that lint is what caused me to move the XXX comment in lib/lp/bugs/interfaces/bug.py), but one complaint left me at a loss.

./lib/lp/bugs/interfaces/bug.py
     435: E301 expected 1 blank line, found 0

Line 435 is the one below beginning with "@operation_parameters":

...
    def newMessage(owner, subject, content):
        """Create a new message, and link it to this object."""

    @operation_parameters(
        person=Reference(IPerson, title=_('Person'), required=True),
...

I don't see what the problem is. I've decided to ignore it.

Thank you!

Gary

To post a comment you must log in.
Revision history for this message
Benji York (benji) wrote :

This branch is good to go. The descriptive and thorough introduction
helped quite a bit in understanding the changes.

As for the lint output, I determined that the comment embedded in the
decorator call is the culprit. I suggest filing a bug against
pocketlint and/or whichever Python linter it's using.

review: Approve (code)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.