projectblacklight
existing bookmarks from before 5.4.0 upgrade become undeletable
I'm having a weird issue with bookmarks, that I haven't been able to isolate/reproduce yet.
I report it here mostly just in case someone else runs into it, to leave a record and share notes.
I am running Blacklight 5.4.0 on a demo server. (not yet in production).
When I log into my own account, and look at my bookmarks, none of the bookmarks in the list have their checkbox checked. Which also means I am unable to individually remove any of them from my bookmarks. (No, checking and then unchecking again doesn't work).
I haven't been able to reproduce this on a fresh install though. I wonder if these bookmarks were saved under an earlier version of BL, and somehow the upgrade resulted in them being undeletable? If that's what happened, that might happen in my production too and would be a problem -- but it makes it hard to reproduce. Or maybe that's not what's going on, but something is going on, on my demo server anyway! Makes me worried about going into production without diagnosing and fixing it, lest all my users with existing bookmarks find they can't individually remove them.
If anyone has any ideas, they would be welcome!
Did you install the migrations, as noted in the release notes for 5.4.0? https://github.com/projectblacklight/blacklight/releases/tag/v5.4.0
I did, yeah. Installed them and ran them.
Although, hmm, it's possible I got confused and ran an older version of the app against the same db that already had the migrations run on them. Would that result in this problem?
Does the migration on the db need to be executed at exactly the same time you put the new code into production, with old code never running on the post-migrated db? If so, that makes going into production a bit harder, but is do-able.
Where possible, I usually run migrations first, without taking the app down, then upgrade the app logic. If old code running after migration causes undeletable bookmarks, guess I need to take app out of service, run migration, put upgraded back in service, to avoid chance of undeletable bookmarks getting created in the interim?
Okay. It is indeed that my already existing bookmarks in the database have a blank document_type column.
I did run the migration, but the migration only adds the document_type column -- that column will be NULL for any pre-existing bookmarks.
Anyone upgrading to BL 5.4.0 from previous versions of Blacklight will have existing bookmarks already saved, so I think this is a problem that needs resolution.
I'm not sure what the right solution is. One way would just be to make a migration that also sets "SolrDocument" as a column default in the db. Or to just have the migration `update bookmarks SET document_type = "SolrDocument" WHERE document_type is NULL"
With this second one, you'd run into the problem where first you run the migration, then some seconds or minutes later you update your Blacklight app -- if the old version of the app was still in use in this intervening seconds or minutes, some 'corrupt' and impossible to delete bookmarks could have been created. So I guess you'd need to run the migration, update the app, THEN run the db update statement again somehow (can't actually run the migration twice)
Appreciate advice from @cbeer or others who wrote the document_type stuff. I can write a PR, but I'm not quite sure what the direction to go is.
Here's some details of the code, that also reveals some un-DRY and in some cases inconsistent behavior....
BookmarksController fetches all of the current user's bookmarks without limiting to any certain document_type in the db:
https://github.com/projectblacklight/blacklight/blob/master/app/controllers/bookmarks_controller.rb#L26
However, the #current_bookmarks method fetches with a WHERE restriction to `SolrDocument. I can't quite trace where this code is coming from, it's several different methods interacting that I haven't been able to follow. But in the debugger, I can see the SQL being executed, and for me it's:
[2014-05-19 15:30:08.714] DEBUG Bookmark Load (1.4ms) SELECT `bookmarks`.* FROM
`bookmarks` WHERE `bookmarks`.`user_id` = 2 AND `bookmarks`.`user_type` = 'User' AND
`bookmarks`.`document_type` = 'SolrDocument' AND `bookmarks`.`document_id` IN ('bib_4354407',
'bib_4225647', 'bib_3443935', 'bib_2883108', 'bib_3579925', 'bib_2561788', 'bib_2327644',
'bib_4343906', 'bib_4333135', 'bib_2273851')
That is, with a `document_type = 'SolrDocument' restriction.
The bookmark_control partial that determiens whether the checkbox is checked is here: https://github.com/projectblacklight/blacklight/blob/master/app/views/catalog/_bookmark_control.html.erb#L7
The pertinent line:
if current_bookmarks.find { |x| x.document_id == document.id and x.document_type == document.class }.blank?
So, the list of bookmarks to display includes all my bookmarks, even those with null document_type But then when deciding if the checkbox should be checked, it 1) checks current_bookmarks to see if 2) a document exists with the same id and a properly set document_type.
In fact, it never gets to (2), because current_bookmarks applies the document_type restriction, so is an empty array. But if it did get to (2), it would again fail on the bookmarks with null `document_type.
That's just for checking to see if checkbox is checked. But even if the bookmarks is incorrectly being shown without a checkmark -- why doesn't checking, and then unchecking the checkbox succesfully delete the bookmark?
I'm not certain -- checking the box does call BookmarksController#create, and then unchecking it calls BookmarksController#destroy, which thinks it's succesfully deleted a bookmark. I think BookmarksController#create is maybe actually creating a duplicate bookmark (this one with document_type filled out?), and then BookmarksController#destroy is just deleting that duplicate?
There's so many different interacting things here that I'm not certain the proper solution here. Appreciate some advice! I am pretty certain this is really a bug.