Skip to content

MBS-14005 / MBS-14079: Show containment for place areas in search (inline and not) #3584

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

reosarevok
Copy link
Member

@reosarevok reosarevok commented Jun 27, 2025

Fix MBS-14005 / Implement MBS-14079

Problem

Place search results only provide the basic area for the place. In many cases, this is not super useful information to be sure the place is the right one. For example, the MBS-14005 example is in "York" - not UK, but a district of Toronto. But nothing about Toronto or Canada is shown in the search results, so a user might well skip it thinking it has nothing to do with what they are looking for.

Solution

For inline search, our code already tried to load the containment - but we had broken it in a previous commit so that nothing was ever loaded. The first commit just fixes that.

For normal search, this just loads the data for both direct and indexed search results and changes the area column to display containment.

Testing

Manually in both inline and normal search, both indexed and direct.

@reosarevok reosarevok added QoL Non-urgent quality of life improvements Bug Bugs that should be checked/fixed soonish labels Jun 27, 2025
@mwiencek
Copy link
Member

The load method doesn't actually return anything, it seems, so @areas was always empty and no containment was loaded. This just uses the same way of loading areas as we use elsewhere.

Apparently that's a regression from e6a7c14#diff-1cb6be6da9da5e50588b6bc9017c3b6c83124e00d030c525acc7836fcbcc7a91. In general load does return the entities for any model, so we should probably fix that (it is a bit more direct and convenient than having to remap them again).

@mwiencek
Copy link
Member

mwiencek commented Jul 7, 2025

If I perform a regular place indexed search (ssh tunneled to one of our mb-solrcloud instances), I get this error which doesn't occur on master:

Can't call method "gid" on an undefined value at lib/MusicBrainz/Server/Data/Area.pm line 331. at lib/MusicBrainz/Server/Data/Area.pm line 331
MusicBrainz::Server::Data::Area::load_ids(?, ?, ?, undef, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) called at lib/MusicBrainz/Server/Data/Search.pm line 970
MusicBrainz::Server::Data::Search::external_search(?, ?, ?, ?, ?, ?) called at lib/MusicBrainz/Server/Controller/Search.pm line 257
MusicBrainz::Server::Controller::Search::do_external_search(?, ?, ?, ?, ?, ?, ?, ?, ?, undef, ?, ?) called at lib/MusicBrainz/Server/Controller/Search.pm line 238
Catalyst::forward(?, ?) called at lib/MusicBrainz/Server/Controller/Search.pm line 61
Catalyst::dispatch(?) called at lib/MusicBrainz/Server.pm line 402
MusicBrainz::Server::__ANON__ at lib/MusicBrainz/Server.pm line 367
MusicBrainz::Server::with_translations(?, ?) called at lib/MusicBrainz/Server.pm line 403
Class::MOP::Method::Wrapped::__ANON__(?) called at lib/MusicBrainz/Server.pm line 417
Class::MOP::Method::Wrapped::__ANON__(?) called at lib/MusicBrainz/Server.pm line 536

I'm also a bit confused about the load_ids addition to lib/MusicBrainz/Server/Data/Place.pm, because I don't see $self->c->model('Place')->load_ids(...) called anywhere in this PR (only against the area model). Is it being called elsewhere somehow?

The load method didn't actually return anything since it was
changed with e6a7c14 -
I did not realize at the time we were using the return value
here.
That means @areas was always empty and no containment was loaded.
I explicitly return @areas now from area::load - eventually
we might want to make the returns explicit for all of these
to avoid confusions like this one in the future.
This loads and displays containment for place areas, since
areas are very important for understanding places
and this helps better figure out the results at a glance.
We might want to do the same for other entities with areas
(mostly artists and labels) in the future, but for now I think
the closest analogy is events and those do show full containment.
@reosarevok
Copy link
Member Author

Fixed that, I think. The place load_ids was actually because I was loading those originally but eventually realized it's not needed... but forgot to drop the method. Dropped it now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Bugs that should be checked/fixed soonish QoL Non-urgent quality of life improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants