Launchpad itself

Merge lp:~kfogel/launchpad/515584-fix-DRY-violation into lp:launchpad/db-devel

  1. 515584-fix-DRY-violation
  2. Merge into db-devel
Proposed by Karl Fogel
Status: Merged
Approved by: Eleanor Berger
Approved revision: not available
Merged at revision: not available
Proposed branch: lp:~kfogel/launchpad/515584-fix-DRY-violation
Merge into: lp:launchpad/db-devel
Diff against target: 88 lines (+30/-34)
2 files modified
lib/lp/bugs/browser/bugtarget.py (+24/-6)
lib/lp/bugs/templates/bugtarget-patches.pt (+6/-28)
To merge this branch: bzr merge lp:~kfogel/launchpad/515584-fix-DRY-violation
Reviewer Review Type Date Requested Status
Eleanor Berger (community) code ui Approve
Review via email: mp+19990@code.launchpad.net

Commit message

Fix a DRY violation between the +patches template and view. Note that although some of the commits say they're about using a Zope form, we went with a different solution instead. See the bug for details.

To post a comment you must log in.
Revision history for this message
Karl Fogel (kfogel) wrote :

Fix a DRY violation between the +patches template and view. Note that although the original plan was to use a Zope form, we ended up using a different solution instead. See the comments at bug #515584 for why.

Note there is a very trivial UI change here: the sort orderings have slightly different user-visible names (e.g., "Patch age" instead of "patch age"). I'm not sure if that needs UI review or not.

Revision history for this message
Eleanor Berger (intellectronica) wrote :

Hi Karl,

All-in-all this branch looks nice. I have one code-related comment: the property patchTaskOrderings should be called patch_task_orderings. Remember that even if its implementation is a method, from the interface perspective it looks just like another attribute, so it should follow the attribute naming convention.

As for the UI, I think there are two issues:

1. You're now capitalizing the sort orderings, which is different from the normal search sort order. I think that both should have the same capitalization, so I would prefer not to capitalize for now.

2. Where previously you displayed the target name (package, project) you now display 'Target', but I don't think this is a good idea, because nowhere else do we use the word target to mean what we mean here, and it will probably not be obvious to users what it means. So I think it's better to continue using targetName.

review: Needs Fixing
Revision history for this message
Karl Fogel (kfogel) wrote :

Thanks for the review, Tom. I've done as you suggest (except I used "target name" rather than "targetName", to avoid inconsistent camelCase). The change is undergoing EC2 testing. Let me know how the code looks now.

Revision history for this message
Eleanor Berger (intellectronica) wrote :

Wasn't the use of targetName (camel cased, for FWIW) in the previous version, a call to a function that returns a string with the type of target? Something like 'project' or 'package'?

Revision history for this message
Karl Fogel (kfogel) wrote :

Duh. Yes. Please see rev 9028.

Revision history for this message
Eleanor Berger (intellectronica) :
review: Approve (code ui)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'lib/lp/bugs/browser/bugtarget.py'
--- lib/lp/bugs/browser/bugtarget.py 2010-02-23 15:34:15 +0000
+++ lib/lp/bugs/browser/bugtarget.py 2010-02-24 15:21:20 +0000
@@ -1283,15 +1283,33 @@
1283 else:1283 else:
1284 return 'Patch attachments in %s' % self.context.displayname1284 return 'Patch attachments in %s' % self.context.displayname
12851285
1286 @property
1287 def patch_task_orderings(self):
1288 """The list of possible sort orderings for the patches view.
1289
1290 The orderings are a list of tuples of the form:
1291 [(DisplayName, InternalOrderingName), ...]
1292 For example:
1293 [("Patch age", "-latest_patch_uploaded"),
1294 ("Importance", "-importance"),
1295 ...]
1296 """
1297 orderings = [("patch age", "-latest_patch_uploaded"),
1298 ("importance", "-importance"),
1299 ("status", "status"),
1300 ("oldest first", "datecreated"),
1301 ("newest first", "-datecreated")]
1302 targetname = self.targetName()
1303 if targetname is not None:
1304 # Lower case for consistency with the other orderings.
1305 orderings.append((targetname.lower(), "targetname"))
1306 return orderings
1307
1308
1286 def batchedPatchTasks(self):1309 def batchedPatchTasks(self):
1287 """Return a BatchNavigator for bug tasks with patch attachments."""1310 """Return a BatchNavigator for bug tasks with patch attachments."""
1288 # XXX: Karl Fogel 2010-02-01 bug=515584: we should be using a
1289 # Zope form instead of validating the values by hand in the
1290 # code. Doing it the Zope form way would specify rendering
1291 # and validation from the same enum, and thus observe DRY.
1292 orderby = self.request.get("orderby", "-latest_patch_uploaded")1311 orderby = self.request.get("orderby", "-latest_patch_uploaded")
1293 if orderby not in ["-latest_patch_uploaded", "-importance", "status",1312 if orderby not in [x[1] for x in self.patch_task_orderings]:
1294 "targetname", "datecreated", "-datecreated"]:
1295 raise UnexpectedFormData(1313 raise UnexpectedFormData(
1296 "Unexpected value for field 'orderby': '%s'" % orderby)1314 "Unexpected value for field 'orderby': '%s'" % orderby)
1297 return BatchNavigator(1315 return BatchNavigator(
12981316
=== modified file 'lib/lp/bugs/templates/bugtarget-patches.pt'
--- lib/lp/bugs/templates/bugtarget-patches.pt 2010-02-09 20:10:31 +0000
+++ lib/lp/bugs/templates/bugtarget-patches.pt 2010-02-24 15:21:20 +0000
@@ -31,34 +31,12 @@
31 </script>31 </script>
3232
33 Order&nbsp;by:&nbsp;<select33 Order&nbsp;by:&nbsp;<select
34 name="orderby" id="orderby" size="1"34 name="orderby" id="orderby" size="1"
35 tal:define="orderby request/orderby|string:-latest_patch_uploaded">35 tal:define="orderby request/orderby|string:-latest_patch_uploaded">
36 <option36 <option tal:repeat="ordering view/patch_task_orderings"
37 value="-latest_patch_uploaded"37 tal:attributes="value python: ordering[1];
38 tal:attributes="selected python:orderby == '-latest_patch_uploaded'"38 selected python:orderby == ordering[1]"
39 >patch age</option>39 tal:content="python: ordering[0]"></option>
40 <option
41 value="-importance"
42 tal:attributes="selected python:orderby == '-importance'"
43 >importance</option>
44 <option
45 value="status"
46 tal:attributes="selected python:orderby == 'status'"
47 >status</option>
48 <option
49 tal:condition="view/targetName"
50 tal:content="view/targetName"
51 tal:attributes="selected python:orderby == 'targetname'"
52 value="targetname"
53 ></option>
54 <option
55 value="datecreated"
56 tal:attributes="selected python:orderby == 'datecreated'"
57 >oldest first</option>
58 <option
59 value="-datecreated"
60 tal:attributes="selected python:orderby == '-datecreated'"
61 >newest first</option>
62 </select>40 </select>
63 <input type="submit" value="sort" id="sort-button"/>41 <input type="submit" value="sort" id="sort-button"/>
64 </form>42 </form>

Subscribers

People subscribed via source and target branches

to status/vote changes: