-
Notifications
You must be signed in to change notification settings - Fork 109
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
Promotions should be sorted lexicographically #1930
Comments
Is this done by parsing the promotion name "shortName.ulid.shorthash" (maybe making a helper function on the Promotion struct for that) and sorting on that instead of the creation time? |
It would not require parsing, the ULID component in the name would make any list entry sortable by default. |
To clarify: as far as I am aware this is a frontend issue which would not require any changes to the backend. |
See also:
|
Ah I see I didn't realize we are always dealing with the same prefix (stage name) so we can indeed just sort on the Name. Are you sure we should be sorting this in the frontend and not in the api though. I know the frontend is also sorting it, but shouldn't we aim to sort it in the backend in general is more the quest |
Maybe doing so doesn't solve the issue you originally raised, as you still have some more "advanced" sorting in the frontend with running promotions versus non running. But it would I believe still make sense to change this in the API as well, to ensure we are returning properly sorted results there as well. |
Agree it's ideally a back end change. Re-labeling accordingly. |
Ill work on this. |
Thank you! |
The
UIAPI server appears to order Promotions for a Stage by their Phase and creation timestamp. However, as the creation timestamp does not include nanoseconds, this approach will fail when multiple Promotions are (programmatically) created within a short period.To address this issue, it should use the ULID in the Promotion name, which allows it to be lexicographically sorted beyond the creation timestamp of the Promotion.
The text was updated successfully, but these errors were encountered: