Edgewall Software
Modify

Opened 2 years ago

Closed 16 months ago

Last modified 16 months ago

#13474 closed defect (fixed)

TicketQuery with format=progress causes red box

Reported by: anonymous Owned by: Clemens Mosig <clemens.mosig@…>
Priority: normal Milestone: 1.5.4
Component: rendering Version: 1.5.3
Severity: normal Keywords:
Cc: clemens.mosig@… Branch:
Release Notes:

Fixed TypeError is raised from progress_bar.html when all intervals are 0%.

API Changes:
Internal Changes:

Description

I am testing th:ChildTicketsPlugin, which suggests the following use of TicketQuery:

[[TicketQuery(parent=#1,format=progress)]]

However, when the query is empty, this causes an error:

Error: Macro TicketQuery(parent=#1,format=progress) failed
'>' not supported between instances of 'NoneType' and 'int'

When I remove format=progress then the result simply displays No results (not as an error box).

See below for the log information generated.

2022-04-05 17:03:15,726 Trac[formatter] ERROR: Macro TicketQuery(parent=#1,format=progress) failed for <Resource 'ticket:2503'>:
Traceback (most recent call last):
  File "/usr/local/lib/python3.10/dist-packages/trac/wiki/formatter.py", line 813, in _macro_formatter
    return macro.ensure_inline(macro.process(args), in_paragraph)
  File "/usr/local/lib/python3.10/dist-packages/trac/wiki/formatter.py", line 380, in process
    text = self.processor(text)
  File "/usr/local/lib/python3.10/dist-packages/trac/wiki/formatter.py", line 351, in _macro_processor
    return self.macro_provider.expand_macro(self.formatter, self.name,
  File "/usr/local/lib/python3.10/dist-packages/trac/ticket/query.py", line 1444, in expand_macro
    chrome.render_fragment(req, 'progress_bar.html', data),
  File "/usr/local/lib/python3.10/dist-packages/trac/web/chrome.py", line 1459, in render_fragment
    return self.render_template_string(template, data, text)
  File "/usr/local/lib/python3.10/dist-packages/trac/web/chrome.py", line 1550, in render_template_string
    string = template.render(data)
  File "/usr/lib/python3/dist-packages/jinja2/environment.py", line 1291, in render
    self.environment.handle_exception()
  File "/usr/lib/python3/dist-packages/jinja2/environment.py", line 925, in handle_exception
    raise rewrite_traceback_stack(source=source)
  File "/usr/local/lib/python3.10/dist-packages/trac/templates/progress_bar.html", line 32, in top-level template code
    }|htmlattr}>
  File "/usr/local/lib/python3.10/dist-packages/trac/util/presentation.py", line 172, in is_greaterthan
    return a > b
TypeError: '>' not supported between instances of 'NoneType' and 'int'

Attachments (3)

initial_error.png (14.9 KB ) - added by Clemens Mosig 2 years ago.
fix_1.png (6.3 KB ) - added by Clemens Mosig 2 years ago.
progress-with-no-matched-tickets.png (25.7 KB ) - added by Jun Omae 16 months ago.

Download all attachments as: .zip

Change History (18)

comment:1 by Jun Omae, 2 years ago

Component: web frontendrendering
Milestone: 1.5.4

comment:2 by Ryan J Ollos, 2 years ago

Milestone: 1.5.41.5.5

comment:3 by Clemens Mosig, 2 years ago

I will have a look at this one.

by Clemens Mosig, 2 years ago

Attachment: initial_error.png added

by Clemens Mosig, 2 years ago

Attachment: fix_1.png added

comment:4 by anonymous, 2 years ago

One way to fix this is replacing the default percentage of the intervals that are used to display the progress bars with 0 (was None). See this commit.

The initial error looked as follows

The issue is that templates/progress_bar.html is unable to handle None values for the percentage of intervals.

Now, with the implemented fix, a progress bar for a query that yields no tickets looks as follows:

It is definitely better than a python error message but it does not look especially pretty. One could try to display an empty progress bar here instead?

I would not opt for a "No Results" message because when using format=table we get an empty table and not "No Results". An empty progress bar would be more consistent.

Open to suggestions! What is the expected behavior? Or would handling None values in templates/progress_bar.html be more preferable?

in reply to:  4 comment:5 by Clemens Mosig, 2 years ago

Replying to anonymous:

Forgot to but my name in there… —> Clemens

comment:6 by Jun Omae, 2 years ago

Root cause is that behavior of > operator with None and an integer differs between Python 2 and 3.

Python 2.7.18 (default, Mar  8 2021, 13:02:45)
[GCC 9.3.0] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> None > 42
False
>>>
Python 3.9.12 (main, Apr 18 2022, 22:40:46)
[GCC 9.4.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> None > 42
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: '>' not supported between instances of 'NoneType' and 'int'

We could fix templates (greaterthan(x)x is none and greaterthan(x)), however functions which have the issue in trac.util.presentation are used in many templates. Also, we cannot fix plugins which used the functions in templates.

I consider that we should allow to pass None in the functions.

$ git grep '\<\(greaterthan\|greaterthanorequal\|lessthan\|lessthanorequal\)\>' -- '*.html'
trac/templates/attachment.html:          <label>File{% if max_size is greaterthanorequal 0 %}
trac/templates/diff_div.html:                clines[loop.index0] if loop.index0 is lessthan(len(clines))
trac/templates/diff_div.html:            #           set within_change = loop.index0 is lessthan(len(block.changed.lines))
trac/templates/diff_div.html:            #           set within_change = loop.index0 is lessthan(len(block.base.lines))
trac/templates/diff_div.html:        #     if loop.index is lessthan(len(item.diffs)):
trac/templates/diff_options.html:         value="${'all' if diff.options.contextlines is lessthan(0) else
trac/templates/diff_view.html:      # set multi = num_changes is greaterthan(1)
trac/templates/history_view.html:        # if len(history) is greaterthan(10):
trac/templates/progress_bar.html:                        interval.percent is greaterthan 0
trac/ticket/templates/ticket.html:                      'trac-new' if change_date is greaterthan(start_time) and
trac/ticket/templates/ticket_change.html:#   if not show_editor and len(change.comment_history) is greaterthan(1):
trac/ticket/templates/ticket_change.html:  #   if comment_version is greaterthan(0):
trac/ticket/templates/ticket_change.html:  #   if comment_version is lessthan(max_version):
trac/ticket/templates/ticket_change.html:  #   if comment_version is greaterthan(0):
trac/ticket/templates/ticket_preview.html:           'trac-new' if change_date is greaterthan(start_time) and
trac/timeline/templates/timeline.html:        #   set highlight = precision and precisedate and timedelta(0) is lessthanorequal(event_delta) and event_delta is lessthan(precision)
trac/versioncontrol/templates/changeset_content.html:  #   elif ndiffs + nprops is greaterthan(0):
trac/versioncontrol/templates/changeset_content.html:      #   if delta is lessthan(timedelta(0, 3600)):
trac/versioncontrol/templates/changeset_content.html:    #   if max_diff_bytes and diff_bytes is greaterthan(max_diff_bytes):
trac/versioncontrol/templates/changeset_content.html:    #   if max_diff_files and diff_files is greaterthan(max_diff_files):
trac/versioncontrol/templates/revisionlog.html:        # if len(items) is greaterthan(10) and len(item_ranges) == 1:
trac/wiki/templates/wiki_diff.html:        # if (new_version - old_version) is greaterthan(1):

comment:7 by Jun Omae, 2 years ago

Proposed fix (unit tests should be added):

  • trac/util/presentation.py

    diff --git a/trac/util/presentation.py b/trac/util/presentation.py
    index cf915ad1c..91495ec8c 100644
    a b def is_not_equalto(a, b):  
    174174    return a != b
    175175
    176176def is_greaterthan(a, b):
     177    if b is None:
     178        raise ValueError('Must not be `None`')
     179    if a is None:
     180        return False
    177181    return a > b
    178182
    179183def is_greaterthanorequal(a, b):
     184    if b is None:
     185        raise ValueError('Must not be `None`')
     186    if a is None:
     187        return False
    180188    return a >= b
    181189
    182190def is_lessthan(a, b):
     191    if b is None:
     192        raise ValueError('Must not be `None`')
     193    if a is None:
     194        return True
    183195    return a < b
    184196
    185197def is_lessthanorequal(a, b):
     198    if b is None:
     199        raise ValueError('Must not be `None`')
     200    if a is None:
     201        return True
    186202    return a <= b
    187203
    188204def is_in(a, b):

in reply to:  6 comment:8 by anonymous, 2 years ago

Root cause is that behavior of > operator with None and an integer differs between Python 2 and 3. We could fix templates (greaterthan(x)x is none and greaterthan(x)), however functions which have the issue in trac.util.presentation are used in many templates. Also, we cannot fix plugins which used the functions in templates.

I consider that we should allow to pass None in the functions.

Hmm, I see.

Why would we allow a to be None, but not b? In python 2 both x < None (returns False) and None < x (returns True) works. Wouldn't we want to replicate python 2 behavior completely if we allow None for one of the arguments?

comment:9 by Clemens Mosig <clemens.mosig@…>, 2 years ago

Cc: clemens.mosig@… added
Owner: set to Clemens Mosig <clemens.mosig@…>
Status: newassigned

comment:10 by trac@…, 16 months ago

I'm going to try using Jun's patch. I have a use case for this feature now.

I understand there might be a more compatible change, but if this works for the progress bar use case, then it's fine with me. Everything can always be made better, but we have to start somewhere.

Thanks!

comment:11 by Jun Omae, 16 months ago

Owner: changed from Clemens Mosig <clemens.mosig@…> to Jun Omae

Patch from the reporter solves the crash, however progress of the macro disappears. I'm going to fix it and add unit tests.

Last edited 16 months ago by Jun Omae (previous) (diff)

comment:12 by Jun Omae, 16 months ago

Ah, the same has been pointed out from the reporter at comment:4….

in reply to:  4 comment:13 by Jun Omae, 16 months ago

Replying to anonymous:

It is definitely better than a python error message but it does not look especially pretty. One could try to display an empty progress bar here instead?

I've filed #13564 as a new ticket because Trac 1.4.x has also the issue.

comment:14 by Jun Omae, 16 months ago

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

Committed the original patch with unit tests in [17675]. Reconsidering it, I decided not to modify methods in presentation.py.

comment:15 by Jun Omae, 16 months ago

Owner: changed from Jun Omae to Clemens Mosig <clemens.mosig@…>

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Clemens Mosig <clemens.mosig@…>.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from Clemens Mosig <clemens.mosig@…> 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.