Launchpad itself

Merge lp:~rockstar/launchpad/no-recipes-on-private-branches into lp:launchpad

  1. no-recipes-on-private-branches
  2. Merge into devel
Proposed by Paul Hummer
Status: Merged
Approved by: Curtis Hovey
Approved revision: no longer in the source branch.
Merged at revision: 11362
Proposed branch: lp:~rockstar/launchpad/no-recipes-on-private-branches
Merge into: lp:launchpad
Diff against target: 49 lines (+18/-3)
2 files modified
lib/lp/code/browser/branch.py (+6/-3)
lib/lp/code/browser/tests/test_sourcepackagerecipe.py (+12/-0)
To merge this branch: bzr merge lp:~rockstar/launchpad/no-recipes-on-private-branches
Reviewer Review Type Date Requested Status
Curtis Hovey (community) code Approve
Review via email: mp+32799@code.launchpad.net

Description of the change

This branch fixes bug #612567 - It doesn't show the "Create recipe" link on private branches.

To post a comment you must log in.
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
Revision history for this message
Curtis Hovey (sinzui) wrote :

We spoke on IRC and you explained that we want to switch the link to a JS link and that is not compatible with fmt:link. And since it will be JS, the tests will be in the browser/windmill.

You decided to change the login() call in test setup.

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/code/browser/branch.py'
2--- lib/lp/code/browser/branch.py 2010-08-04 04:07:21 +0000
3+++ lib/lp/code/browser/branch.py 2010-08-16 20:42:46 +0000
4@@ -319,7 +319,10 @@
5 '+upgrade', 'Upgrade this branch', icon='edit', enabled=enabled)
6
7 def create_recipe(self):
8- enabled = config.build_from_branch.enabled
9+ if not self.context.private and config.build_from_branch.enabled:
10+ enabled = True
11+ else:
12+ enabled = False
13 text = 'Create packaging recipe'
14 return Link('+new-recipe', text, enabled=enabled, icon='add')
15
16@@ -564,8 +567,8 @@
17 def show_merge_links(self):
18 """Return whether or not merge proposal links should be shown.
19
20- Merge proposal links should not be shown if there is only one branch in
21- a non-final state.
22+ Merge proposal links should not be shown if there is only one branch
23+ in a non-final state.
24 """
25 if not self.context.target.supports_merge_proposals:
26 return False
27
28=== modified file 'lib/lp/code/browser/tests/test_sourcepackagerecipe.py'
29--- lib/lp/code/browser/tests/test_sourcepackagerecipe.py 2010-08-09 17:14:10 +0000
30+++ lib/lp/code/browser/tests/test_sourcepackagerecipe.py 2010-08-16 20:42:46 +0000
31@@ -136,6 +136,18 @@
32 self.assertTextMatchesExpressionIgnoreWhitespace(
33 pattern, main_text)
34
35+ def test_create_new_recipe_private_branch(self):
36+ # Recipes can't be created on private branches.
37+ with person_logged_in(self.chef):
38+ branch = self.factory.makeBranch(private=True, owner=self.chef)
39+ branch_url = canonical_url(branch)
40+
41+ browser = self.getUserBrowser(branch_url, user=self.chef)
42+ self.assertRaises(
43+ LinkNotFoundError,
44+ browser.getLink,
45+ 'Create packaging recipe')
46+
47 def test_create_new_recipe_users_teams_as_owner_options(self):
48 # Teams that the user is in are options for the recipe owner.
49 self.factory.makeTeam(