Code review comment for lp:~rockstar/launchpad/no-recipes-on-private-branches

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

I would have used
    login_person(self.chef)
or removed the need for logout using
    with person_logged_in(self.chef):

Also you changed the menu, but test the template; I would have tested the view. I was puzzled and saw in the templates:

    <span
      tal:define="link context_menu/create_recipe"
      tal:condition="link/enabled"
      tal:replace="structure link/render"
      />

That is odd, because I think we have a safty net in the formatters for this that ensure a disabled link is never rendered:

    <a tal:replace="context_menu/create_recipe/fmt:link" />

I can see why you tested the template, but should it have an independent rule?

review: Needs Information

« Back to merge proposal