Edgewall Software
Modify

Opened 8 years ago

Closed 6 years ago

Last modified 6 years ago

#10838 closed defect (fixed)

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

Reported by: ilewismsl Owned by: Peter Suter
Priority: normal Milestone: 1.0.2
Component: ticket system Version: 1.0
Severity: minor Keywords: TicketQuery progress
Cc: Branch:
Release Notes:

Fixed error in TicketQuery(format=progress) macro when e.g. order keys appear in query_args of milestone_groups.

API Changes:
Internal Changes:

Description

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

Attachments (4)

T10838-TicketQueryProgressOrder.patch (1.6 KB ) - added by Peter Suter 8 years ago.
T10838-TicketQueryProgressOrder-UnitTest.patch (4.0 KB ) - added by Peter Suter 8 years ago.
Attempt at unit test
T10838-TicketQueryProgressOrder-2.patch (8.5 KB ) - added by Peter Suter 6 years ago.
py25-sqlite.log.gz (3.8 KB ) - added by Jun Omae 6 years ago.

Download all attachments as: .zip

Change History (21)

comment:1 by Christian Boos, 8 years ago

Milestone: next-stable-1.0.x

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.

in reply to:  1 comment:2 by Peter Suter, 8 years ago

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.)

by Peter Suter, 8 years ago

comment:3 by Peter Suter, 8 years ago

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

Adding some unit tests might be a good idea.

by Peter Suter, 8 years ago

Attempt at unit test

comment:4 by Peter Suter, 8 years ago

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

comment:5 by Peter Suter, 7 years ago

Owner: set to Peter Suter
Status: newassigned

#11065 was closed as a duplicate.

comment:6 by Peter Suter, 6 years ago

Milestone: next-stable-1.0.x1.0.2

comment:7 by Peter Suter, 6 years ago

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.

Last edited 6 years ago by Peter Suter (previous) (diff)

by Peter Suter, 6 years ago

comment:8 by Peter Suter, 6 years ago

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

comment:9 by Peter Suter, 6 years ago

Release Notes: modified (diff)
Resolution: fixed
Status: assignedclosed

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

by Jun Omae, 6 years ago

Attachment: py25-sqlite.log.gz added

comment:10 by Jun Omae, 6 years ago

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

comment:11 by Peter Suter, 6 years ago

Reproduced (after installing pygments).

comment:12 by Peter Suter, 6 years ago

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?

Last edited 6 years ago by Peter Suter (previous) (diff)

comment:13 by Peter Suter, 6 years ago

Would this be acceptable?

  • trac/wiki/tests/formatter.py

    diff -r b760ea033dbb trac/wiki/tests/formatter.py
    a b  
    161161                HelloWorldMacro, DivHelloWorldMacro, TableHelloWorldMacro,
    162162                DivCodeMacro, DivCodeElementMacro, DivCodeStreamMacro,
    163163                NoneMacro, WikiProcessorSampleMacro, SampleResolver]
    164         self.env = EnvironmentStub(enable=['trac.*'] + all_test_components)
     164        self.env = EnvironmentStub(enable=['trac.*'] + all_test_components,
     165                                   disable=['trac.mimeview.pygments'])
    165166        # -- macros support
    166167        self.env.path = ''
    167168        # -- intertrac support

comment:14 by Jun Omae, 6 years ago

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.

in reply to:  14 ; comment:15 by Peter Suter, 6 years ago

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.

in reply to:  15 comment:16 by Jun Omae, 6 years ago

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].

Last edited 6 years ago by Jun Omae (previous) (diff)

comment:17 by Jun Omae, 6 years ago

Committed in [12619]. Thanks for investigating!

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Peter Suter.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from Peter Suter to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.