Code review comment for lp:~adeuring/launchpad/bug-479331

Revision history for this message
Abel Deuring (adeuring) wrote :

This branch fixes (sort of) bug 479331: It's not possible to set someone else as bug supervisor.

While the bug description says that anybody should be able to appoint the bug supervisor, it seems for now better to keep the policy of the structural bug subscription which is autmatically performed for the new supervisor: People may only subscribe themselves and teams they administer.

Firstly, the policy for structural bug subscription is reasonable: people who are not aware of the consequences of becoming a bug supervisor might feel being spammed by the possibly huge amount of bug mails they suddenly receive.

Secondly, the option to simply allow anybody to appoint a bug supervisor would mean to implement another exception to the subscription permission check in StructuralSubscriptionTargetMixin.userCanAlterSubscription(), which is already slightly too complicated for my taste, with the special handling of IDistributionSourcePackage targets.

So I only added a check if the person which tries to appoint the bug supervisor can actually do this: userCanAlterSubscription() is now called in BugSupervisorEditView.validate(). This involved to rename the method _userCanAlterSubscription(), to ad it to the interface class and to change two ZCML files.

The problem that most people cannot appoint others as bug supervisors was not caught in the tests because the persons used in the tests were all Launchpad administrators, which are allowed to make structural subscriptions for others. So I had to change the "test persons", and I had to change the teams they can subscribe. (BTW, our test data has way too many LP administrators: I needed some time to find persons who are team admins but not LP admins.)

The changed and added tests quickly re-appoint several persons and team as bug supervisors. When a new bug supervisor is appointed, the structural of the old supervisor is not automatically removed. The set of people subscribed are shown in other page tests, so these sets changed too: I did not bother add anything to remove the subscription.

test:./bin/test -v -f lp stories.initial-bug-contacts

= Launchpad lint =

Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.

Linting changed files:
  lib/canonical/launchpad/database/structuralsubscription.py
  lib/canonical/launchpad/interfaces/structuralsubscription.py
  lib/lp/bugs/browser/bugsupervisor.py
  lib/lp/bugs/stories/initial-bug-contacts/05-set-distribution-bugcontact.txt
  lib/lp/bugs/stories/initial-bug-contacts/10-set-upstream-bugcontact.txt
  lib/lp/bugs/stories/initial-bug-contacts/20-file-upstream-bug.txt
  lib/lp/bugs/stories/initial-bug-contacts/25-file-distribution-bug.txt
  lib/lp/registry/configure.zcml

== Pylint notices ==

lib/canonical/launchpad/interfaces/structuralsubscription.py
    22: [F0401] Unable to import 'lazr.enum' (No module named enum)
    29: [F0401] Unable to import 'lazr.restful.declarations' (No module named restful)
    34: [F0401] Unable to import 'lazr.restful.fields' (No module named restful)

« Back to merge proposal