Code review comment for lp:~bac/launchpad/bug-436259-add-member

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" />