launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #04091
[Merge] lp:~matsubara/launchpad/404279-private-ppa-link into lp:launchpad
Diogo Matsubara has proposed merging lp:~matsubara/launchpad/404279-private-ppa-link into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #404279 in Launchpad itself: "apt urls on private ppa pages are incorrect (missing credentials)"
https://bugs.launchpad.net/launchpad/+bug/404279
For more details, see:
https://code.launchpad.net/~matsubara/launchpad/404279-private-ppa-link/+merge/66335
== 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.
== 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.
--
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.
=== modified file 'lib/lp/soyuz/browser/archive.py'
--- lib/lp/soyuz/browser/archive.py 2011-06-23 16:11:42 +0000
+++ lib/lp/soyuz/browser/archive.py 2011-06-29 15:31:21 +0000
@@ -123,10 +123,7 @@
BuildNavigationMixin,
BuildRecordsView,
)
-from lp.soyuz.browser.sourceslist import (
- SourcesListEntries,
- SourcesListEntriesView,
- )
+from lp.soyuz.browser.sourceslist import SourcesListEntriesWidget
from lp.soyuz.browser.widgets.archive import PPANameWidget
from lp.soyuz.enums import (
ArchivePermissionType,
@@ -563,7 +560,7 @@
links = ['copy', 'delete']
-class ArchiveViewBase(LaunchpadView):
+class ArchiveViewBase(LaunchpadView, SourcesListEntriesWidget):
"""Common features for Archive view classes."""
def initialize(self):
@@ -583,22 +580,15 @@
"being dispatched.")
self.request.response.addNotification(structured(notification))
super(ArchiveViewBase, self).initialize()
+ # Set the archive attribute so SourcesListEntriesWidget can be built
+ # correctly.
+ self.archive = self.context
@cachedproperty
def private(self):
return self.context.private
@cachedproperty
- def has_sources(self):
- """Whether or not this PPA has any sources for the view.
-
- This can be overridden by subclasses as necessary. It allows
- the view to determine whether to display "This PPA does not yet
- have any published sources" or "No sources matching 'blah'."
- """
- return not self.context.getPublishedSources().is_empty()
-
- @cachedproperty
def repository_usage(self):
"""Return a dictionary with usage details of this repository."""
@@ -648,14 +638,6 @@
quota=quota)
@property
- def archive_url(self):
- """Return an archive_url where available, or None."""
- if self.has_sources and not self.context.is_copy:
- return self.context.archive_url
- else:
- return None
-
- @property
def archive_label(self):
"""Return either 'PPA' or 'Archive' as the label for archives.
@@ -904,14 +886,6 @@
return TextLineEditorWidget(self.context, display_name, title, 'h1')
@property
- def sources_list_entries(self):
- """Setup and return the source list entries widget."""
- entries = SourcesListEntries(
- self.context.distribution, self.archive_url,
- self.context.series_with_sources)
- return SourcesListEntriesView(entries, self.request)
-
- @property
def default_series_filter(self):
"""Return the distroseries identified by the user-agent."""
version_number = get_user_agent_distroseries(
=== modified file 'lib/lp/soyuz/browser/archivesubscription.py'
--- lib/lp/soyuz/browser/archivesubscription.py 2011-03-29 05:07:21 +0000
+++ lib/lp/soyuz/browser/archivesubscription.py 2011-06-29 15:31:21 +0000
@@ -47,12 +47,8 @@
from lp.registry.interfaces.person import IPersonSet
from lp.services.fields import PersonChoice
from lp.services.propertycache import cachedproperty
-from lp.soyuz.browser.sourceslist import (
- SourcesListEntries,
- SourcesListEntriesView,
- )
+from lp.soyuz.browser.sourceslist import SourcesListEntriesWidget
from lp.soyuz.interfaces.archive import IArchiveSet
-from lp.soyuz.interfaces.archiveauthtoken import IArchiveAuthTokenSet
from lp.soyuz.interfaces.archivesubscriber import (
IArchiveSubscriberSet,
IPersonalArchiveSubscription,
@@ -156,7 +152,7 @@
def subscriptions(self):
"""Return all the subscriptions for this archive."""
result = list(getUtility(IArchiveSubscriberSet
- ).getByArchive( self.context))
+ ).getByArchive(self.context))
ids = set(map(attrgetter('subscriber_id'), result))
list(getUtility(IPersonSet).getPrecachedPersonsFromIDs(ids,
need_validity=True))
@@ -337,7 +333,7 @@
return personal_subscription_tokens
-class PersonArchiveSubscriptionView(LaunchpadView):
+class PersonArchiveSubscriptionView(LaunchpadView, SourcesListEntriesWidget):
"""Display a user's archive subscription and relevant info.
This includes the current sources.list entries (if the subscription
@@ -353,9 +349,12 @@
def initialize(self):
"""Process any posted actions."""
super(PersonArchiveSubscriptionView, self).initialize()
+ # Set the archive attribute so SourcesListEntriesWidget can be built
+ # correctly.
+ self.archive = self.context.archive
# If an activation was requested and there isn't a currently
- # active token, then create a token, provided a notification
+ # active token, then create a token, provide a notification
# and redirect.
if self.request.form.get('activate') and not self.active_token:
self.context.archive.newAuthToken(self.context.subscriber)
@@ -377,27 +376,3 @@
"\"sources.list\"." % self.context.archive.displayname)
self.request.response.redirect(self.request.getURL())
-
- @cachedproperty
- def sources_list_entries(self):
- """Setup and return the sources list entries widget."""
- if self.active_token is None:
- return None
-
- comment = "Personal access of %s to %s" % (
- self.context.subscriber.displayname,
- self.context.archive.displayname)
-
- entries = SourcesListEntries(
- self.context.archive.distribution,
- self.active_token.archive_url,
- self.context.archive.series_with_sources)
-
- return SourcesListEntriesView(entries, self.request, comment=comment)
-
- @cachedproperty
- def active_token(self):
- """Returns the corresponding current token for this subscription."""
- token_set = getUtility(IArchiveAuthTokenSet)
- return token_set.getActiveTokenForArchiveAndPerson(
- self.context.archive, self.context.subscriber)
=== modified file 'lib/lp/soyuz/browser/sourceslist.py'
--- lib/lp/soyuz/browser/sourceslist.py 2010-08-20 20:31:18 +0000
+++ lib/lp/soyuz/browser/sourceslist.py 2011-06-29 15:31:21 +0000
@@ -8,6 +8,7 @@
from z3c.ptcompat import ViewPageTemplateFile
from zope.app.form.interfaces import IInputWidget
from zope.app.form.utility import setUpWidget
+from zope.component import getUtility
from zope.schema import Choice
from zope.schema.vocabulary import (
SimpleTerm,
@@ -17,6 +18,8 @@
from canonical.launchpad import _
from canonical.launchpad.webapp import LaunchpadView
from lp.services.browser_helpers import get_user_agent_distroseries
+from lp.services.propertycache import cachedproperty
+from lp.soyuz.interfaces.archiveauthtoken import IArchiveAuthTokenSet
class SourcesListEntries:
@@ -24,6 +27,7 @@
Represents a set of distroseries in a distribution archive.
"""
+
def __init__(self, distribution, archive_url, valid_series):
self.distribution = distribution
self.archive_url = archive_url
@@ -132,3 +136,53 @@
# user that they should select a distroseries.
return self.initial_value_without_selection
+
+class SourcesListEntriesWidget:
+ """Setup the sources list entries widget.
+
+ This class assumes self.user is set in child classes.
+ """
+
+ @cachedproperty
+ def sources_list_entries(self):
+ """Setup and return the sources list entries widget."""
+ if self.active_token is None:
+ entries = SourcesListEntries(
+ self.archive.distribution, self.archive_url,
+ self.archive.series_with_sources)
+ return SourcesListEntriesView(entries, self.request)
+ else:
+ comment = "Personal access of %s to %s" % (
+ self.user.displayname,
+ self.archive.displayname)
+ entries = SourcesListEntries(
+ self.archive.distribution,
+ self.active_token.archive_url,
+ self.archive.series_with_sources)
+ return SourcesListEntriesView(
+ entries, self.request, comment=comment)
+
+ @cachedproperty
+ def active_token(self):
+ """Return the corresponding current token for this subscription."""
+ token_set = getUtility(IArchiveAuthTokenSet)
+ return token_set.getActiveTokenForArchiveAndPerson(
+ self.archive, self.user)
+
+ @property
+ def archive_url(self):
+ """Return an archive_url where available, or None."""
+ if self.has_sources and not self.archive.is_copy:
+ return self.archive.archive_url
+ else:
+ return None
+
+ @cachedproperty
+ def has_sources(self):
+ """Whether or not this PPA has any sources for the view.
+
+ This can be overridden by subclasses as necessary. It allows
+ the view to determine whether to display "This PPA does not yet
+ have any published sources" or "No sources matching 'blah'."
+ """
+ return not self.archive.getPublishedSources().is_empty()
=== modified file 'lib/lp/soyuz/browser/tests/archive-views.txt'
--- lib/lp/soyuz/browser/tests/archive-views.txt 2011-06-06 14:27:47 +0000
+++ lib/lp/soyuz/browser/tests/archive-views.txt 2011-06-29 15:31:21 +0000
@@ -49,8 +49,7 @@
>>> print copy_archive_view.archive_label
archive
-The ArchiveView is provides the html for the inline description
-editing widget.
+The ArchiveView provides the html for the inline description editing widget.
>>> print ppa_archive_view.archive_description_html.title
PPA description
=== modified file 'lib/lp/soyuz/browser/tests/archivesubscription-views.txt'
--- lib/lp/soyuz/browser/tests/archivesubscription-views.txt 2010-04-28 15:42:29 +0000
+++ lib/lp/soyuz/browser/tests/archivesubscription-views.txt 2011-06-29 15:31:21 +0000
@@ -318,7 +318,7 @@
This view displays a single subscription of a person, as well as the
corresponding token information.
-The view includes a label to denifen its main heading.
+The view includes a label to define its main heading.
>>> from lp.soyuz.browser.archivesubscription import (
... PersonalArchiveSubscription)
@@ -333,11 +333,6 @@
>>> print view.active_token
None
-Therefore, it will not return a sources_list_entry either:
-
- >>> print view.sources_list_entries
- None
-
But if 'activate' is posted, the view will generate a new token and
redirect:
Follow ups
