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
1=== modified file 'lib/lp/bugs/browser/bugtarget.py'
2--- lib/lp/bugs/browser/bugtarget.py 2010-02-23 15:34:15 +0000
3+++ lib/lp/bugs/browser/bugtarget.py 2010-02-24 15:21:20 +0000
4@@ -1283,15 +1283,33 @@
5 else:
6 return 'Patch attachments in %s' % self.context.displayname
7
8+ @property
9+ def patch_task_orderings(self):
10+ """The list of possible sort orderings for the patches view.
11+
12+ The orderings are a list of tuples of the form:
13+ [(DisplayName, InternalOrderingName), ...]
14+ For example:
15+ [("Patch age", "-latest_patch_uploaded"),
16+ ("Importance", "-importance"),
17+ ...]
18+ """
19+ orderings = [("patch age", "-latest_patch_uploaded"),
20+ ("importance", "-importance"),
21+ ("status", "status"),
22+ ("oldest first", "datecreated"),
23+ ("newest first", "-datecreated")]
24+ targetname = self.targetName()
25+ if targetname is not None:
26+ # Lower case for consistency with the other orderings.
27+ orderings.append((targetname.lower(), "targetname"))
28+ return orderings
29+
30+
31 def batchedPatchTasks(self):
32 """Return a BatchNavigator for bug tasks with patch attachments."""
33- # XXX: Karl Fogel 2010-02-01 bug=515584: we should be using a
34- # Zope form instead of validating the values by hand in the
35- # code. Doing it the Zope form way would specify rendering
36- # and validation from the same enum, and thus observe DRY.
37 orderby = self.request.get("orderby", "-latest_patch_uploaded")
38- if orderby not in ["-latest_patch_uploaded", "-importance", "status",
39- "targetname", "datecreated", "-datecreated"]:
40+ if orderby not in [x[1] for x in self.patch_task_orderings]:
41 raise UnexpectedFormData(
42 "Unexpected value for field 'orderby': '%s'" % orderby)
43 return BatchNavigator(
44
45=== modified file 'lib/lp/bugs/templates/bugtarget-patches.pt'
46--- lib/lp/bugs/templates/bugtarget-patches.pt 2010-02-09 20:10:31 +0000
47+++ lib/lp/bugs/templates/bugtarget-patches.pt 2010-02-24 15:21:20 +0000
48@@ -31,34 +31,12 @@
49 </script>
50
51 Order&nbsp;by:&nbsp;<select
52- name="orderby" id="orderby" size="1"
53- tal:define="orderby request/orderby|string:-latest_patch_uploaded">
54- <option
55- value="-latest_patch_uploaded"
56- tal:attributes="selected python:orderby == '-latest_patch_uploaded'"
57- >patch age</option>
58- <option
59- value="-importance"
60- tal:attributes="selected python:orderby == '-importance'"
61- >importance</option>
62- <option
63- value="status"
64- tal:attributes="selected python:orderby == 'status'"
65- >status</option>
66- <option
67- tal:condition="view/targetName"
68- tal:content="view/targetName"
69- tal:attributes="selected python:orderby == 'targetname'"
70- value="targetname"
71- ></option>
72- <option
73- value="datecreated"
74- tal:attributes="selected python:orderby == 'datecreated'"
75- >oldest first</option>
76- <option
77- value="-datecreated"
78- tal:attributes="selected python:orderby == '-datecreated'"
79- >newest first</option>
80+ name="orderby" id="orderby" size="1"
81+ tal:define="orderby request/orderby|string:-latest_patch_uploaded">
82+ <option tal:repeat="ordering view/patch_task_orderings"
83+ tal:attributes="value python: ordering[1];
84+ selected python:orderby == ordering[1]"
85+ tal:content="python: ordering[0]"></option>
86 </select>
87 <input type="submit" value="sort" id="sort-button"/>
88 </form>

Subscribers

People subscribed via source and target branches

to status/vote changes: