-
-
Notifications
You must be signed in to change notification settings - Fork 284
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
MBS-9253: List EP release groups above singles #3387
base: master
Are you sure you want to change the base?
Conversation
This has been requested forever, and it makes sense in all markets except maybe Japan. Given Japan would want singles before albums, which we don't do anyway, this is probably not too relevant.
Holy sh*t :-0 |
(CASE | ||
WHEN rg.type = 3 THEN 2 -- Sort EPs above singles | ||
WHEN rg.type = 2 THEN 3 -- Sort singles below EPs | ||
ELSE rg.type | ||
END | ||
)::SMALLINT, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with your solution here as it's pretty straightforward, though I wondered if we should be using release_group_primary_type.child_order
for this at first. Unless we want the general order of the types in a list to differ from the discography display order? (It would also require a new trigger on release_group_primary_type
for when the child_order
changes.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could use that I guess, yes - it never even crossed my mind since I never think about it at all... Wouldn't that be more annoying though since it'd require new triggers and a join here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it'd also require one more join. The advantage would be that we could tweak the order again in the future without a schema change. (My only worry would be the trigger taking forever due to the number of rows needing to be updated after a child_order
change. We could do it from psql with statement_timeout
disabled, but it might time out from the UI.) Let's see what @yvanzo thinks?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tbh I think further order changes are unlikely enough that there might not be a point here - we rarely add primary types and I cannot think of any that would require special sorting.
For secondary types that might actually be useful though, if we decide to implement a specific order for those (MBS-13790).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wondered if we should be using
release_group_primary_type.child_order
for this at first.
Yes, that seems obvious: child_order
is purposed for ordering, not type
.
require new trigger(s) […] taking forever
I’m not sure what trigger(s) you are speaking about.
About search, neither type
nor child_order
are looked after, only gid
and name
, so there won’t be any issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m not sure what trigger(s) you are speaking about.
Triggers to update artist_release_group
whenever release_group_primary_type
changes, since that means we need that data in here for the sort (and that means we also need an extra join here, of course). I don't expect to change the child_order
often so it might be okay though 🤷♂️
Description
Listing EPs above singles has been requested forever, and it makes sense in all markets except maybe Japan. Given Japan would want singles before albums, which we don't do anyway, this is probably not too relevant.
This just special-cases the sorting of those two, both on the slow method query directly, and in the
get_artist_release_group_rows
function for the fast method. We would need to update all the single and EP entries in the table, I guess, so I added a one-off script for that.Testing
Tested the slow method locally by using it by default for a moment (changing the condition for fast to
if ($self->has_materialized_artist_release_group_data && 0)
). Fast method is still untested.