Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 8 commits into
base: dev
Choose a base branch
from

Conversation

toy
Copy link
Contributor

@toy toy commented Sep 23, 2024

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

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

@toy toy requested a review from ulferts September 23, 2024 11:14
@toy toy force-pushed the bug/57550-custom-field-with-format-version-are-ordered-as-strings branch from ff62ae7 to b4767cb Compare September 24, 2024 13:29
@toy toy changed the base branch from dev to bug/57577-broken-ordering-by-multi-value-custom-fields September 24, 2024 13:30
@toy toy force-pushed the bug/57577-broken-ordering-by-multi-value-custom-fields branch from efe8cf7 to c768f8b Compare September 26, 2024 13:00
Base automatically changed from bug/57577-broken-ordering-by-multi-value-custom-fields to dev September 26, 2024 13:25
@toy toy force-pushed the bug/57550-custom-field-with-format-version-are-ordered-as-strings branch from b4767cb to 3bd5e06 Compare September 26, 2024 13:53
Copy link
Contributor

@ulferts ulferts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice change here @toy. And good find in the first place of using the collation.

The reason why I did not merge this now is that I have a recommendation I would like to talk through with you.

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.

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.

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.

@toy
Copy link
Contributor Author

toy commented Oct 1, 2024

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

@ulferts
Copy link
Contributor

ulferts commented Oct 8, 2024

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.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants