-
Notifications
You must be signed in to change notification settings - Fork 360
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
Multi facet selection - Record last user state #4109
Conversation
ec04495
to
c17d041
Compare
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.
@EreMaijala, are tests passing for you? Mine are consistently failing with:
There were 2 failures:
1) VuFindTest\Mink\SearchFacetsTest::testCheckboxFacetSelection with data set "select one (multi/single)" (['Books'], 8, true, false)
Failed asserting that 'Showing 1 - 8 results of 8' [ASCII](length: 26) contains "Showing 1 - 19 results" [ASCII](length: 22).
/usr/local/vufind/module/VuFind/tests/integration-tests/src/VuFindTest/Mink/SearchFacetsTest.php:1224
2) VuFindTest\Mink\SearchFacetsTest::testCheckboxFacetSelection with data set "select two (multi/single)" (['Books', 'Fiction'], 7, true, false)
Failed asserting that 'Showing 1 - 7 results of 7' [ASCII](length: 26) contains "Showing 1 - 19 results" [ASCII](length: 22).
/usr/local/vufind/module/VuFind/tests/integration-tests/src/VuFindTest/Mink/SearchFacetsTest.php:1224
I haven't taken a close look at why this might be -- not sure if it's a bug in the code or in the test, assuming it's not just a local glitch.
Please let me know if you need me to do more investigation on my end; I thought it would be helpful to get a second opinion before diving in too deep!
@demiankatz Oops, I failed to consider the tests. They will obviously need to be updated because they assume multi-selection is inactive on each page load. |
@rominail, are you interested in trying to figure this out, or do you want help? (I don't think you've worked with the test suite before, so I'm happy to help if you don't have time to dive in at the moment -- but if you're interested in learning, this might be a good opportunity to do so). |
I just realized I totally missed the updates here |
6096bac
to
8eee075
Compare
@rominail, I noticed that there was a conflict here caused by the removal of the bootstrap3 theme; I went ahead and resolved it for you. It will be nice no longer having to edit two files for every change! Please let me know if you need anything else from my end.... |
Thanks, I'm working on setting up a docker image of vufind locally in order to test my PR, I'll let you know when I'm done or if I have troubles |
ca42d45
to
58dcb59
Compare
I added a test and modified the other ones |
Thanks, @rominail, I'll take a closer look at this soon! For future reference, there is a Phing task called |
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, @rominail, this is looking good! I just pushed up a couple of small commits -- one to make the types in your new trait methods slightly more specific, and one to improve the assertions in testMultiFacetsSelectionPersistence (I'm not sure why you were using assertStringContainsString to test a Boolean value, so I simplified that and added checks both before and after toggling the control to ensure we're checking the state in both situations).
I'm running the full test suite now and I expect it to pass. Unless something goes wrong and surprises me, I'll push the merge button on this within the next few minutes.
For the multi facets js selection, record user choice in local storage