Launchpad itself

Merge lp://qastaging/~bac/launchpad/bug-436259-add-member into lp://qastaging/launchpad

  1. bug-436259-add-member
  2. Merge into devel
Proposed by Brad Crittenden
Status: Merged
Approved by: Curtis Hovey
Approved revision: no longer in the source branch.
Merged at revision: not available
Proposed branch: lp://qastaging/~bac/launchpad/bug-436259-add-member
Merge into: lp://qastaging/launchpad
Diff against target: 65 lines
3 files modified
lib/lp/registry/browser/person.py (+1/-1)
lib/lp/registry/stories/foaf/xx-team-home.txt (+3/-3)
lib/lp/registry/templates/team-members.pt (+11/-6)
To merge this branch: bzr merge lp://qastaging/~bac/launchpad/bug-436259-add-member
Reviewer Review Type Date Requested Status
Curtis Hovey (community) code Approve
Review via email: mp+13875@code.qastaging.launchpad.net

Commit message

Add the ability to add a member to a team from the +members page. Fixed the layout of the link for accepting or declining proposed members.

To post a comment you must log in.
Revision history for this message
Brad Crittenden (bac) wrote :

= Summary =

Bug 436259 asked for an '(+) Add member' link on the +members page. While doing that
I noticed another layout problem with overlapping icon and text.

== Proposed fix ==

Add the link. Fix the overlap by using sprites as required.

== Pre-implementation notes ==

None.

== Implementation details ==

As above.

== Tests ==

bin/test -vv -t xx-add-member.txt -t xx-approve-members.txt

== Demo and Q/A ==

Look at http://launchpad.net/~some-team-you-administer

= Launchpad lint =

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

Revision history for this message
Curtis Hovey (sinzui) wrote :

Hi Brad thanks for working on this. I have some concerns about the tests, and I think there is a better way to handle the crafted link.

I do not like the LinkNotFoundError tests is stories--they are not stories.

The real concerns we have are: have we set the permissions correctly in the browser/ class, and do we use the correct renderer.

In the case of add_member that is defined in a menu and uses the correct render, I expect a test to verify what the permissions are check_persmission(context, 'launchpad.Edit'). We do not need a browser to verify imprecisely who cannot use the link.

In the case of +editproposedmembers, we have a problem. We are crafting the link. This is bad, because the official definition of the link can change. I think we want to verify the link as above, and render the link as

<li
 tal:define="link context:menu/overview/proposed_members"
 tal:condition="link/enabled">
 <a class="sprite edit"
   tal:attributes="href link/fmt:url">Approve or decline members</a>
</li>

...

OH! This is wrong, well something is. We only redefine link text and icons when the link is in a sentence. The link is defined to have an add icon. The icon should not be different. The text of the link is 'Approve/Decline applicants' Can we just fix the link text to be 'Approve or decline members' and use the edit icon as you suggest. Then you could just render the link using
<a tal:replace="structure context:menu/overview/proposed_members/fmt:link" />

review: Needs Fixing (code)
Revision history for this message
Brad Crittenden (bac) wrote :

The testing changes were completely backed out and the link changed as you requested.

Revision history for this message
Curtis Hovey (sinzui) wrote :

Thanks for you patience. I am glad we found and fix core problem with the test.

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-10-19 22:49:43 +0000
3+++ lib/lp/registry/browser/person.py 2009-10-26 14:58:04 +0000
4@@ -1138,7 +1138,7 @@
5 @enabled_with_permission('launchpad.Edit')
6 def proposed_members(self):
7 target = '+editproposedmembers'
8- text = 'Approve/Decline applicants'
9+ text = 'Approve or decline members'
10 return Link(target, text, icon='add')
11
12 def map(self):
13
14=== modified file 'lib/lp/registry/stories/foaf/xx-team-home.txt'
15--- lib/lp/registry/stories/foaf/xx-team-home.txt 2009-10-13 15:04:42 +0000
16+++ lib/lp/registry/stories/foaf/xx-team-home.txt 2009-10-26 14:58:04 +0000
17@@ -191,10 +191,10 @@
18 Pending approval
19 Sample Person
20 Andrew Bennetts
21- Approve/Decline applicants
22+ Approve or decline members
23
24- >>> owner_browser.getLink('Approve/Decline applicants')
25- <Link text='Approve/Decline applicants' url='.../+editproposedmembers'>
26+ >>> owner_browser.getLink('Approve or decline members')
27+ <Link text='Approve or decline members' url='.../+editproposedmembers'>
28
29
30 == Non members ==
31
32=== modified file 'lib/lp/registry/templates/team-members.pt'
33--- lib/lp/registry/templates/team-members.pt 2009-09-16 18:36:46 +0000
34+++ lib/lp/registry/templates/team-members.pt 2009-10-26 14:58:04 +0000
35@@ -87,6 +87,11 @@
36 <tal:navigation
37 content="structure view/active_memberships/@@+navigation-links-lower" />
38 </div>
39+ <div style="float: right; margin-top: 1em;"
40+ tal:define="link view/menu:overview/add_member"
41+ tal:condition="link/enabled">
42+ <a tal:replace="structure link/fmt:link" />
43+ </div>
44 <br />
45 </li>
46
47@@ -190,12 +195,12 @@
48 </tbody>
49 </table>
50
51- <ul class="edit" tal:condition="user_can_edit_memberships">
52- <li>
53- <a href="+editproposedmembers"
54- >Approve or decline members</a>
55- </li>
56- </ul>
57+ <div style="float:right">
58+ <a tal:define="overview_menu context/menu:overview"
59+ tal:replace="structure overview_menu/proposed_members/fmt:link"
60+ />
61+ </div>
62+
63 </tal:proposed>
64 </li>
65 <br />