-
-
Notifications
You must be signed in to change notification settings - Fork 287
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-13597: Add release filter to label index pages #3450
base: master
Are you sure you want to change the base?
Conversation
This was always trying to compare, leading to "argument isn't numeric" warnings.
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.
Tested locally and worked well for me, including the old release filter for artists. Look into adding the copyright/license comments at least, and maybe the suggested improvements if you agree with those.
@@ -253,6 +266,22 @@ sub find_labels_by_artist | |||
return $self->c->model('Label')->get_by_ids_sorted_by_name(@$ids); | |||
} | |||
|
|||
# This might seem stupid, but it helps filtering multi-label releases | |||
sub find_labels_by_label |
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.
It feels like this belongs more under Data::Label
as find_by_release_label
, since you can just join with label
there and use query_to_list
directly.
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 could see that, but that would also seem true of find_labels_by_artist
then? Admittedly I was a bit suprised all those things are in Data::Release
(find_artist_credits_by_label
I would have expected on AC and whatnot).
This extends the release filter from the artist releases page to label index pages, which have very similar lists. Most of the code can be reused, so I moved some into roles. The only real difference with the artist forms are that ACs are taken for all the releases in the label and that "no label exists" is not a filter option for labels, for obvious reasons. Tests added, with very small changes from the artist release ones.
Implement MBS-13597
Problem
We have very useful filter tools for all sort of artist pages that have long and unwieldy tables. Another thing that is long and unwieldy: label pages with sometimes thousands of releases. And we cannot filter those.
Solution
This extends the release filter from the artist releases page to label index pages, which have very similar lists. Most of the code can be reused, so I moved some into roles.
The only real difference with the artist forms are that ACs are taken for all the releases in the label and that "no label exists" is not a filter option for labels, for obvious reasons.
Testing
Tests added, with very small changes from the artist release ones.
While testing this locally I saw we were getting a warning due to the new code to keep ACs when moving around in filters, so I deal with that in the small first commit.