spotlight
spotlight copied to clipboard
projectblacklight
Bug updating browse category that exists in SirTrevor blocks
If you update a browse category in a way that triggers a slug update, any page that includes that browse category in the browse category block will throw an error on the edit page.
This is because the as_json method calls a find which can trigger an ActiveRecord::NotFound error.
It also appears that it might be possible to create a browse block w/o any browse categories, which can cause an error as well.
I've been thinking about how to solve this without too much trouble. I'm beginning to suspect we may want to introduce a new join table to link pages with embedded resources (and use the primary key of that link in the blocks themselves).
I think this problem can also occur if a browse category is renamed (not just deleted).
I agree that ultimately https://github.com/projectblacklight/spotlight/issues/1782#issuecomment-288609585 is going to be the approach that we should take (or should have taken initially) however it will require a non-trivial amount of work to accomplish and will require data migrations to be written to migrate all existing data to an appropriate join table.
If we want a stop-gap to deal w/ the issue in the interim I can think of two ways that we might approach this which will have varying user experience. As it stands the rendered view of the page works out fine and simply does not show the browse category that has changed. On the edit screen we can potentially do one of the following.
- Show the browse category whose slug has changed with some sort of styling and/or note indicating that the browse category will not render and most likely has had its slug changed.
- We can simply omit the browse category from displaying on the edit screen all-together.
If we want to go with the approach in the comment above that'll take a bit more time, and wanted to stop to check-in before making a decision to go one way or the other.
I also agree keeping track of the embedded categories by PK rather than slug is way we want to go, long-term, since eliminates the problem with no unexpected UX issues.
If we don't think we can work on that ideal approach in the near future, I think @jkeck's second option sounds best to me (if I understand things correctly). The downside of the curator having to re-select a category whose slug changed, while not ideal and possibly slightly confusing (especially if someone else was the one to change the browse category slug), seems tolerable and cleaner than the first option would be.
Unless I am misunderstanding, either of the two options above will not result in an acceptable exhibit creator experience. Jessie -- Since the work to fix this is non-trivial, I am wondering how much time it would take to fix the problems by hand in the two exhibits that are currently experiencing them, if I provide the current page slug? Then the non-trivial fix can be scheduled for our next work cycle, so that this work doesn't unduly detract from our current Parker focus.
Gary as I was writing this comment, I saw your response. There must be something I am not understanding about option 2. The whole point is that the exhibit creator needs to edit the page(s) in question. Anyway we can discuss in person tomorrow.
I'm mentioning something that can be implemented nearly immediately and will solve the issue of certain pages being completely un-editable in the immediate term while working on a more permanent solution in the long term. I understand that it may not be considered an acceptable exhibit creator experience, but it is a decidedly better experience than receiving an error page and being unable to edit the page at all.
As far as the more involved solution, I've actually been thinking about this and am wondering if it actually ultimately solves the problem while maintaining data integrity through export/import (which is why we store the slug of the browse category instead of the primary key). Perhaps there is something I don't understand about the data that we would end up persisting in the join table mentioned in https://github.com/projectblacklight/spotlight/issues/1782#issuecomment-288609585 but if it's a join table that I'm thinking of it would be storing primary keys and would not necessarily export properly. I would be happy to hear that I'm wrong about this, and it might be part of Active Record or our export code that I'm not as familiar with (and would be happy to learn more).
If I understand the approach suggested correctly, it would require that we add some database migrations to add a join table, we would need to update the widget code to handle the new join table primary key, and write data migration code that can update all pages that have browse category widgets. The first two tasks are relatively straight forward with the last one being the most complicated part (and I want to validate it doesn't cause issues with export as I mention above).
Another approach that I had initially suggested when we first noticed this bug is to attach an after save event to the browse category that will update any pages within that particular exhibit that has that browse category. That code might be a bit easier to write/test, but not sure if it's the correct approach (e.g. we want to avoid getting into a situation where we have a lot of these save callbacks to try to update widget content instead of using the correct data model).
This bug also affects the sir trevor widget for page blocks. Locally, i made the same fix to the page block model that was made here to the browse block model and that is working for now.
@starsplatter would you be willing to submit a patch to fix the issue w/ Page blocks?
This is at least partially addressed by @dunn PR #2213 - sir_trevor: remove deleted feature pages from widget. It fixed the issue we saw locally reported by @starsplatter. I was able to remove our override.
