Merge lp:~jcsackett/launchpad/remove-bad-subscribers into lp:launchpad

[j.c.sackett]

Updates transition code so direct subscribers are only given access if they're supposed to have it.

Summary
=======
Currently, if a bug is transitioned to a private status, then all direct
subscribers are given access to the private bug. We really only want those who
have the appropriate APG or AAG, as well as those in some special roles to
have access.

This updates the code to ensure only those special roles are ensured access,
and that subscriptions are handled appropriately.

Preimp
======
Spoke with Purple Squad.

Implementation
==============
Missing subscribers has been updated to required_subscribers; this is because
they are the only people required after transition.

The order of events has been updated--the information type is changed and the
access grants are reconciled before moving forward, so that we can rely on the
loss of access as a means of determining who needs to be removed.

Instead of all subscribers without access being granted access, only those who
are in required subscribers are given access.

Because all subscribers are not now given access, if the unsubscribe job
feature flag is not set, we manually remove the subscribers. This can be
removed once we no longer have the flag on the creation of the job.

Tests
=====
bin/test -vvcm lp.bugs.model.tests.test_bug

QA
==
Switch a bug to private; make sure only the expected parties still have
access/are subscribed.

LoC
===
This is part of disclosure.

Lint
====

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/bugs/model/tests/test_bug.py
  lib/lp/bugs/model/bug.py

[Curtis Hovey (sinzui)]

Hi Jon.

As Steven's and Ian's work to honour structural subscriptions is completing, the focus on subscribing roles will soon be mute. I do not want to abandon your work though. The removal of test_transition_to_private_grants_subscribers_access demonstrates that there is still a problem with Lp's behaviour that your branch fixes. We do not want random direct bug subscribers to be granted access.

1. I think the preservation of existing driver subscriptions is good.
   Maybe the code can be DRY if `required_subscribers.add(pillar.driver)`
   was in the ` if information_type in PRIVATE_INFORMATION_TYPES` block
2. Is the bug reporter and person making the information type change
   also subscribed and given an access grant?
3. Is there a test that shows that the reporter, changer, and driver
   have access?

review: Needs Information (code)

[j.c.sackett (jcsackett)]

> 1. I think the preservation of existing driver subscriptions is good.
> Maybe the code can be DRY if `required_subscribers.add(pillar.driver)`
> was in the ` if information_type in PRIVATE_INFORMATION_TYPES` block

Sure, that's doable; it also needs the !ubuntu check on it.

> 2. Is the bug reporter and person making the information type change
> also subscribed and given an access grant?

They are; line 8 of the current diff.

> 3. Is there a test that shows that the reporter, changer, and driver
> have access?

Yes--those are the modified tests in test_bug. They don't directly test access, as they began as subscriber checks, but as you can only be subscribed to a bug you have access to on a private change, the function is tested. We could update those tests to explicitly check for access, if you think we no longer need any subscriber tests; if we need to preserve those tests I'm hesitant to add more as they would be essentially identical checks.

[Curtis Hovey (sinzui)]

Thank you. You can land this when you are satisfied with the driver.

review: Approve (code)