-
Notifications
You must be signed in to change notification settings - Fork 359
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
Add bulk action buttons to bottom of favorites #4211
base: dev
Are you sure you want to change the base?
Add bulk action buttons to bottom of favorites #4211
Conversation
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.
Thanks, @ThoWagen -- I went ahead and resolved the conflicts to make the PR easier to read, then left a couple of suggestions below. I haven't tried hands-on testing yet.
themes/bootstrap5/templates/myresearch/bulk-action-buttons.phtml
Outdated
Show resolved
Hide resolved
themes/bootstrap5/templates/myresearch/bulk-action-buttons.phtml
Outdated
Show resolved
Hide resolved
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.
@ThoWagen, this looks good to me when I do hands-on testing, but I'm encountering a strange thing: I started adding some automated tests to cover the functionality of the bottom buttons, just so we ensure that they don't break in future. See commit 0cdcc27.
These new tests are passing for me about 90% of the time... but I've encountered some intermittent failures, most frequently:
1) VuFindTest\Mink\BulkTest::testBulkPrint with data set "bottom button" ('bottom_')
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'print=true&id[]=Solr|testsample1&id[]=Solr|testsample2'
+'lookfor=id%3A(testsample1+OR+testsample2)'
/usr/local/vufind/module/VuFind/tests/integration-tests/src/VuFindTest/Mink/BulkTest.php:355
It seems like once in a while, the click on the bottom button is failing, or the timing is off, and as a result, we stay on the search results instead of loading the print view.
This ONLY happens to me when clicking the bottom button (so far), and this test has never failed in the past with the top buttons.
I'm curious whether you see the same thing in your own test environment if you try these tests, or if you have any theories about what may be going on. It could be a quirk of Mink (we've seen other cases where clicks sometimes fail to register, for example, and maybe something isn't rendering right "off-screen" in the headless browser). It might also be a timing issue, but I don't think so: I added code to wait a second and try again, and that didn't seem to make any difference. However, just in case it's pointing to some real underlying issue, I thought I'd ask for your confirmation. If nothing stands out to you, I can also check with @EreMaijala, who has gotten quite good at diagnosing weird Mink problems. ;-)
Similar to the search lists this PR adds the bulk selection buttons to the bottom of the favorites and switches the the order of the buttons with selection controls bar in both cases.