Code review comment for lp:~jcsackett/launchpad/remove-bad-subscribers

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.

« Back to merge proposal