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

[Karl Fogel]

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.

[Karl Fogel (kfogel)]

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.

[Eleanor Berger (intellectronica)]

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

[Karl Fogel (kfogel)]

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.

[Eleanor Berger (intellectronica)]

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'?

[Karl Fogel (kfogel)]

Duh. Yes. Please see rev 9028.

[Eleanor Berger (intellectronica)]

review: Approve (code ui)

=== 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 @@
         else:
             return 'Patch attachments in %s' % self.context.displayname
 
+    @property
+    def patch_task_orderings(self):
+        """The list of possible sort orderings for the patches view.
+
+        The orderings are a list of tuples of the form:
+          [(DisplayName, InternalOrderingName), ...]
+        For example:
+          [("Patch age", "-latest_patch_uploaded"),
+           ("Importance", "-importance"),
+           ...]
+        """
+        orderings = [("patch age", "-latest_patch_uploaded"),
+                     ("importance", "-importance"),
+                     ("status", "status"),
+                     ("oldest first", "datecreated"),
+                     ("newest first", "-datecreated")]
+        targetname = self.targetName()
+        if targetname is not None:
+            # Lower case for consistency with the other orderings.
+            orderings.append((targetname.lower(), "targetname"))
+        return orderings
+
+
     def batchedPatchTasks(self):
         """Return a BatchNavigator for bug tasks with patch attachments."""
-        # XXX: Karl Fogel 2010-02-01 bug=515584: we should be using a
-        # Zope form instead of validating the values by hand in the
-        # code.  Doing it the Zope form way would specify rendering
-        # and validation from the same enum, and thus observe DRY.
         orderby = self.request.get("orderby", "-latest_patch_uploaded")
-        if orderby not in ["-latest_patch_uploaded", "-importance", "status",
-                           "targetname", "datecreated", "-datecreated"]:
+        if orderby not in [x[1] for x in self.patch_task_orderings]:
             raise UnexpectedFormData(
                 "Unexpected value for field 'orderby': '%s'" % orderby)
         return BatchNavigator(

=== 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 @@
     </script>
 
     Order&nbsp;by:&nbsp;<select
-                    name="orderby" id="orderby" size="1"
-                    tal:define="orderby request/orderby|string:-latest_patch_uploaded">
-      <option
-        value="-latest_patch_uploaded"
-        tal:attributes="selected python:orderby == '-latest_patch_uploaded'"
-      >patch age</option>
-      <option
-        value="-importance"
-        tal:attributes="selected python:orderby == '-importance'"
-      >importance</option>
-      <option
-        value="status"
-        tal:attributes="selected python:orderby == 'status'"
-      >status</option>
-      <option
-        tal:condition="view/targetName"
-        tal:content="view/targetName"
-        tal:attributes="selected python:orderby == 'targetname'"
-        value="targetname"
-      ></option>
-      <option
-        value="datecreated"
-        tal:attributes="selected python:orderby == 'datecreated'"
-      >oldest first</option>
-      <option
-        value="-datecreated"
-        tal:attributes="selected python:orderby == '-datecreated'"
-      >newest first</option>
+          name="orderby" id="orderby" size="1"
+          tal:define="orderby request/orderby|string:-latest_patch_uploaded">
+      <option tal:repeat="ordering view/patch_task_orderings"
+              tal:attributes="value python: ordering[1];
+                              selected python:orderby == ordering[1]"
+              tal:content="python: ordering[0]"></option>
     </select>
     <input type="submit" value="sort" id="sort-button"/>
   </form>