launchpad-reviewers team mailing list archive

[Diogo Matsubara <matsubara@xxxxxxxxxxxxx>]

The proposal to merge lp:~matsubara/launchpad/404279-private-ppa-link into lp:launchpad has been updated.

Description changed to:

== Summary ==

Bug 404279 proposes that the credentials for private PPAs should be shown in
the PPA page itself rather than showing PPA urls that won't work without the
credentials.

== Proposed fix ==

Currently, on a private PPA page (e.g.
https://qastaging.launchpad.net/~canonical-ux/+archive/walled-garden) there's
a section that shows the apt url which users can copy and paste into their
sources.list file to be able to access the PPA. The apt url doesn't contain the
credentials needed to access the PPA. The credentials are displayed in the
user's archive subscription page (e.g.
https://qastaging.launchpad.net/~matsubara/+archivesubscriptions/22392).

This fix generalizes the way the sources.list widget is built so it can be
reused in the PPA index page as well as in the person's archive subscription
page.

== Pre-implementation notes ==

I had a chat with Huw and Jono about this bug and this is my first take to fix
it. Jono suggested that a new class could be created to hold the methods
needed on both pages. Huw pointed out that it would be better to show the
credentials in the PPA page rather than just adding a link to the person's
archive subscription page.

== Implementation details ==

- There are some typo clean ups in this branch unrelated to the fix itself.
- SourcesListEntriesWidget might no be a good name for this class, so some advice from someone with more domain knowledge is appreciated. 
- I'm not sure if it's ok to create a class that expects child classes to have a certain attribute, such as self.user. I added a note in the docstring though.
- Should I add more test coverage for this change?

== Tests ==

 ./bin/test -t archive-views.txt -t archivesubscription-views.txt -t xx-private-ppa-subscription-stories.txt -t xx-private-ppa-subscriptions.txt

== Demo and Q/A ==

1. Logged in as cprov, create a new PPA (https://launchpad.dev/~cprov/+activate-ppa)
2. Logged in as a LP admin, make the PPA private (https://launchpad.dev/~cprov/+archive/foobar/+admin)
3. Copy the iceweasel package from another PPA into the newly created PPA (https://launchpad.dev/~cprov/+archive/ppa/+copy-packages)
4. Give access to cprov to be able to access that page (https://launchpad.dev/~cprov/+archive/foobar/+subscriptions)
5. Look at the Person archive subscription page (https://launchpad.dev/~cprov/+archivesubscriptions/22/+index)
6. Look at the PPA index page (https://launchpad.dev/~cprov/+archive/foobar)

They both should show the same information.

== lint ==

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/soyuz/browser/archive.py
  lib/lp/soyuz/browser/archivesubscription.py
  lib/lp/soyuz/browser/sourceslist.py
  lib/lp/soyuz/browser/tests/archive-views.txt
  lib/lp/soyuz/browser/tests/archivesubscription-views.txt

./lib/lp/soyuz/browser/archivesubscription.py
     155: E201 whitespace after '('
     218: E202 whitespace before ')'
     330: E202 whitespace before '}'
./lib/lp/soyuz/browser/sourceslist.py
      30: E301 expected 1 blank line, found 0
      52: E202 whitespace before '}'
      69: E241 multiple spaces after ','
     163: E501 line too long (81 characters)
     190: W391 blank line at end of file
     163: Line exceeds 78 characters.
./lib/lp/soyuz/browser/tests/archive-views.txt
       1: narrative uses a moin header.
       9: narrative uses a moin header.
     485: narrative uses a moin header.
     528: narrative uses a moin header.
     655: narrative uses a moin header.
    1039: narrative uses a moin header.
    1316: narrative uses a moin header.
    1382: narrative uses a moin header.
./lib/lp/soyuz/browser/tests/archivesubscription-views.txt
     173: narrative uses a moin header.
     316: narrative uses a moin header.

For more details, see:
https://code.launchpad.net/~matsubara/launchpad/404279-private-ppa-link/+merge/66335
-- 
https://code.launchpad.net/~matsubara/launchpad/404279-private-ppa-link/+merge/66335
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~matsubara/launchpad/404279-private-ppa-link into lp:launchpad.