Bookmark counter spec #3263
Bookmark counter spec #3263
Conversation
| visit solr_document_path('2007020969') | ||
| expect(page).to have_no_css('#bookmarks_nav') | ||
| check 'Bookmark' | ||
|
|
There was a problem hiding this comment.
Can you add a check here before navigating away from the page that shows it has been saved? Typically you can look for In Bookmarks in the page content.
There was a problem hiding this comment.
Thanks, good call.
I have added that check, but it fails the rails 6 check. I built an internal test app using rails 6.1.7.8 and encountered the same failure. When inspecting the app, it looks like the BookmarkToggle event handler callback is fired twice in succession, so the checkbox checks and unchecks itself with one click. I haven't pinpointed the issue, but perhaps it's related to how the js assets are built/handled when using rails 6?
There was a problem hiding this comment.
Some additional context:
In the rails 6.1.7.8 test app, the browser receives both a blacklight.source.js and blacklight.debug.js file that appear the same. The clicked function is called in both these js files according to the debugger. This has the effect of the checkbox being clicked twice in succession, which reflects the failing test. While I am unfamiliar with the asset pipeline for rails 6 and BL, I will investigate turning off debug mode.
There was a problem hiding this comment.
We can discuss pushing #3224 first at the next BL meeting. Then we don't have to worry about the odd things that webpack is doing.
As @mamrey noted on #3263, this can lead to a user being unable to add bookmarks, since the relevant eventlistener is added twice, so every time the user clicks to add a bookmark, the toggle function is run twice, essentially doing nothing. This bug (and fix) only affect Sprockets -- When using Importmap or Propshaft, this branch of the view isn't even reached.
…element - replaces previously removed flaky spec - cleans up modifications to CatalogController after test - clean up bookmarks before each example
mamrey
force-pushed
the
bookmark-counter-spec
branch
from
1a356ad
to
cffd2cc
Compare
|
Thank you for further investigating the issue and moving things forward @sandbergja! |
A flaky test from #3215 was removed in #3245.
This PR fixes and replaces said spec, testing the ability to add/remove bookmarks without the bookmark navbar element present.
These changes also add an
afterhook to clean up modifications toCatalogControllerin the bookmark icon exampleRan the bookmark test suite 100 times, all green.