Launchpad itself

Merge lp:~michael.nelson/launchpad/458200-p3a into lp:launchpad

  1. 458200-p3a
  2. Merge into devel
Proposed by Michael Nelson
Status: Merged
Merged at revision: not available
Proposed branch: lp:~michael.nelson/launchpad/458200-p3a
Merge into: lp:launchpad
Diff against target: 184 lines (+110/-11)
3 files modified
lib/lp/registry/browser/person.py (+3/-3)
lib/lp/registry/browser/tests/person-views.txt (+5/-3)
lib/lp/registry/browser/tests/test_person_view.py (+102/-5)
To merge this branch: bzr merge lp:~michael.nelson/launchpad/458200-p3a
Reviewer Review Type Date Requested Status
Abel Deuring (community) code Approve
Review via email: mp+14216@code.launchpad.net

Commit message

Instead of showing the PPA section on a team/person page when there is at least one public ppa, we
show the section when there is at least one ppa that the current user can view.

To post a comment you must log in.
Revision history for this message
Michael Nelson (michael.nelson) wrote :

= Summary =

This fixes bug 458200 where members viewing a team would not see the ppa
section if the team had a single private PPA.

== Proposed fix ==

Instead of showing the section when there is at least one public ppa, we
show the section when there is at least one ppa that the current user
can view.

== Pre-implementation notes ==

Muharem and I talked about this together during our pairing session.

== Implementation details ==

I'm a little uncertain about the tests - I couldn't see any other unit
tests for views that used a layer other than LaunchpadZopeless, but I
couldn't use LaunchpadZopeless as otherwise check_permission always
returns true. Is this OK - or is it reflecting that I'm doing something
I shouldn't be (like using check_permission in a view?).

== Tests ==

bin/test -vvt TestShouldShowPpaSection

== Demo and Q/A ==

To demo locally:

1. Login as <email address hidden>:test
2. visit https://launchpad.dev/~launchpad and create a new PPA called
ppaprivate.
3. Click on Administer archive and set the PPA to private (adding an
arbitrary buildd_secret)
4. re-visit https://launchpad.dev/~launchpad and verify the ppa section is
   there.
5. Click on active members, then add member, adding cprov.
6. logout, view https://launchpad.dev/~launchpad and confirm that PPA
section is not displayed.
7. login as <email address hidden>:cprov and confirm that the PPA
section *is* displayed.
8. logout and log back in as foo.bar and add a second ppa called publicppa.
9. logout and visit https://launchpad.dev/~launchpad and ensure that the
PPA section *is* displayed with just the public ppa listed.
10. login as cprov and both are listed.

If you do the same test on RF, point 7 will fail.

For QA we can ask the person on the bug to confirm that they now see the
ppa section.

= Launchpad lint =

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

Linting changed files:
  lib/lp/registry/browser/tests/test_person_view.py
  lib/lp/registry/browser/person.py

== Pylint notices ==

lib/lp/registry/browser/person.py
    117: [F0401] Unable to import 'lazr.delegates' (No module named
delegates)
    118: [F0401] Unable to import 'lazr.config' (No module named config)
    119: [F0401] Unable to import 'lazr.restful.interface' (No module
named restful)
    230: [F0401] Unable to import 'lazr.uri' (No module named uri)

--
Michael

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

nice work!

review: Approve (code)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/registry/browser/person.py'
2--- lib/lp/registry/browser/person.py 2009-11-20 05:40:25 +0000
3+++ lib/lp/registry/browser/person.py 2009-11-21 14:47:15 +0000
4@@ -3000,15 +3000,15 @@
5 """Return True if "Personal package archives" is to be shown.
6
7 We display it if:
8- person has any public PPA or current_user has lp.edit
9+ current_user may view at least one PPA or current_user has lp.edit
10 """
11 # If the current user has edit permission, show the section.
12 if check_permission('launchpad.Edit', self.context):
13 return True
14
15- # If the current user has any public PPA, show the section.
16+ # If the current user can view any PPA, show the section.
17 for ppa in self.context.ppas:
18- if not ppa.private:
19+ if check_permission('launchpad.View', ppa):
20 return True
21
22 return False
23
24=== modified file 'lib/lp/registry/browser/tests/person-views.txt'
25--- lib/lp/registry/browser/tests/person-views.txt 2009-11-17 13:37:49 +0000
26+++ lib/lp/registry/browser/tests/person-views.txt 2009-11-21 14:47:14 +0000
27@@ -748,7 +748,8 @@
28
29 The property checks two things to decide whether to return True or not:
30 * Return True if the current user has launchpad.Edit permission
31- * Return True if the person has PPAs and at least one of them is public.
32+ * Return True if the person has PPAs and at least one of them is viewable
33+ by the current user.
34
35 Cprov is a user with a PPA:
36
37@@ -768,7 +769,8 @@
38 >>> view.should_show_ppa_section
39 True
40
41-If we disable Cprov's PPA, the section is still presented to anyone.
42+If we disable Cprov's PPA, the section is not presented to anonymous
43+users who cannot view the PPA, but is displayed to Celso.
44
45 >>> login("admin@canonical.com")
46 >>> cprov.archive.enabled = False
47@@ -776,7 +778,7 @@
48 >>> login(ANONYMOUS)
49 >>> view = create_initialized_view(cprov, "+index")
50 >>> view.should_show_ppa_section
51- True
52+ False
53
54 >>> login("celso.providelo@canonical.com")
55 >>> view = create_initialized_view(cprov, "+index")
56
57=== modified file 'lib/lp/registry/browser/tests/test_person_view.py'
58--- lib/lp/registry/browser/tests/test_person_view.py 2009-10-16 15:00:55 +0000
59+++ lib/lp/registry/browser/tests/test_person_view.py 2009-11-21 14:47:15 +0000
60@@ -7,16 +7,17 @@
61
62 from zope.component import getUtility
63
64+from canonical.launchpad.ftests import ANONYMOUS, login
65 from canonical.launchpad.webapp.interfaces import NotFoundError
66 from lp.registry.interfaces.karma import IKarmaCacheManager
67 from canonical.launchpad.webapp.servers import LaunchpadTestRequest
68-from canonical.testing import LaunchpadZopelessLayer
69+from canonical.testing import LaunchpadFunctionalLayer, LaunchpadZopelessLayer
70 from lp.registry.browser.person import PersonView
71 from lp.registry.model.karma import KarmaCategory
72-from lp.testing import TestCaseWithFactory
73-
74-
75-class TestPersonView(TestCaseWithFactory):
76+from lp.testing import TestCaseWithFactory, login_person
77+
78+
79+class TestPersonViewKarma(TestCaseWithFactory):
80
81 layer = LaunchpadZopelessLayer
82
83@@ -69,5 +70,101 @@
84 return karmacache
85
86
87+class TestShouldShowPpaSection(TestCaseWithFactory):
88+
89+ layer = LaunchpadFunctionalLayer
90+
91+ def setUp(self):
92+ TestCaseWithFactory.setUp(self)
93+ self.owner = self.factory.makePerson(name='mowgli')
94+ self.person_ppa = self.factory.makeArchive(owner=self.owner)
95+ self.team = self.factory.makeTeam(name='jbook', owner=self.owner)
96+
97+ # The team is the owner of the PPA.
98+ self.team_ppa = self.factory.makeArchive(owner=self.team)
99+ self.team_view = PersonView(self.team, LaunchpadTestRequest())
100+
101+ def make_ppa_private(self, ppa):
102+ """Helper method to privatise a ppa."""
103+ login('foo.bar@canonical.com')
104+ ppa.private = True
105+ ppa.buildd_secret = "secret"
106+ login(ANONYMOUS)
107+
108+ def test_viewing_person_with_public_ppa(self):
109+ # Show PPA section only if context has at least one PPA the user is
110+ # authorised to view the PPA.
111+ login(ANONYMOUS)
112+ person_view = PersonView(self.owner, LaunchpadTestRequest())
113+ self.failUnless(person_view.should_show_ppa_section)
114+
115+ def test_viewing_person_without_ppa(self):
116+ # If the context person does not have a ppa then the section
117+ # should not display.
118+ login(ANONYMOUS)
119+ person_without_ppa = self.factory.makePerson()
120+ person_view = PersonView(person_without_ppa, LaunchpadTestRequest())
121+ self.failIf(person_view.should_show_ppa_section)
122+
123+ def test_viewing_self(self):
124+ # If the current user has edit access to the context person then
125+ # the section should always display
126+ login_person(self.owner)
127+ person_view = PersonView(self.owner, LaunchpadTestRequest())
128+ self.failUnless(person_view.should_show_ppa_section)
129+
130+ # If the ppa is private, the section is still displayed to
131+ # a user with edit access to the person.
132+ self.make_ppa_private(self.person_ppa)
133+ login_person(self.owner)
134+ person_view = PersonView(self.owner, LaunchpadTestRequest())
135+ self.failUnless(person_view.should_show_ppa_section)
136+
137+ # Even a person without a PPA will see the section when viewing
138+ # themselves.
139+ person_without_ppa = self.factory.makePerson()
140+ login_person(person_without_ppa)
141+ person_view = PersonView(person_without_ppa, LaunchpadTestRequest())
142+ self.failUnless(person_view.should_show_ppa_section)
143+
144+ def test_anon_viewing_person_with_private_ppa(self):
145+ # If the ppa is private, the ppa section will not be displayed
146+ # to users without view access to the ppa.
147+ self.make_ppa_private(self.person_ppa)
148+ login(ANONYMOUS)
149+ person_view = PersonView(self.owner, LaunchpadTestRequest())
150+ self.failIf(person_view.should_show_ppa_section)
151+
152+ # But if the context person has a second ppa that is public,
153+ # then anon users will see the section.
154+ second_ppa = self.factory.makeArchive(owner=self.owner)
155+ person_view = PersonView(self.owner, LaunchpadTestRequest())
156+ self.failUnless(person_view.should_show_ppa_section)
157+
158+ def test_viewing_team_with_private_ppa(self):
159+ # If a team PPA is private, the ppa section will be displayed
160+ # to team members.
161+ self.make_ppa_private(self.team_ppa)
162+ member = self.factory.makePerson()
163+ login_person(self.owner)
164+ self.team.addMember(member, self.owner)
165+ login_person(member)
166+
167+ # So the member will see the section.
168+ person_view = PersonView(self.team, LaunchpadTestRequest())
169+ self.failUnless(person_view.should_show_ppa_section)
170+
171+ # But other users who are not members will not.
172+ non_member = self.factory.makePerson()
173+ login_person(non_member)
174+ person_view = PersonView(self.team, LaunchpadTestRequest())
175+ self.failIf(person_view.should_show_ppa_section)
176+
177+ # Unless the team also has another ppa which is public.
178+ second_ppa = self.factory.makeArchive(owner=self.team)
179+ person_view = PersonView(self.team, LaunchpadTestRequest())
180+ self.failUnless(person_view.should_show_ppa_section)
181+
182+
183 def test_suite():
184 return unittest.TestLoader().loadTestsFromName(__name__)