TicketQuery progress display link ignores order= keys in milestone_groups query_args causing query error

[ilewismsl]

Using the macro invocation

[[TicketQuery(owner=ian, format=progress)]]

in a wiki page creates a progress bar with links for various classes of ticket.

The resulting progress bar creates query URLs that work for all classes of ticket except for closed tickets. For closed tickets it produces a link that looks like:

/trac/msl/query?owner=ian&status=closed&group=resolution&order=time&col=id&col=summary&col=owner&col=type&col=priority&col=component&col=severity&col=time&col=changetime&max=0&order=id

This URL includes two order= keys: order=time near the begining and order=id at the end.

This confuses Trac and results in a runtime error.

All other classes of ticket, such as accepted tickets produce a link that looks like:

/trac/msl/query?owner=ian&status=accepted&max=0&order=id

with a single order=id statement.

The source of this error is that our milestone groups includes an order=time key.

We got this by direct copying from the sample at ​TracIni without much thought about what it meant.

[milestone-groups]
...
# optional extra param for the query (two additional columns: created and modified and sort on created)
closed.query_args = group=resolution,order=time,col=id,col=summary,col=owner,col=type,col=priority,col=component,col=severity,col=time,col=changetime
...

Error message

Trac detected an internal error:
TypeError: unhashable type: 'list'

Python traceback

Most recent call last: 
File "build/bdist.win32/egg/trac/web/main.py", line 497, in _dispatch_request
  dispatcher.dispatch(req)
File "build/bdist.win32/egg/trac/web/main.py", line 214, in dispatch
  resp = chosen_handler.process_request(req)
File "build/bdist.win32/egg/trac/ticket/query.py", line 939, in process_request
  max)
File "build/bdist.win32/egg/trac/ticket/query.py", line 77, in __init__
  self.order = synonyms.get(order, order)     # 0.11 compatibility

System information

User Agent: Mozilla/5.0 (compatible; MSIE 9.0; Windows NT 6.1; WOW64; Trident/5.0; SLCC2; .NET CLR 2.0.50727; .NET CLR 3.5.30729; .NET CLR 3.0.30729; Media Center PC 6.0; .NET4.0C; .NET4.0E)

Trac	1.0
Docutils	0.8.1
Genshi	0.6 (without speedups)
mod_python	3.3.1
Pygments	1.4
pysqlite	2.4.1
Python	2.6.6 (r266:84297, Aug 24 2010, 18:46:32) [MSC v.1500 32 bit (Intel)]
setuptools	0.6c11
SQLite	3.5.9
Subversion	1.7.1 (r1186859)
jQuery	1.7.2

[Christian Boos]

In fact this happens as soon as order is specified multiple times, as #10907 demonstrates.

So we have to be robust against this condition (and even do something with that, one day → #10178), and also find out why the progress type generates multiple values for order.

[Peter Suter]

Replying to cboos:

and also find out why the progress type generates multiple values for order.

I guess the problem is here​, where the [milestone-groups] .query_args alias qry_args are appended to the other constraints.

(I think I was trying to follow roadmap.milestone_stats_data.query_href​, but there the "other constraints" can not include order.)

[Peter Suter]

In this patch I try to let Query.from_string deal with these additional arguments.

Adding some unit tests might be a good idea.

[Peter Suter]

Attempt at unit test

[Peter Suter]

I tried writing a test for this, but have some questions:

• Should this go in trac/ticket/tests/query.py (already has TicketQueryMacroTestCase, but that does not really expand the macro) or trac/ticket/tests/wikisyntax.py (already has QUERY_TEST_CASES and QUERY2_TEST_CASES that do expand the macro) or somewhere else?
• req.chrome and req.session are required for this. (e.g. in render_template) These were apparently not required for any other such test. Is this a flaw in this macro or in the test setup? Does simply adding empty dictionaries for these make sense or should I do something else? Should I only add them for this one test case?
• I copied the ticket_setup / ticket_teardown to tweak [milestone-groups]. Should this instead be merged into the old testcase (possibly changing the existing reference output)?

[Peter Suter]

#11065 was closed as a duplicate.

[Peter Suter]

req.chrome and req.session are required for [testing] this.

I now see​ that​ many​ tests​ do​ this​ in the Mock() constructor.

The suggested fix in the attached patch has problems if &or& is used.

[Peter Suter]

attachment:T10838-TicketQueryProgressOrder-2.patch​ solves the &or& issue and includes more tests with the Mock(chrome={}, session={}) change.

[Peter Suter]

Committed in [12615] and merged in [12616].

[Jun Omae]

After [12616], I got 3 failures in unit tests on trunk with Python 2.5-2.7. See py25-sqlite.log.gz​.

[Peter Suter]

Reproduced (after installing pygments).

[Peter Suter]

Looks like adding chrome={}, session={} in changeset:12615#file2 triggers this.

Before, PygmentsRenderer.render() failed​ with a (silenced​) 'Mock' object has no attribute 'session' AttributeError.

Any suggestions how to proceed?

[Peter Suter]

Would this be acceptable?

trac/wiki/tests/formatter.py

@@ -161 +161 @@
                 HelloWorldMacro, DivHelloWorldMacro, TableHelloWorldMacro,
                 DivCodeMacro, DivCodeElementMacro, DivCodeStreamMacro,
                 NoneMacro, WikiProcessorSampleMacro, SampleResolver]
-        self.env = EnvironmentStub(enable=['trac.*'] + all_test_components)
+        self.env = EnvironmentStub(enable=['trac.*'] + all_test_components,
+                                   disable=['trac.mimeview.pygments'])
         # -- macros support
         self.env.path = ''
         # -- intertrac support

[Jun Omae]

It seems root cause is introduced in r12184. We should use #!default rather than #!python to get same result with/without pygments, see log:jomae.git@t10834.4.

[Peter Suter]

Sounds like a good idea.

see log:jomae.git@t10834.4.

1.1.2dev: follow-ups to r10834, … (#10834)

I think you used the ticket number for the revision number by mistake.

[Jun Omae]

see log:jomae.git@t10834.4.

1.1.2dev: follow-ups to r10834, … (#10834)

I think you used the ticket number for the revision number by mistake.

Oh, thanks for point.

Also, I think we should add unit tests for pygments highlightning with lineno annotator as the original intention, see [3fb9ad6f/jomae.git] and [9c525901/jomae.git].

[Jun Omae]

Committed in [12619]. Thanks for investigating!