Roundup Tracker - Issues

Issue 2551122

classification
sorted method of MultilinkHTMLProperty does a string sort even if the property is an integer
Type: behavior Severity: major
Components: Web interface Versions: 2.0.0
process
Status: fixed fixed
:
: rouilj : ngaba, rouilj, schlatterbeck
Priority: high : NeedsTesting, patch

Created on 2021-03-27 03:59 by rouilj, last changed 2021-05-23 22:12 by rouilj.

Files
File name Uploaded Description Edit Remove
UseOrderprop.patch rouilj, 2021-05-22 05:12
classic_overlay.tgz rouilj, 2021-05-22 19:54 Tarball of three files needed to reproduce issue.
Messages
msg7142 Author: [hidden] (rouilj) Date: 2021-03-27 03:59
Details and discussion in thread including:

  https://sourceforge.net/p/roundup/mailman/message/37247607/

short form:

sorted method uses:

  value.sort(key=lambda a:a[property], reverse=reverse)

but a[property] is always a string so you get 1, 12, 2, 21, 22 etc.

Need to covert a[property] into it's actual declared type.
Hyperdb has a sort_repr(cls, a[property], property) method
for just this purpose. However there is no method defined for
Integer or Number. Define one for Integer and Number returning
int(val) or float(val) respectively. Also need to handle case
where value (a[property] is None) (see issue2551120) as this
causes a crash.

Extensive discussion on how to handle None state and set it to some
value that will allow the sort to work is inconclusive so far.
I suggest using int(sys.maxint || sys.maxsize) (python2/3)
and float('inf'). This should sort unknown to the end.

Once sort_repr is done for Integer and Float something like:

   cls = self._db.getclass(self._prop.classname)
   sort_repr = cls.property.sort_repr # (or maybe cls[property])
   value.sort(key=lambda a:sort_repr(cls, a[property], property),
              reverse=reverse)

but I am not sure what self is passed to sort_repr.

Problem found by ngaba.
msg7235 Author: [hidden] (rouilj) Date: 2021-05-19 00:50
Hi Nagy:

I have struck out on trying to reproduce this in my tracker.  So I am
starting from scratch (simpler tracker) to reproduce the problem.

My plan is to start with the classic tracker. Change the schema for
the status class removing the order property and adding:

   ord = Integer()

so this is the integer property I will use for ordering. Use
setorderprop('ord') for the status class to get that field to be used
for ordering.

The 8 statuses in the classic tracker will have the same value for ord
as they used to have for order. Except item 5 in-progress will have an
ord value of 15.

If I sort a list of issues in a multilink by the status using
sorted(), I should see in-progress(ord=15) between items with ord
values of 1 and 2.

Then I will change the issue.item template to display items in the
superseder multilink using:

        <a tal:repeat="sup context.superseder.sorted('ord')"
        tal:content="python:sup['id'] + '-' + sup['status'] +
               ','*(not repeat['sup'].end)"
        tal:attributes="href string:issue${sup/id}"></a>

Then I will create an issue with 8 superseder issues, one in each
state.  I should see the superseder issues mis-sorted with
in-progress(ord=15) after unread(ord=1) and before deferred(ord=2)
with the mis-sorted item.

Does this sound like it will reproduce the problem you are describing?
msg7237 Author: [hidden] (ngaba) Date: 2021-05-20 16:20
Hi John:

Yes, I think your steps will reproduce the issue.
msg7240 Author: [hidden] (rouilj) Date: 2021-05-22 05:12
Hi Nagy:

I think this patch fixes it. Can you apply it over the previous patch
for None value causing traceback and let me know if it fixes it.
msg7241 Author: [hidden] (rouilj) Date: 2021-05-22 19:54
Untar classic_overlay.tgz to get the changes (schema.py,
issue.item.html, initial_data.py) from classic template I
used to reproduce this.
msg7244 Author: [hidden] (ngaba) Date: 2021-05-23 19:04
Dear John,

Thank you for the patch.
It seems it fixed the bug described in msg7235 here (using my own different schema.py, ofc).
msg7245 Author: [hidden] (rouilj) Date: 2021-05-23 22:12
Committed on:

changeset:   6421:9c57f2814597

Marked needs testing. Same issue with testing as issue2551120.
The testing framework for the templating functions does not
have a complete db. The mock that exists to test templating isn't 
sufficient to test this code.
History
Date User Action Args
2021-05-23 22:12:53rouiljsetkeywords: + NeedsTesting, - Blocker
status: pending -> fixed
resolution: fixed
messages: + msg7245
2021-05-23 19:04:45ngabasetmessages: + msg7244
2021-05-22 19:55:47rouiljsetstatus: open -> pending
2021-05-22 19:54:46rouiljsetfiles: + classic_overlay.tgz
messages: + msg7241
2021-05-22 05:12:55rouiljsetfiles: + UseOrderprop.patch
keywords: + patch
messages: + msg7240
2021-05-20 16:20:10ngabasetmessages: + msg7237
2021-05-19 00:50:02rouiljsetstatus: new -> open
messages: + msg7235
2021-05-06 19:19:33rouiljsetkeywords: + Blocker
2021-03-27 03:59:23rouiljcreate