Fix bulk favorites save from search results #3960
Conversation
|
Thanks for catching this, @LuomaJuha. One question, though: is it possible a better solution would be to ensure we never use GET for this form, rather than making the controller tolerant of GET? I'm concerned that if this is not a POST, the URL may become too long in some scenarios, so I think fixing the form method rather than the processing might be the safer solution. But maybe there's a reason that is not possible, in which case this may be the best we can do. Let me know what you think, and whether you'd like me to look into this more deeply when time permits. (This week and next my time is short due to conferences). |
|
Made a small change, could this be better? Actually after i made this change, i noticed better way, gonna adjust that.... |
LuomaJuha
requested review from
demiankatz
and removed request for
demiankatz
|
Thanks, @LuomaJuha -- please let me know when you're ready for me to take another look! |
|
@demiankatz will do! |
|
I modified the templates to submit instead of using links and forward to proper controllers, and also added action to form. |
There was a problem hiding this comment.
Thanks, @LuomaJuha; this looks reasonable to me, though the test suite is now failing. Do you have a setup that you can use to fix the test? If not, let me know and I can see if I can help. In any case, here are the failures I'm seeing:
There was 1 error:
1) VuFindTest\Mink\FavoritesTest::testAddRecordToFavoritesNewAccount
Exception: Failed to set value after 6 attempts.
/usr/local/vufind/module/VuFind/src/VuFindTest/Integration/MinkTestCase.php:677
/usr/local/vufind/module/VuFind/tests/integration-tests/src/VuFindTest/Mink/FavoritesTest.php:144
--
There was 1 failure:
1) VuFindTest\Mink\ListViewsTest::testFavoritesInAccordionMode
Element not found: .modal-body .btn.btn-primary index 0
Failed asserting that null is of type object.
/usr/local/vufind/module/VuFind/src/VuFindTest/Integration/MinkTestCase.php:514
/usr/local/vufind/module/VuFind/src/VuFindTest/Integration/MinkTestCase.php:606
/usr/local/vufind/module/VuFind/tests/integration-tests/src/VuFindTest/Mink/ListViewsTest.php:154
ERRORS!
Tests: 2937, Assertions: 18477, Errors: 1, Failures: 1, Skipped: 22.
There was a problem hiding this comment.
Thanks, @LuomaJuha -- all tests are passing for me, and this seems to make sense to me. Just one question about a possible simplification....
themes/bootstrap3/templates/cart/form-record-hidden-inputs.phtml
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Thanks, @LuomaJuha; looks like this needs a vendor/bin/phing update-bootstrap5 to get everything back in sync now.
| <?php | ||
| $recordId = $this->driver?->getUniqueId() ?? $this->recordId ?? false; | ||
| $recordSource = $this->driver?->getSourceIdentifier() ?? $this->recordSource ?? DEFAULT_SEARCH_BACKEND; | ||
| if ($recordId && str_contains($recordId, '|')) { |
There was a problem hiding this comment.
I'm concerned that this could malfunction in multiple ways if an ID contains a pipe character. Is there a safer way to determine when unpacking is required? i.e. do we only unpack recordId if there is no driver and no source set? (I didn't analyze all the code paths to figure out options, but I can try to spend a little time on that if it would help).
Additionally, are the variables reversed? Down below, you construct "source|id" strings, but here you're unpacking it as "id|source".
There was a problem hiding this comment.
oh, missed that... will adjust them also!
There was a problem hiding this comment.
Hmm, i think it would be safer to leave it out and let the calling templates to give what is needed? I think that part was littlebit of overthinking.
There was a problem hiding this comment.
Yes, definitely safer this way -- thanks!
There was a problem hiding this comment.
This looks good to me now, and tests are passing. @ThoWagen offered to take a look at it as well, though, so I'll wait to hear from him before merging in case he notices anything I have overlooked.
|
@demiankatz Great! More eyes, the better :) |
There was a problem hiding this comment.
This looks good when it comes to fixing the problem!
I wonder if we should move more of the logic for managing list to the FavoritesService and calling that in all the controllers instead of redirecting and forwarding back and forth between actions. That could reduce the amount of work and susceptibility to error when manipulating the query and post parameters for each redirect/forward. The controllers would only need to acquire all the necessary parameters from the request and pass them to the service.
Since that would be a more significant refactoring it would probably make sense to do that in a separate PR if you think it's worth it and just fix the error here.
|
@ThoWagen I think that could be better in longer run! |
|
Sounds good. I'll merge this as-is, and maybe we can have a follow-up targeted against the dev-11.0 branch to do larger refactoring for the next major release, if @LuomaJuha wants to take that on! |
Steps to reproduce: select items in the search results, save to list, create new list.