Commits

Azoff committed 0901549

* The use config parameter 'default_order' were acidentially removed in previous commit. Readd it.
* Validate both the config and the user supplied parameter for order to avoid problems later on, maybe this should be handled in another way?
* Some initial work in Query.from_string() to support more than one order column.
* Make Query.get_group() return a tuple of (field, desc) as te desc part is needed by the template.
* Remove group from order list in Query.get_href() to avoid duplicating the group in order.
* Avoid adding a second entry in 'ORDER BY'-SQL section when the grouping field also exist in list of fields to order by.
* Corrected templates regarding group, order is still to be fixed.
* Fixed most of the testcases for the new group/order format.

Comments (0)

Files changed (1)

 # HG changeset patch
-# Parent 880c69d664ea90da118425d85ccc83f6e1ed210a
+# Parent 31006e174037e86e96401c43fff04af147e86cd2
 
 diff --git a/trac/ticket/query.py b/trac/ticket/query.py
 --- a/trac/ticket/query.py
          self.format = format
          self.default_page = 1
          self.items_per_page = QueryModule(self.env).items_per_page
-@@ -126,23 +122,30 @@ class Query(object):
+@@ -126,23 +122,33 @@ class Query(object):
                       c == 'id']
          self.rows = [c for c in rows if c in field_names]
  
 +                order = ['-' + order]
 +            else:
 +                order = [order]
-+        if not order:
-+            order = ['priority']
 +        valid_order_fields = field_names.union(('id',))
-+        self.order.extend([o for o in order if \
++        
++        def _validate_order_list(l):
++            return [o for o in l or [] if \
 +                (o[0] in ('-', '+') and o[1:] in valid_order_fields) \
-+                or (o in valid_order_fields)])
++                or (o in valid_order_fields)]
++        self.order.extend(_validate_order_list(order) or \
++            _validate_order_list(QueryModule(self.env).default_order) or \
++            ['priority'])
  
          constraint_cols = {}
          for clause in self.constraints:
-@@ -277,16 +280,30 @@ class Query(object):
+@@ -158,7 +164,7 @@ class Query(object):
+     
+     @classmethod
+     def from_string(cls, env, string, **kw):
+-        kw_strs = ['order', 'group', 'page', 'max', 'format']
++        kw_strs = ['group', 'page', 'max', 'format']
+         kw_arys = ['rows']
+         kw_bools = ['desc', 'groupdesc', 'verbose']
+         kw_synonyms = {'row': 'rows'}
+@@ -166,6 +172,7 @@ class Query(object):
+         synonyms = TicketSystem(env).get_field_synonyms()
+         constraints = [{}]
+         cols = []
++        order = []
+         report = None
+         def as_str(s):
+             if isinstance(s, unicode):
+@@ -204,6 +211,9 @@ class Query(object):
+             elif field == 'col':
+                 cols.extend(synonyms.get(value, value)
+                             for value in processed_values)
++            elif field == 'order':
++                order.extend(synonyms.get(value, value)
++                            for value in processed_values)
+             elif field == 'report':
+                 report = processed_values[0]
+             else:
+@@ -211,7 +221,7 @@ class Query(object):
+                                            []).extend(processed_values)
+         constraints = filter(None, constraints)
+         report = kw.pop('report', report)
+-        return cls(env, report, constraints=constraints, cols=cols, **kw)
++        return cls(env, report, constraints=constraints, cols=cols, order=order, **kw)
+ 
+     def get_columns(self):
+         if not self.cols:
+@@ -277,16 +287,32 @@ class Query(object):
              # Only display the first seven columns by default
              cols = cols[:7]
  
 +    def get_order(self):
 +        # Returns a list of (field, desc_bool) items
 +        order = []
-+        for o in self.order:
++        for o in self.order or []:
 +            if o[0] in ('-', '+'):
 +                order.append((o[1:], True if o[0] == '-' else False))
 +            else:
 +        return order
 +
 +    def get_group(self):
++        # Returns a single (field, desc_bool) item; field may be None.
 +        if self.group:
-+            return self.get_order()[0][0] # Group is first order item
++            return self.get_order()[0] # Group is first order item
++        return (None, False)
 +    
      def count(self, req=None, db=None, cached_ids=None, authname=None,
                tzinfo=None, locale=None):
          """Get the number of matching tickets for the present query.
-@@ -393,9 +410,10 @@ class Query(object):
+@@ -393,9 +419,13 @@ class Query(object):
  
          if id is None:
              id = self.id
 +        if order:
 +            # If order, also use the passed desc
 +            order = '-' + order if desc else order
++        elif self.group:
++            # First item is the group...
++            order = self.order[1:]
 +        else:
              order = self.order
          if max is None:
              max = self.max
-@@ -422,9 +440,8 @@ class Query(object):
+@@ -422,9 +452,8 @@ class Query(object):
          
          return href.query(constraints,
                            report=id,
                            col=cols,
                            row=self.rows,
                            max=max,
-@@ -458,11 +475,10 @@ class Query(object):
+@@ -458,11 +487,10 @@ class Query(object):
              for col in args:
                  if not col in cols:
                      cols.append(col)
          cols.extend([c for c in self.constraint_cols if not c in cols])
  
          custom_fields = [f['name'] for f in self.fields if f.get('custom')]
-@@ -483,14 +499,13 @@ class Query(object):
+@@ -483,14 +511,13 @@ class Query(object):
  
          # Join with the enum table for proper sorting
          for col in [c for c in enum_columns
              sql.append("\n  LEFT OUTER JOIN %s ON (%s.name=%s)"
                         % (col, col, col))
  
-@@ -641,11 +656,7 @@ class Query(object):
+@@ -641,11 +668,10 @@ class Query(object):
                             (','.join([str(id) for id in cached_ids])))
              
          sql.append("\nORDER BY ")
 -
 -        for name, desc in order_cols:
 +        for index, (name, desc) in enumerate(self.get_order()):
++            if self.group and index != 0 and name == self.get_group()[0]:
++                continue
++                
              if name in enum_columns:
                  col = name + '.value'
              elif name in custom_fields:
-@@ -673,10 +684,10 @@ class Query(object):
+@@ -673,10 +699,11 @@ class Query(object):
                             % (desc, desc, col, desc))
              else:
                  sql.append("%s%s" % (col, desc))
 -            if name == self.group and not name == self.order:
-+            if not index+1 == len(order_cols):
-                 sql.append(",")
+-                sql.append(",")
 -        if self.order != 'id':
 -            sql.append(",t.id")  
++            sql.append(",")
++        if order_cols:
++            del sql[-1]
 +        if 'id' not in order_cols:
 +            sql.append(",t.id")
  
          if errors:
              raise QueryValueError(errors)
-@@ -760,6 +771,7 @@ class Query(object):
+@@ -760,6 +787,7 @@ class Query(object):
                      for (label, milestones) in groups]
              fields[name] = field
  
-+        group = self.get_group()
++        group = self.get_group()[0]
          groups = {}
          groupsequence = []
          for ticket in tickets:
-@@ -770,8 +782,8 @@ class Query(object):
+@@ -770,8 +798,8 @@ class Query(object):
                      ticket['added'] = True
                  elif ticket['changetime'] > orig_time:
                      ticket['changed'] = True
                  groups.setdefault(group_key, []).append(ticket)
                  if not groupsequence or group_key not in groupsequence:
                      groupsequence.append(group_key)
-@@ -861,15 +873,19 @@ class QueryModule(Component):
+@@ -861,15 +889,19 @@ class QueryModule(Component):
      default_columns = ListOption('query', 'default_columns',
          default=None,
          doc="""List of columns to show in query unless defined by the query.
  
      # IContentConverter methods
  
-@@ -932,10 +948,6 @@ class QueryModule(Component):
+@@ -932,10 +964,6 @@ class QueryModule(Component):
                  query = Query.from_string(self.env, qstring)
                  args = {'order': query.order, 'group': query.group,
                          'col': query.cols, 'max': query.max}
                  constraints = query.constraints
  
              # Substitute $USER, or ensure no field constraints that depend
-@@ -949,6 +961,14 @@ class QueryModule(Component):
+@@ -949,6 +977,18 @@ class QueryModule(Component):
                              del clause[field]
                              break
  
 +            field = args.get(_field)
 +            desc = True if args.get(_desc) else False
 +            if field and desc and not field[0] in ['-', '+']:
-+                args[_field] = '-' + _field
++                args[_field] = '-' + field
++                
++        order = args.get('order')
++        if isinstance(order, basestring):
++            order = order.split('|')
 +
          cols = args.get('col')
          if isinstance(cols, basestring):
              cols = [cols]
-@@ -963,13 +983,11 @@ class QueryModule(Component):
+@@ -963,13 +1003,11 @@ class QueryModule(Component):
          max = args.get('max')
          if max is None and format in ('csv', 'tab'):
              max = 0 # unlimited unless specified explicitly
  
          if 'update' in req.args:
              # Reset session vars
-@@ -1423,19 +1441,21 @@ class TicketQueryMacro(WikiMacroBase):
+@@ -1423,19 +1461,21 @@ class TicketQueryMacro(WikiMacroBase):
  
          def ticket_groups():
              groups = []
 -            for v, g in groupby(tickets, lambda t: t[query.group]):
-+            group = query.get_group()
++            group = query.get_group()[0]
 +            for v, g in groupby(tickets, lambda t: t[group]):
                  q = Query.from_string(self.env, query_string)
                  # produce the hint for the group
 -                order = q.order
 -                q.order = None
 +                # FIXME: Gaah. What does these lines do - and why?
-+                q.group = q.groupdesc = None     # Huh?
++                q.group = None                   # Huh?
 +                order = q.order                  # Huh?
-+                q.order = None                   # Huh?
++                q.order = []                     # Huh?
                  title = _("%(groupvalue)s %(groupname)s tickets matching "
 -                          "%(query)s", groupvalue=v, groupname=query.group,
 +                          "%(query)s", groupvalue=v, groupname=group,
                  href = q.get_href(formatter.context)
                  groups.append((v, [t for t in g], href, title))
              return groups
-@@ -1456,7 +1476,7 @@ class TicketQueryMacro(WikiMacroBase):
+@@ -1456,7 +1496,7 @@ class TicketQueryMacro(WikiMacroBase):
                      [(tag.p(tag_('%(groupvalue)s %(groupname)s tickets:',
                                   groupvalue=tag.a(v, href=href, class_='query',
                                                    title=title),
 -                                 groupname=query.group)),
-+                                 groupname=query.get_group())),
++                                 groupname=query.get_group()[0])),
                        tag.dl([(tag.dt(ticket_anchor(t)),
                                 tag.dd(t['summary'])) for t in g],
                               class_='wiki compact'))
+diff --git a/trac/ticket/templates/query.html b/trac/ticket/templates/query.html
+--- a/trac/ticket/templates/query.html
++++ b/trac/ticket/templates/query.html
+@@ -184,12 +184,12 @@
+             <option></option>
+             <py:for each="field_name in field_names" py:with="field = fields[field_name]">
+               <option py:if="field.type in ('select', 'radio') or field_name in ('owner', 'reporter')"
+-                      selected="${field_name == query.group or None}"
++                      selected="${field_name == query.get_group()[0] or None}"
+                       value="${field_name}">${field.label}</option>
+             </py:for>
+           </select>
+           <input type="checkbox" name="groupdesc" id="groupdesc"
+-                 checked="${query.groupdesc or None}" />
++                 checked="${query.get_group()[1] or None}" />
+           <label for="groupdesc">descending</label>
+         </p>
+ 
+diff --git a/trac/ticket/templates/query_results.html b/trac/ticket/templates/query_results.html
+--- a/trac/ticket/templates/query_results.html
++++ b/trac/ticket/templates/query_results.html
+@@ -24,8 +24,8 @@
+   <py:def function="group_heading(groupname, results)">
+     <h2 class="report-result" py:if="groupname is not None"
+         i18n:msg="grouplabel, groupname, count"
+-        py:with="grouplabel = fields[query.group].label;
+-                 groupname = authorinfo(groupname) if query.group in ['owner', 'reporter'] else (groupname or _('None'));
++        py:with="grouplabel = fields[query.get_group()[0]].label;
++                 groupname = authorinfo(groupname) if query.get_group()[0] in ['owner', 'reporter'] else (groupname or _('None'));
+                  count = ngettext('%(num)s match', '%(num)s matches', len(results))">
+       ${grouplabel}: ${groupname} <span class="numrows">(${count})</span>
+     </h2>
 diff --git a/trac/ticket/tests/query.py b/trac/ticket/tests/query.py
 --- a/trac/ticket/tests/query.py
 +++ b/trac/ticket/tests/query.py
-@@ -741,6 +741,17 @@ ORDER BY COALESCE(priority.value,'')='',
+@@ -152,7 +152,7 @@ ORDER BY COALESCE(t.id,0)=0,t.id""")
+         query = Query(self.env, order='id', group='milestone')
+         sql, args = query.get_sql()
+         self.assertEqualSQL(sql,
+-"""SELECT t.id AS id,t.summary AS summary,t.owner AS owner,t.type AS type,t.status AS status,t.priority AS priority,t.milestone AS milestone,t.time AS time,t.changetime AS changetime,priority.value AS priority_value
++"""SELECT t.id AS id,t.summary AS summary,t.owner AS owner,t.type AS type,t.status AS status,t.priority AS priority,t.time AS time,t.changetime AS changetime,t.milestone AS milestone,priority.value AS priority_value
+ FROM ticket AS t
+   LEFT OUTER JOIN enum AS priority ON (priority.type='priority' AND priority.name=priority)
+   LEFT OUTER JOIN milestone ON (milestone.name=milestone)
+@@ -164,7 +164,7 @@ ORDER BY COALESCE(t.milestone,'')='',COA
+         query = Query(self.env, order='id', group='milestone', groupdesc=1)
+         sql, args = query.get_sql()
+         self.assertEqualSQL(sql,
+-"""SELECT t.id AS id,t.summary AS summary,t.owner AS owner,t.type AS type,t.status AS status,t.priority AS priority,t.milestone AS milestone,t.time AS time,t.changetime AS changetime,priority.value AS priority_value
++"""SELECT t.id AS id,t.summary AS summary,t.owner AS owner,t.type AS type,t.status AS status,t.priority AS priority,t.time AS time,t.changetime AS changetime,t.milestone AS milestone,priority.value AS priority_value
+ FROM ticket AS t
+   LEFT OUTER JOIN enum AS priority ON (priority.type='priority' AND priority.name=priority)
+   LEFT OUTER JOIN milestone ON (milestone.name=milestone)
+@@ -502,7 +502,7 @@ ORDER BY COALESCE(t.id,0)=0,t.id""")
+         query = Query(self.env, group='summary')
+         sql, args = query.get_sql()
+         self.assertEqualSQL(sql,
+-"""SELECT t.id AS id,t.owner AS owner,t.type AS type,t.status AS status,t.priority AS priority,t.milestone AS milestone,t.summary AS summary,t.time AS time,t.changetime AS changetime,priority.value AS priority_value
++"""SELECT t.id AS id,t.owner AS owner,t.type AS type,t.status AS status,t.priority AS priority,t.milestone AS milestone,t.time AS time,t.changetime AS changetime,t.summary AS summary,priority.value AS priority_value
+ FROM ticket AS t
+   LEFT OUTER JOIN enum AS priority ON (priority.type='priority' AND priority.name=priority)
+ ORDER BY COALESCE(t.summary,'')='',t.summary,COALESCE(priority.value,'')='',CAST(priority.value AS integer),t.id""")
+@@ -525,7 +525,7 @@ ORDER BY COALESCE(priority.value,'')='',
+         query = Query(self.env, order='component', group='milestone')
+         sql, args = query.get_sql()
+         self.assertEqualSQL(sql,
+-"""SELECT t.id AS id,t.summary AS summary,t.owner AS owner,t.type AS type,t.status AS status,t.priority AS priority,t.component AS component,t.milestone AS milestone,t.time AS time,t.changetime AS changetime,priority.value AS priority_value
++"""SELECT t.id AS id,t.summary AS summary,t.owner AS owner,t.type AS type,t.status AS status,t.priority AS priority,t.component AS component,t.time AS time,t.changetime AS changetime,t.milestone AS milestone,priority.value AS priority_value
+ FROM ticket AS t
+   LEFT OUTER JOIN enum AS priority ON (priority.type='priority' AND priority.name=priority)
+   LEFT OUTER JOIN milestone ON (milestone.name=milestone)
+@@ -560,42 +560,48 @@ ORDER BY COALESCE(t.time,0)=0 DESC,t.tim
+     def test_default_group_invalid_field_1(self):
+         # invalid argument and no config
+         query = Query(self.env, group='foo', groupdesc=1)
+-        self.assertEqual(None, query.group)
+-        self.assertEqual(0, query.groupdesc)
++        self.assertEqual((None, None), query.get_group())
++        self.assertEqual(['priority'], query.order)
++        self.assertEqual([('priority', False)], query.get_order())
+ 
+     def test_default_group_invalid_field_2(self):
+         # valid argument and no config
+         query = Query(self.env, group='status')
+-        self.assertEqual('status', query.group)
+-        self.assertEqual(0, query.groupdesc)
++        self.assertEqual(('status', False), query.get_group())
++        self.assertEqual(['status', 'priority'], query.order)
++        self.assertEqual([('status', False), ('priority', False)], query.get_order())
+ 
+     def test_default_group_invalid_field_3(self):
+         # no argument and invalid config
+         self.env.config.set('query', 'default_group', '-foo')
+         query = Query(self.env)
+-        self.assertEqual(None, query.group)
+-        self.assertEqual(0, query.groupdesc)
++        self.assertEqual((None, None), query.get_group())
++        self.assertEqual(['priority'], query.order)
++        self.assertEqual([('priority', False)], query.get_order())
+ 
+     def test_default_group_invalid_field_4(self):
+         # invalid argument and invalid config
+         self.env.config.set('query', 'default_group', '-foo')
+         query = Query(self.env, group='bar')
+-        self.assertEqual(None, query.group)
+-        self.assertEqual(0, query.groupdesc)
++        self.assertEqual((None, None), query.get_group())
++        self.assertEqual(['priority'], query.order)
++        self.assertEqual([('priority', False)], query.get_order())
+ 
+     def test_default_group_invalid_field_5(self):
+         # invalid argument but valid config
+         self.env.config.set('query', 'default_group', '-milestone')
+         query = Query(self.env, group='bar')
+-        self.assertEqual('milestone', query.group)
+-        self.assertEqual(1, query.groupdesc)
++        self.assertEqual(('milestone', True), query.get_group())
++        self.assertEqual(['-milestone', 'priority'], query.order)
++        self.assertEqual([('milestone', True), ('priority', False)], query.get_order())
+ 
+     def test_default_group_invalid_field_6(self):
+         # valid argument and valid config
+         self.env.config.set('query', 'default_group', '-milestone')
+         query = Query(self.env, group='type')
+-        self.assertEqual('type', query.group)
+-        self.assertEqual(0, query.groupdesc)
++        self.assertEqual(('type', False), query.get_group())
++        self.assertEqual(['type', 'priority'], query.order)
++        self.assertEqual([('type', False), ('priority', False)], query.get_order())
+ 
+     def test_default_order_no_config_with_request(self):
+         query = Query(self.env, order='summary')
+@@ -658,42 +664,42 @@ ORDER BY COALESCE(t.time,0)=0 DESC,t.tim
+     def test_default_order_invalid_field_1(self):
+         # invalid argument and no config
+         query = Query(self.env, order='foo', desc=1)
+-        self.assertEqual('priority', query.order)
+-        self.assertEqual(0, query.desc)
++        self.assertEqual(['priority'], query.order)
++        self.assertEqual([('priority', False)], query.get_order())
+ 
+     def test_default_order_invalid_field_2(self):
+         # valid argument and no config
+         query = Query(self.env, order='status')
+-        self.assertEqual('status', query.order)
+-        self.assertEqual(0, query.desc)
++        self.assertEqual(['status'], query.order)
++        self.assertEqual([('status', False)], query.get_order())
+ 
+     def test_default_order_invalid_field_3(self):
+         # no argument and invalid config
+         self.env.config.set('query', 'default_order', '-foo')
+         query = Query(self.env)
+-        self.assertEqual('priority', query.order)
+-        self.assertEqual(0, query.desc)
++        self.assertEqual(['priority'], query.order)
++        self.assertEqual([('priority', False)], query.get_order())
+ 
+     def test_default_order_invalid_field_4(self):
+         # invalid argument and invalid config
+         self.env.config.set('query', 'default_order', '-foo')
+         query = Query(self.env, order='bar')
+-        self.assertEqual('priority', query.order)
+-        self.assertEqual(0, query.desc)
++        self.assertEqual(['priority'], query.order)
++        self.assertEqual([('priority', False)], query.get_order())
+ 
+     def test_default_order_invalid_field_5(self):
+         # invalid argument but valid config
+         self.env.config.set('query', 'default_order', '-milestone')
+         query = Query(self.env, order='bar')
+-        self.assertEqual('milestone', query.order)
+-        self.assertEqual(1, query.desc)
++        self.assertEqual(['-milestone'], query.order)
++        self.assertEqual([('milestone', True)], query.get_order())
+ 
+     def test_default_order_invalid_field_6(self):
+         # valid argument and valid config
+         self.env.config.set('query', 'default_order', '-milestone')
+         query = Query(self.env, order='type')
+-        self.assertEqual('type', query.order)
+-        self.assertEqual(0, query.desc)
++        self.assertEqual(['type'], query.order)
++        self.assertEqual([('type', False)], query.get_order())
+ 
+     def test_default_cols_with_config_no_request(self):
+         self.env.config.set('query', 'default_columns', 'id,time,changetime,summary,milestone,type')
+@@ -741,6 +747,17 @@ ORDER BY COALESCE(priority.value,'')='',
          self.assertEqual([], args)
          tickets = query.execute(self.req)
  
 +    def test_get_group(self):
 +        query = Query(self.env, group='-milestone', order=['-priority', 'id'])
 +        self.assertEqual(True, query.group)
-+        self.assertEqual('milestone', query.get_group())
++        self.assertEqual(('milestone', True), query.get_group())
 +
      def test_csv_escape(self):
          query = Mock(get_columns=lambda: ['col1'],