Merge lp:~rvb/launchpad/fix-missing-add-distro-link into lp:launchpad

Proposed by Raphaël Badin
Status: Merged
Merged at revision: 12561
Proposed branch: lp:~rvb/launchpad/fix-missing-add-distro-link
Merge into: lp:launchpad
Diff against target: 57 lines (+24/-2)
3 files modified
lib/lp/registry/browser/distribution.py (+3/-1)
lib/lp/registry/browser/menu.py (+5/-0)
lib/lp/soyuz/stories/soyuz/xx-distribution-add.txt (+16/-1)
To merge this branch: bzr merge lp:~rvb/launchpad/fix-missing-add-distro-link
Reviewer Review Type Date Requested Status
j.c.sackett (community) Approve
Review via email: mp+52104@code.launchpad.net

Commit message

[r=jcsackett][bug=728553] Added a 'Register a distribution' link to the distros page.

Description of the change

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Added a link to create a new distribution to the distributions page
(shown only to admin users).

The permission check will probably need to be against another permission
than 'launchpad.Admin' once the permission model for creating
distributions is in place.

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/soyuz/stories/soyuz/xx-distribution-add.txt
  lib/lp/registry/browser/menu.py
  lib/lp/registry/browser/distribution.py

./lib/lp/soyuz/stories/soyuz/xx-distribution-add.txt
       1: narrative uses a moin header.
       3: narrative has trailing whitespace.
       6: source has bad indentation.
      14: source has bad indentation.
      20: source has bad indentation.
      27: source has bad indentation.
      31: source has bad indentation.
      35: source exceeds 78 characters.
      39: source has bad indentation.
      43: source has bad indentation.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iQIcBAEBAgAGBQJNb853AAoJEMC8revFINFckd0P/1p5rcdp1TXvUNOUAoHO5fMD
n5sP05zCUC/2hVCu1d0OOJTFWD9f0XIemKx2g9dIlb9/umqsn4smJ//o2N2Ldfvk
2+6gIc35r7/QfHLMxDhnhm15IV9Ln1XE118OCjj1k5Aclbgnh3PFNMUPTtW5vhHu
BMw19mZaMZMZpnOapOrYR876QNCyh1yn7VzuBwDL+IKn0tcYSKDjy9yvZOAIvLUm
tvp1mVIbWLvnssjqpz0UtWXlwkaeZQf3zPLRdwncW7WJanAgNYourxjCxHXxPGhX
uCD1ap8ufLwUTE8DdlJknYOrtjM2+lUuM96yur4hfsI2otlpGj7TSuKut8lzLkMp
fcJzMECvkEDvodqeAIq+9L95iWT27981NlPV4dNXtAG1w4SIKw+KK2XlvDvjwklR
3zXQ9I6i85UcZAQmEiJwDv+GQMoMJd9Rrze6QiBpewZq+nXQ6RYnGkp5FWPBSmrI
pkBPEi9FhIsVDQtfIUB18OWZSaoBRZuE60noGgbYuz55vHL4cGngcXSZnbOVaxsI
BOEngVMMaJTMhA8BEkBVVcBDAi8msSmWQHSWG+U6lUKXIgtYVgZLC8+c1/poOGo2
JGYaeEn6sDdcFOtEz5vEwXi+30EOl1RtpeyBRmdNrLAC5Kf7iffuNMDYwGCDO/Xw
xRHdSkx+0oPrpdYbI57m
=6flm
-----END PGP SIGNATURE-----

To post a comment you must log in.
Revision history for this message
j.c.sackett (jcsackett) wrote :

> === modified file 'lib/lp/registry/browser/menu.py'
> --- lib/lp/registry/browser/menu.py 2010-08-20 20:31:18 +0000
> +++ lib/lp/registry/browser/menu.py 2011-03-03 17:49:43 +0000
> @@ -47,6 +47,14 @@
> text = 'Register a team'
> return Link('/people/+newteam', text, icon='add')
>
> + # TODO: rvb 20110103 DISTRO.PERMISSIONS
> + # this should be checked against the right permission once
> + # the permission model for distributions is in place
> + @enabled_with_permission('launchpad.Admin')
> + def register_distribution(self):
> + text = 'Register a distribution'
> + return Link('/distros/+add', text, icon='add')
> +
> def create_account(self):
> text = 'Create an account'
> # Only enable this link for anonymous users.

The TODO above should really be put in as an XXX, and probably have an associated bug. See https://dev.launchpad.net/PolicyandProcess/XXXPolicy for more info on what I mean.

I'm unsure if a decorator or a zcml entry would be the best way to lock down permissions on this. Did you have a particular rationale for going with the decorator?

> === modified file 'lib/lp/soyuz/stories/soyuz/xx-distribution-add.txt'
> --- lib/lp/soyuz/stories/soyuz/xx-distribution-add.txt 2009-08-13 15:12:16 +0000
> +++ lib/lp/soyuz/stories/soyuz/xx-distribution-add.txt 2011-03-03 17:49:43 +0000
> @@ -1,6 +1,21 @@
> = Creating new distributions =
>
> -This can be done only by launchpad admins.
> +A non launchpad admin doesn't see the link to create a new distribution on
> +the distributions page:
> +
> + >>> user_browser.open("http://launchpad.dev/distros")
> + >>> user_browser.getLink("Register a distribution")
> + Traceback (most recent call last):
> + ...
> + LinkNotFoundError
> +
> +A launchpad admin sees the link to create a new distribution:
> +
> + >>> admin_browser.open("http://launchpad.dev/distros")
> + >>> admin_browser.getLink("Register a distribution").url
> + 'http://launchpad.dev/distros/+add'
> +
> +A launchpad admin can create a new distribution:
>
> >>> user_browser.open("http://launchpad.dev/distros/+add")
> Traceback (most recent call last):

This story looks fine, but I would really prefer a unittest instead of or in addition to this. Historically we've over-relied on doctests and stories, which don't perfectly isolate things that may be failing.

I see that you had a unittest in a previous revision, that actually looks pretty good, though it can probably be added to lib/lp/registry/browser/tests. What was the reason to convert it to a story?

review: Needs Information
Revision history for this message
Julian Edwards (julian-edwards) wrote :

I told him to edit the story test because this is a story change, it's not a unit test.

Also, the decorator is the standard way of enabling links in the UI. Check out some of the other browser code.

rvb, you also need to do that formatting change on the list of links :)

Revision history for this message
Raphaël Badin (rvb) wrote :

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

> The TODO above should really be put in as an XXX, and probably have an associated bug. See https://dev.launchpad.net/PolicyandProcess/XXXPolicy for more info on what I mean.

After checking with bigjools, only the perm model for distroseries will
change, so I removed the comment. Thanks for the link though.

> I'm unsure if a decorator or a zcml entry would be the best way to lock down permissions on this. Did you have a particular rationale for going with the decorator?

To be honest I must say that being still faily new to the code I tend to
reproduce the kind of mechanisms I found already in place. I also think
that decorators, with their AOP like syntax and readability are very
well suited to address crosscutting concerns like permission checking.
I'll gladly look into another way of doing this if you think there is a
better way.

> This story looks fine, but I would really prefer a unittest instead of or in addition to this. Historically we've over-relied on doctests and stories, which don't perfectly isolate things that may be failing.
>
> I see that you had a unittest in a previous revision, that actually looks pretty good, though it can probably be added to lib/lp/registry/browser/tests. What was the reason to convert it to a story?

I started with a unittest for the very reason you mentioned but was then
advised by bigjools to check out this existing story which was indeed
the right place to add my test because a) my tests are pretty simple and
are only checking things in the UI and b) they really belong to this
story that was created to show how to create a distribution (but was
only checking the form to add a distribution, not the presence of a link
to that form)

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iQIcBAEBAgAGBQJNcLVUAAoJEMC8revFINFcQQMQAI12MZVf2Y033EcCaO95Jibq
Ehd0cG776PVqe0PPH6T+ycFZhrlTkyunwcuDmQIg3gSsh3FEV1MyXjdeIQnbTg73
Uuv1GMeP7fkDqo4xP690LY39V8nmUgY79UUbwFZQgekXGZ2FMMWYN8SRabWsor0A
rEC1wIMuFOXqfRYEGJ6WqDBtLLmX5c3ogruO4aIj060O2PO57Yvr3vxhx2Ph9oKW
fGjsYdb3czWWslE86fCU6q2SJ69l75TaOd7xlUWyJDlS0IysjA8TNGR2CO0Tu4ny
L1FUXusoNmWHYb+xn2lnaBPmpqC7+06oj+w4QyOczrTU0rezvdf+1KMehhopwzJn
hLtprdpZpmy0b9lBkHXS9A5mmnI/8UisTN3YDgFC97hd+jx3MJYoeukK1qmJ0t64
PhwfMct0WHbBbWV6E6EOerGXuUUIRpqeL2K8R2JeBbgJ2nS3ulg2Jt1sO/IaT+Cd
UitW8po12+wSrtjtPs22u13mRAmX0sm5Yf5iWgqu8kFfw2OLWHnTU0lQ/SR47p5b
kC/1jonStjHm2aIvTrlAKoz0g8UzCO1aCAfWKcCITbCqk8IGtnrqIBoX+CY1lZio
CLoqg+T+8pWA0Bg5j1GVuuvcFAbJnb/INTrq8D+qwGqd5snt1daMhoNgpzPlbSqn
ANsJVpb7K7jOfGAvAI9g
=01rX
-----END PGP SIGNATURE-----

Revision history for this message
j.c.sackett (jcsackett) wrote :

Thanks for addressing my questions, both of you.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/registry/browser/distribution.py'
2--- lib/lp/registry/browser/distribution.py 2011-02-03 10:35:36 +0000
3+++ lib/lp/registry/browser/distribution.py 2011-03-04 09:58:20 +0000
4@@ -714,7 +714,9 @@
5 """Action menu for `DistributionSetView`."""
6
7 usedfor = IDistributionSet
8- links = ['register_team', 'register_project', 'create_account']
9+ links = [
10+ 'register_team', 'register_project', 'register_distribution',
11+ 'create_account']
12
13
14 class DistributionSetView(LaunchpadView):
15
16=== modified file 'lib/lp/registry/browser/menu.py'
17--- lib/lp/registry/browser/menu.py 2010-08-20 20:31:18 +0000
18+++ lib/lp/registry/browser/menu.py 2011-03-04 09:58:20 +0000
19@@ -47,6 +47,11 @@
20 text = 'Register a team'
21 return Link('/people/+newteam', text, icon='add')
22
23+ @enabled_with_permission('launchpad.Admin')
24+ def register_distribution(self):
25+ text = 'Register a distribution'
26+ return Link('/distros/+add', text, icon='add')
27+
28 def create_account(self):
29 text = 'Create an account'
30 # Only enable this link for anonymous users.
31
32=== modified file 'lib/lp/soyuz/stories/soyuz/xx-distribution-add.txt'
33--- lib/lp/soyuz/stories/soyuz/xx-distribution-add.txt 2009-08-13 15:12:16 +0000
34+++ lib/lp/soyuz/stories/soyuz/xx-distribution-add.txt 2011-03-04 09:58:20 +0000
35@@ -1,6 +1,21 @@
36 = Creating new distributions =
37
38-This can be done only by launchpad admins.
39+A non launchpad admin doesn't see the link to create a new distribution on
40+the distributions page:
41+
42+ >>> user_browser.open("http://launchpad.dev/distros")
43+ >>> user_browser.getLink("Register a distribution")
44+ Traceback (most recent call last):
45+ ...
46+ LinkNotFoundError
47+
48+A launchpad admin sees the link to create a new distribution:
49+
50+ >>> admin_browser.open("http://launchpad.dev/distros")
51+ >>> admin_browser.getLink("Register a distribution").url
52+ 'http://launchpad.dev/distros/+add'
53+
54+A launchpad admin can create a new distribution:
55
56 >>> user_browser.open("http://launchpad.dev/distros/+add")
57 Traceback (most recent call last):