-
Notifications
You must be signed in to change notification settings - Fork 5
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
Populate concept pages using ID instead of label [TOGGLED] #11414
Conversation
Size Change: +170 B (+0.02%) Total Size: 996 kB
ℹ️ View Unchanged
|
There is some strange behaviour on image search pages that results in a reload, after correctly filtering results. With @davidpmccormick's help we tracked this down to the behaviour of Screen.Recording.2024-11-21.at.17.12.37.movI don't think this is blocking for release as the bug should be unreachable by users without the toggle and will likely be fixed by the addition of aggregation and search filters by concept ID. Any advice on why it's happening would be useful though? |
Co-authored-by: Raphaelle Cantin <[email protected]>
ac4ae1f
to
64f3f9f
Compare
I'm not sure what we expect at this stage, but no filter is shown as selected http://localhost:3000/search/works?subjects.concepts=avkn7rq3%2Cg4ek4ccz and therefore can't be removed without changing the URL. The "All images" link does originally work (goes to Without the toggle though, it all seems to work as normal, so if those are expected we can merge? |
This is the expected behaviour without further work to update the way aggregations behave. The redirection to images I think is related, but I can't pin down where it comes from. This behaviour should be hidden by the toggle for regular users, so i'm not worried about this going out. |
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.
Left a comment for what I think is missing work in adding filters, but if we're happy with current behaviour behind toggle, I'm happy to get this merged as work in progress - functionalities without toggles were as normal.
The current filters for subject & contributor presume filtering by label, but the concept pages return a filter on concept identifier which is newly indexed when the toggle is on. The current filters also rely on aggregatable values for subject and contributor label which are not yet available in the catalogue API. The consequence is subject & contributor filters with the toggle on won't work as expected until we (a) re-index with subject & contributor as aggregatable values(b) update the catalogue-api to provide aggregations of these values, and (c) update the filters in this repository to accommodate. This issue tracks that work: wellcomecollection/platform#5825 (comment) It will all have to be either backwards compatible or toggled until done in order to gracefully release this change. This work is all part of wellcomecollection/platform#5781, removing "dead-ends" in concept pages in this quarters OKRs. I should have added this context to the ticket to start with, sorry! |
What does this change?
Warning
This needs to wait for the catalogue API change below to go out, will update with better test criteria when that's merged.
Follows:
Resolves: wellcomecollection/platform#5654
Part of: wellcomecollection/platform#5781
This change updates the search filters in use to populate concept pages to use IDs instead of labels, removing the opportunity for "dead-end" concept pages to exist.
How to test
How can we measure success?
Happy users, not seeing dead ends when browsing the collection through concepts.
Have we considered potential risks?
This change should be behind a toggle, so not in public view minimising wirk.