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/57577 broken ordering by multi value custom fields #16762

Merged
merged 16 commits into from
Sep 26, 2024

Conversation

toy
Copy link
Contributor

@toy toy commented Sep 19, 2024

Ticket

OP#57577

What are you trying to accomplish?

Fix inconsistencies of ordering by multi value custom fields (list, user, version). Preparing for OP#57305

Merge checklist

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

@toy toy marked this pull request as draft September 19, 2024 18:14
@toy toy force-pushed the bug/57577-broken-ordering-by-multi-value-custom-fields branch from 8b910da to 1051935 Compare September 20, 2024 13:36
@toy toy marked this pull request as ready for review September 20, 2024 13:53
@toy toy requested a review from ulferts September 23, 2024 11:14
@toy toy force-pushed the bug/57577-broken-ordering-by-multi-value-custom-fields branch from 1051935 to efe8cf7 Compare September 24, 2024 13:04
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.

Apart from the one tiny naming issue, the changes are good. So merge at will once that is resolved.

The structure of the commits really helped me work through the PR. Thanks for that.

It appears to me that at least part of the performance problems related to sorting by multi value user custom fields are already resolved by this PR. At least on the data I have available, the performance was no longer abhorrent.

You were right about the need to have the selected values for multi fields ordered in the same way as is now used when ordering work packages and projects. That can be a different topic so please open a ticket for this.

project_with_cf_value(*id_by_value.fetch_values("3")), # 3
project_with_cf_value(*id_by_value.fetch_values("3", "20")), # 3, 20
project_with_cf_value(*id_by_value.fetch_values("20")), # 20
project_without_cf_value # TODO: decide on order of absent values
]
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 for adding the additional cases where the same value is permutated.

@toy
Copy link
Contributor Author

toy commented Sep 26, 2024

The structure of the commits really helped me work through the PR. Thanks for that.

❤️

It appears to me that at least part of the performance problems related to sorting by multi value user custom fields are already resolved by this PR. At least on the data I have available, the performance was no longer abhorrent.

Yes it should be ~3 times faster after this PR

You were right about the need to have the selected values for multi fields ordered in the same way as is now used when ordering work packages and projects. That can be a different topic so please open a ticket for this.

👍

@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
@toy toy merged commit 1650a29 into dev Sep 26, 2024
11 checks passed
@toy toy deleted the bug/57577-broken-ordering-by-multi-value-custom-fields branch September 26, 2024 13:25
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