openproject icon indicating copy to clipboard operation
openproject copied to clipboard

Bug/57550 custom field with format version are ordered as strings

Open toy opened this issue 1 year ago • 3 comments

Ticket

OP#57550

What are you trying to accomplish?

Order versions as versions instead of as strings

What approach did you choose and why?

Using collation that treats runs of digits as one number rather than as separate digits.

Questionable:

  1. Creating collation with IF NOT EXISTS to reduce small chance of conflict. Also not dropping it in down direction
  2. I'm not sure if keeping order_by_semver_name is meaningful
  3. It is also now using order instead of reorder, no complaints from tests about that

Merge checklist

  • [x] Added/updated tests
  • [ ] Added/updated documentation in Lookbook (patterns, previews, etc)
  • [ ] Tested major browsers (Chrome, Firefox, Edge, ...)

toy avatar Sep 23 '24 11:09 toy

Currently there is a discrepancy between ordering a work package list by a version custom field as opposed to ordering it by the hard wired version property. For the later, the null values are always placed last. I propose to remove this in the same PR, or at least to do it right after. It would only require to remove the null_handling statement defined for version in property_select.rb.

Related discussion about ordering of absent values for custom fields. What about start date and due date columns? The PR #8000 and OP#32156 don't describe the reason for adding null_handling that puts empty values always at the end for 3 properties.

The other thing I would include in this PR is the cleanup of the Version.sort_by_name scope which I believe has no more merit.

You mean Version.order_by_name (or Version.order_by_semver_name)?

Just saw that Version.order_by_name orders by lowered name which according to few tests preserves collation, so still orders versions not as plain strings. We can change the collation to also be not case sensitive, but it will make it non deterministic

Lastly, it is a bit weird that there is now a name and a semver_name order available in the API which basically do the same but I would still keep the two to not break potential integrations.

I'm wondering how does Queries::Versions::Orders::NameOrder work, if it overrides private order to receive no arguments while in base it receives one, so apply_to will raise

toy avatar Oct 01 '24 17:10 toy

Related discussion about ordering of absent values for custom fields. What about start date and due date columns? The PR https://github.com/opf/openproject/pull/8000 and OP#32156 don't describe the reason for adding null_handling that puts empty values always at the end for 3 properties.

I'd just remove it for versions so that it the same whenever one orders by version name, regardless of whether that is done on a custom field or on the hard wired property. But it is also not super important so if you want it done in the scope of the null value order feature that would also be fine with me. And apparently there is also the same discrepancy between date custom fields and due and start date so I definitely see the benefit in having all of them harmonized.

You mean Version.order_by_name (or Version.order_by_semver_name)?

Yes, I mean the order_by_name method. Sorry for the confusion.

Just saw that Version.order_by_name orders by lowered name which according to few tests preserves collation, so still orders versions not as plain strings. We can change the collation to also be not case sensitive, but it will make it non deterministic

At least in my test, the change in the collation already seems to do that even without the transformation to LOWER. I did not check why:

image

I'm wondering how does Queries::Versions::Orders::NameOrder work, if it overrides private order to receive no arguments while in base it receives one, so apply_to will raise

It fails of course :). Apparently it is just broken. Could be fixed in a separate PR but since the change of simply removing the method override is simple, I'd include it.

ulferts avatar Oct 08 '24 09:10 ulferts

At least in my test, the change in the collation already seems to do that even without the transformation to LOWER. I did not check why:

Just to share here the learning that it is caused by using ICU collation: it is not ignoring the case, but ICU collation is more sophisticated than just comparing character codes while applying some language rules - it compares characters on multiple levels and case difference is less important than case-ignoring difference. So following is the correct collation: a 0, A 0, à 0, a 1, A 1, à 1

toy avatar Oct 18 '24 11:10 toy