launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Review: Approve code

8	+ def contextAllowsNewBugs(self):
9	+ return (self.contextUsesMalone() and
10	+ self.getMainContext().getAllowedBugInformationTypes())

I'm not sure that this needs to check contextUsesMalone(). The only callsite is already wrapped in a contextUsesMalone() check, and it's not really directly relevant to this.

19	+ def contextAllowsNewBugs(self):
20	+ product = self.default_product
21	+ return product is not None and product.getAllowedBugInformationTypes()

I'd just have ProjectGroupFileBugGuidedView.contextAllowsNewBugs always return True. The product's filebug view will reject them once they're redirected to it after selecting a problematic product.

98	+ def test_getAllowedInformationTypesIncludesCurrent(self):

s/IncludesCurrent/_includes_current/

218	+ <tal:no-new-bugs tal:condition="not: view/contextAllowsNewBugs">
219	+ You can't create new bugs for
220	+ <tal:name replace="context/displayname"/>.
221	+ <tal:sharing-link condition="context/required:launchpad.Edit">
222	+ <br>Sharing policies may be changed on the
223	+ <a tal:attributes="href string:${context/fmt:url:mainsite}/+sharing">sharing page</a>.
224	+ </tal:sharing-link>
225	+ </tal:no-new-bugs>

The project owner should probably see something slightly less non sequitur like "You can fix this by _changing the bug sharing policy_.", and it should talk about reporting bugs, not creating them. It could possibly also state that "nobody" can -- not just "you" can't -- create new bugs (the branch listing warning might need a similar revision), but that's just nitpicking and possibly difficult to word.

347	+ if information_type is None:
348	+ raise ValueError(
349	+ 'Branches cannot be created without an information type')

It's possible that an assertion or manual AssertionError is more correct here. The message should at lease be updated; it's not that an information type should have been given, it's that createBranch is being called on a BranchNamespace that is forbidden, which should never happen.

240	=== modified file 'lib/lp/bugs/templates/bugtarget-macros-filebug.pt'
241	--- lib/lp/bugs/templates/bugtarget-macros-filebug.pt 2012-08-01 06:59:20 +0000
242	+++ lib/lp/bugs/templates/bugtarget-macros-filebug.pt 2012-09-20 01:01:27 +0000

Related to the above, I think the project group changes in this template should be reverted. The Forbidden case is exceptional and will be explained after the redirect to Product:+filebug.

378	+ <tal:sharing-link condition="context/required:launchpad.Edit">
379	+ <br>Sharing policies may be changed on the
380	+ <a tal:attributes="href string:${context/fmt:url:mainsite}/+sharing">sharing page</a>.
381	+ </tal:sharing-link>

As with bugs, the sharing policy thing is a bit of a non sequitur.

408	+ FORBIDDEN = DBItem(6, """
409	+ New branches are forbidden
410	+
411	+ No new branches may be created but existing branches may still be
412	+ updated.
413	+ """)

422	+ FORBIDDEN = DBItem(5, """
423	+ New bugs are forbidden
424	+
425	+ No new bugs may be filed but existing bugs may still be updated.
426	+ """)
427	+

The titles of the other sharing policies are single words or fragments. I'd just go with "Forbidden" for both of these. Except for the legacy +filebug view name, we also nowadays refer to "reporting" a bug, not "filing" one. The absence of a comma before "but" is also inconsistent with eg. my description for "Proprietary, can be public" -- I'm happy for it to go either way here, but we should be consistent.

455	+ naked_product.setBranchSharingPolicy(BranchSharingPolicy.FORBIDDEN)
456	+ naked_product.setBugSharingPolicy(BugSharingPolicy.FORBIDDEN)

This will probably blow up if the subscription has expired. setFooSharingPolicy require a commercial subscription for any value other than PUBLIC; you might want to whitelist FORBIDDEN too.
-- 
https://code.launchpad.net/~wallyworld/launchpad/commercial-expired-job-1047026/+merge/125384
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.