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
=== modified file 'lib/lp/registry/browser/person.py'
--- lib/lp/registry/browser/person.py 2009-11-20 05:40:25 +0000
+++ lib/lp/registry/browser/person.py 2009-11-21 14:47:15 +0000
@@ -3000,15 +3000,15 @@
3000 """Return True if "Personal package archives" is to be shown.3000 """Return True if "Personal package archives" is to be shown.
30013001
3002 We display it if:3002 We display it if:
3003 person has any public PPA or current_user has lp.edit3003 current_user may view at least one PPA or current_user has lp.edit
3004 """3004 """
3005 # If the current user has edit permission, show the section.3005 # If the current user has edit permission, show the section.
3006 if check_permission('launchpad.Edit', self.context):3006 if check_permission('launchpad.Edit', self.context):
3007 return True3007 return True
30083008
3009 # If the current user has any public PPA, show the section.3009 # If the current user can view any PPA, show the section.
3010 for ppa in self.context.ppas:3010 for ppa in self.context.ppas:
3011 if not ppa.private:3011 if check_permission('launchpad.View', ppa):
3012 return True3012 return True
30133013
3014 return False3014 return False
30153015
=== modified file 'lib/lp/registry/browser/tests/person-views.txt'
--- lib/lp/registry/browser/tests/person-views.txt 2009-11-17 13:37:49 +0000
+++ lib/lp/registry/browser/tests/person-views.txt 2009-11-21 14:47:14 +0000
@@ -748,7 +748,8 @@
748748
749The property checks two things to decide whether to return True or not:749The property checks two things to decide whether to return True or not:
750 * Return True if the current user has launchpad.Edit permission750 * Return True if the current user has launchpad.Edit permission
751 * Return True if the person has PPAs and at least one of them is public.751 * Return True if the person has PPAs and at least one of them is viewable
752 by the current user.
752753
753Cprov is a user with a PPA:754Cprov is a user with a PPA:
754755
@@ -768,7 +769,8 @@
768 >>> view.should_show_ppa_section769 >>> view.should_show_ppa_section
769 True770 True
770771
771If we disable Cprov's PPA, the section is still presented to anyone.772If we disable Cprov's PPA, the section is not presented to anonymous
773users who cannot view the PPA, but is displayed to Celso.
772774
773 >>> login("admin@canonical.com")775 >>> login("admin@canonical.com")
774 >>> cprov.archive.enabled = False776 >>> cprov.archive.enabled = False
@@ -776,7 +778,7 @@
776 >>> login(ANONYMOUS)778 >>> login(ANONYMOUS)
777 >>> view = create_initialized_view(cprov, "+index")779 >>> view = create_initialized_view(cprov, "+index")
778 >>> view.should_show_ppa_section780 >>> view.should_show_ppa_section
779 True781 False
780782
781 >>> login("celso.providelo@canonical.com")783 >>> login("celso.providelo@canonical.com")
782 >>> view = create_initialized_view(cprov, "+index")784 >>> view = create_initialized_view(cprov, "+index")
783785
=== modified file 'lib/lp/registry/browser/tests/test_person_view.py'
--- lib/lp/registry/browser/tests/test_person_view.py 2009-10-16 15:00:55 +0000
+++ lib/lp/registry/browser/tests/test_person_view.py 2009-11-21 14:47:15 +0000
@@ -7,16 +7,17 @@
77
8from zope.component import getUtility8from zope.component import getUtility
99
10from canonical.launchpad.ftests import ANONYMOUS, login
10from canonical.launchpad.webapp.interfaces import NotFoundError11from canonical.launchpad.webapp.interfaces import NotFoundError
11from lp.registry.interfaces.karma import IKarmaCacheManager12from lp.registry.interfaces.karma import IKarmaCacheManager
12from canonical.launchpad.webapp.servers import LaunchpadTestRequest13from canonical.launchpad.webapp.servers import LaunchpadTestRequest
13from canonical.testing import LaunchpadZopelessLayer14from canonical.testing import LaunchpadFunctionalLayer, LaunchpadZopelessLayer
14from lp.registry.browser.person import PersonView15from lp.registry.browser.person import PersonView
15from lp.registry.model.karma import KarmaCategory16from lp.registry.model.karma import KarmaCategory
16from lp.testing import TestCaseWithFactory17from lp.testing import TestCaseWithFactory, login_person
1718
1819
19class TestPersonView(TestCaseWithFactory):20class TestPersonViewKarma(TestCaseWithFactory):
2021
21 layer = LaunchpadZopelessLayer22 layer = LaunchpadZopelessLayer
2223
@@ -69,5 +70,101 @@
69 return karmacache70 return karmacache
7071
7172
73class TestShouldShowPpaSection(TestCaseWithFactory):
74
75 layer = LaunchpadFunctionalLayer
76
77 def setUp(self):
78 TestCaseWithFactory.setUp(self)
79 self.owner = self.factory.makePerson(name='mowgli')
80 self.person_ppa = self.factory.makeArchive(owner=self.owner)
81 self.team = self.factory.makeTeam(name='jbook', owner=self.owner)
82
83 # The team is the owner of the PPA.
84 self.team_ppa = self.factory.makeArchive(owner=self.team)
85 self.team_view = PersonView(self.team, LaunchpadTestRequest())
86
87 def make_ppa_private(self, ppa):
88 """Helper method to privatise a ppa."""
89 login('foo.bar@canonical.com')
90 ppa.private = True
91 ppa.buildd_secret = "secret"
92 login(ANONYMOUS)
93
94 def test_viewing_person_with_public_ppa(self):
95 # Show PPA section only if context has at least one PPA the user is
96 # authorised to view the PPA.
97 login(ANONYMOUS)
98 person_view = PersonView(self.owner, LaunchpadTestRequest())
99 self.failUnless(person_view.should_show_ppa_section)
100
101 def test_viewing_person_without_ppa(self):
102 # If the context person does not have a ppa then the section
103 # should not display.
104 login(ANONYMOUS)
105 person_without_ppa = self.factory.makePerson()
106 person_view = PersonView(person_without_ppa, LaunchpadTestRequest())
107 self.failIf(person_view.should_show_ppa_section)
108
109 def test_viewing_self(self):
110 # If the current user has edit access to the context person then
111 # the section should always display
112 login_person(self.owner)
113 person_view = PersonView(self.owner, LaunchpadTestRequest())
114 self.failUnless(person_view.should_show_ppa_section)
115
116 # If the ppa is private, the section is still displayed to
117 # a user with edit access to the person.
118 self.make_ppa_private(self.person_ppa)
119 login_person(self.owner)
120 person_view = PersonView(self.owner, LaunchpadTestRequest())
121 self.failUnless(person_view.should_show_ppa_section)
122
123 # Even a person without a PPA will see the section when viewing
124 # themselves.
125 person_without_ppa = self.factory.makePerson()
126 login_person(person_without_ppa)
127 person_view = PersonView(person_without_ppa, LaunchpadTestRequest())
128 self.failUnless(person_view.should_show_ppa_section)
129
130 def test_anon_viewing_person_with_private_ppa(self):
131 # If the ppa is private, the ppa section will not be displayed
132 # to users without view access to the ppa.
133 self.make_ppa_private(self.person_ppa)
134 login(ANONYMOUS)
135 person_view = PersonView(self.owner, LaunchpadTestRequest())
136 self.failIf(person_view.should_show_ppa_section)
137
138 # But if the context person has a second ppa that is public,
139 # then anon users will see the section.
140 second_ppa = self.factory.makeArchive(owner=self.owner)
141 person_view = PersonView(self.owner, LaunchpadTestRequest())
142 self.failUnless(person_view.should_show_ppa_section)
143
144 def test_viewing_team_with_private_ppa(self):
145 # If a team PPA is private, the ppa section will be displayed
146 # to team members.
147 self.make_ppa_private(self.team_ppa)
148 member = self.factory.makePerson()
149 login_person(self.owner)
150 self.team.addMember(member, self.owner)
151 login_person(member)
152
153 # So the member will see the section.
154 person_view = PersonView(self.team, LaunchpadTestRequest())
155 self.failUnless(person_view.should_show_ppa_section)
156
157 # But other users who are not members will not.
158 non_member = self.factory.makePerson()
159 login_person(non_member)
160 person_view = PersonView(self.team, LaunchpadTestRequest())
161 self.failIf(person_view.should_show_ppa_section)
162
163 # Unless the team also has another ppa which is public.
164 second_ppa = self.factory.makeArchive(owner=self.team)
165 person_view = PersonView(self.team, LaunchpadTestRequest())
166 self.failUnless(person_view.should_show_ppa_section)
167
168
72def test_suite():169def test_suite():
73 return unittest.TestLoader().loadTestsFromName(__name__)170 return unittest.TestLoader().loadTestsFromName(__name__)