launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

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.