Merge lp:~james-w/launchpad/suppress-notifications-for-all into lp:launchpad
Status: | Merged |
---|---|
Approved by: | Jonathan Lange |
Approved revision: | no longer in the source branch. |
Merged at revision: | 15383 |
Proposed branch: | lp:~james-w/launchpad/suppress-notifications-for-all |
Merge into: | lp:launchpad |
Diff against target: |
298 lines (+44/-82) 12 files modified
lib/lp/registry/model/person.py (+1/-2) lib/lp/soyuz/browser/archive.py (+0/-7) lib/lp/soyuz/configure.zcml (+3/-3) lib/lp/soyuz/interfaces/archive.py (+1/-1) lib/lp/soyuz/model/archive.py (+3/-10) lib/lp/soyuz/stories/ppa/xx-ppa-workflow.txt (+2/-6) lib/lp/soyuz/stories/webservice/xx-archive-commercial.txt (+5/-15) lib/lp/soyuz/stories/webservice/xx-archive.txt (+12/-0) lib/lp/soyuz/tests/test_archive.py (+12/-9) lib/lp/soyuz/tests/test_archive_privacy.py (+0/-11) lib/lp/soyuz/tests/test_archive_subscriptions.py (+3/-9) lib/lp/soyuz/tests/test_person_createppa.py (+2/-9) |
To merge this branch: | bzr merge lp:~james-w/launchpad/suppress-notifications-for-all |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Benji York (community) | code | Approve | |
Review via email: mp+107560@code.launchpad.net |
Commit message
Allow the archive owner to set 'suppress_
Description of the change
= Summary =
Archives have a "suppress_
historical reasons is only settable by commercial admins. There's nothing
sensitive about the attribute, so we can allow it to be set by any owner.
== Proposed fix ==
Change the permissions for the attribute to require Launchpad.Edit rather
than Launchpad.
== Pre-implementation notes ==
I didn't have a pre-implementation call with anyone on the LP team.
== LOC Rationale ==
38 lines in credit, bringing us to 140 total for this arc of work.
== Implementation details ==
Changing the permission required is pretty easy, and most of the
other changes are test changes to test the new behaviour rather
than the old.
This change does drop the (partially implemented) restriction that
only private archives can have suppress_
I decided this was best as there would be a lot of lines of code
required to enforce this, even leaving aside presenting that
restriction well in the UI.
This change does not make the attribute setable on the +edit page
of the archive, leaving it on +admin for now. I'm not sure that
it should be moved to +edit, but it can be done in a followup
branch if needed.
There are also a couple of changes that seem unrelated, but they
are cleaning up debt in this area (changes to the commercial
archives doctest file so that it no longer refers to a file
that doesn't exist, and doesn't create an archive that isn't
used anywhere else in the file.)
== Tests ==
There are new tests that check that owners can set and change
the attribute, and a tests that a non-owner can't. Tests
were removed that checked that commercial admins can set
the attribute.
== Demo and Q/A ==
I believe that testing that the attribute can still be
toggled on the +admin form should be sufficient for
allowing this to rollout. Checking that owners can now
change the attribute will be necessary for closing the
bug.
I don't think that testing that the attribute still causes
emails to be suppressed is necessary as none of that code
changed.
= Launchpad lint =
I don't think these reported issues should be addressed
as they would harm readability:
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/
lib/lp/
lib/lp/
lib/lp/
lib/lp/
lib/lp/
lib/lp/
lib/lp/
lib/lp/
lib/lp/
lib/lp/
lib/lp/
./lib/lp/
278: want exceeds 78 characters.
./lib/lp/
42: want exceeds 78 characters.
46: want exceeds 78 characters.
169: want exceeds 78 characters.
185: want exceeds 78 characters.
201: want exceeds 78 characters.
217: want exceeds 78 characters.
361: want exceeds 78 characters.
422: want exceeds 78 characters.
553: want exceeds 78 characters.
./lib/lp/
913: E303 too many blank lines (2)
./lib/lp/
346: redefinition of function 'suppress_
357: redefinition of function 'private' from line 353
./lib/lp/
2990: E501 line too long (84 characters)
This branch looks good. I spotted one small improvement you could make
and had a couple of thoughts about lint while reading over it.
The newly-dedented return on line 104 of the diff can now fit on one
line.
Lint:
- I concur that leaving the long lines for doctests is normally the
right thing to do.
- It looks like fixing the "too many blank lines" in archive.py
wouldn't hurt too much.
- I couldn't find the long line reported on line 2990 of person.py.
Maybe the lint report quoted was from a point in time when the
changed code was all on one line.