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

Proposed by j.c.sackett
Status: Merged
Approved by: j.c.sackett
Approved revision: no longer in the source branch.
Merged at revision: 15747
Proposed branch: lp:~jcsackett/launchpad/remove-bad-subscribers
Merge into: lp:launchpad
Diff against target: 0 lines
To merge this branch: bzr merge lp:~jcsackett/launchpad/remove-bad-subscribers
Reviewer Review Type Date Requested Status
Curtis Hovey (community) code Approve
Review via email: mp+117771@code.launchpad.net

Commit message

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

Description of the change

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

To post a comment you must log in.
Revision history for this message
Curtis Hovey (sinzui) wrote :

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)
Revision history for this message
j.c.sackett (jcsackett) wrote :

> 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.

Revision history for this message
Curtis Hovey (sinzui) wrote :

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

review: Approve (code)

Preview Diff

Empty