Review: Needs Fixing code Looking very good, Steve. I have many comments, as always, including a couple of bugs.
152 -The upstream maintainer will be subscribed to security-related private 153 -bugs, because upstream has no security contact, in this case. This branch is a prerequisite for the work that will make this test obsolete, but I believe it's still relevant. It's talking about an actual BugSubscription here. 170 === modified file 'lib/lp/bugs/doc/security-teams.txt' Likewise with both changes here. This branch only enables implicit subscriptions -- we still need to test that explicit subscriptions work until we remove them. 201 - def getBugNotificationRecipients(duplicateof=None, old_bug=None, 202 - include_master_dupe_subscribers=False): 203 + def getBugNotificationRecipients(duplicateof=None, old_bug=None): Nice cleanup of a dead codepath. 278 + def forbidden_recipients_filter(self, column=None): This actually filters to the allowed recipients, not the forbidden ones. I also wonder if it would be better to return a single expression, which is the Or or True, letting callsites just include it inline without extend()ing. And is it really worth having column= be optional? 337 + all = Select(BugTask.assigneeID, BugTask.bug == self.bug) 338 + assignees = load_people(Person.id.is_in(all)) Why have you split the subquery out into a separate local? Might as well inline it, or at least not shadow a builtin. Also, did you consider inlining the subsequent filter into load_people's where argument? 368 + return subscribers.difference( 369 + self.direct_subscribers_at_all_levels, Direct subscribers will usually have a grant, but not always (eg. after their project access has been revoked, but the sub removal job hasn't run yet). This needs to be filtered too. 392 + if bug.private: 393 + filters.extend([ 394 + Or(*get_bug_privacy_filter_terms( 395 + StructuralSubscription.subscriberID)), 396 + BugTaskFlat.bug == bug]) I'm a little concerned about this. The function didn't previously return nothing for private bugs, so this may be the wrong level to filter at. Did you consider other locations? 424 - def test_structural_bug_supervisor_becomes_direct_on_private(self): 425 - # If a bug supervisor has a structural subscription to the bug, and 426 - # the bug is marked as private, the supervisor should get a direct 427 - # subscription. Otherwise they should be removed, per other tests. Why is this test no longer valid? Yes, the behaviour should be removed later, but this branch doesn't do it. 448 - def test_information_type_does_not_leak(self): 449 - # Make sure that bug notifications for private bugs do not leak to 450 - # people with a subscription on the product. This seems like a reasonable integration test to retain. You've added a test that sharing => notification in test_private_bug_with_structural_subscriber_with_access, but unless I've missed one this is the only one that tests !sharing => !notification. 797 - def test_private_bug_target_change_doesnt_add_everyone(self): Again, I don't see that this test's value is gone. -- https://code.launchpad.net/~stevenk/launchpad/structsub-private-bugs-redux/+merge/116798 Your team Launchpad code reviewers is subscribed to branch lp:launchpad. _______________________________________________ Mailing list: https://launchpad.net/~launchpad-reviewers Post to : [email protected] Unsubscribe : https://launchpad.net/~launchpad-reviewers More help : https://help.launchpad.net/ListHelp
