Skip to content
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

Fix header bar search form #1354

Merged
merged 4 commits into from
Sep 13, 2022
Merged

Fix header bar search form #1354

merged 4 commits into from
Sep 13, 2022

Conversation

joelit
Copy link
Contributor

@joelit joelit commented Sep 6, 2022

Reasons for creating this PR

Cookies are not set correctly for things like multiple vocabulary selection in the global search field. This would break the global search UI element, restricting the use of global search to only skosmos API or writing the search parameters directly to the URL.

Link to relevant issue(s), if any

Description of the changes in this PR

Known problems or uncertainties in this PR

The search vocabulary parameter for "all vocabularies" is not set correctly in the cookie.

Checklist

  • phpUnit tests pass locally with my changes
  • I have added tests that show that the new code works, or tests are not relevant for this PR (e.g. only HTML/CSS changes)
  • The PR doesn't reduce accessibility of the front-end code (e.g. tab focus, scaling to different resolutions, use of .sr-only class, color contrast)
  • The PR doesn't introduce unintended code changes (e.g. empty lines or useless reindentation)

@joelit joelit added this to the 2.16 milestone Sep 6, 2022
@codecov
Copy link

codecov bot commented Sep 6, 2022

Codecov Report

Merging #1354 (e8715d1) into master (5e6da4b) will increase coverage by 1.24%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##             master    #1354      +/-   ##
============================================
+ Coverage     70.68%   71.92%   +1.24%     
- Complexity     1646     1831     +185     
============================================
  Files            32       32              
  Lines          3786     3961     +175     
============================================
+ Hits           2676     2849     +173     
- Misses         1110     1112       +2     
Impacted Files Coverage Δ
model/ConceptPropertyValueLiteral.php 93.87% <0.00%> (-1.78%) ⬇️
model/Concept.php 87.63% <0.00%> (+4.00%) ⬆️
model/LabelSkosXL.php 34.61% <0.00%> (+34.61%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@joelit
Copy link
Contributor Author

joelit commented Sep 7, 2022

When fixing the language setting for the same menu, it turned out the component which writes the search query url parameters hasn't worked properly beforehand, as the paremeters aren't separeted and the parameter values are not encoded.
Screenshot from 2022-09-07 11-29-29
Skosmos tries to pre-fill the search query in the text field and fails parsing it.

@joelit joelit marked this pull request as ready for review September 13, 2022 11:12
@joelit joelit requested a review from osma September 13, 2022 11:13
@joelit joelit added the bug label Sep 13, 2022
@osma
Copy link
Member

osma commented Sep 13, 2022

The search vocabulary parameter for "all vocabularies" is not set correctly in the cookie.

I think I know why this happens. Selecting/deselecting options (vocabularies) from the multiselect widget triggers the onChange event, which again calls the updateVocabParam() function that sets the cookie:

onChange: function(element, checked) {
if (element) {
vocabId = element[0].value;
} else {
vocabId = '';
}
if (checked && selectedVocabs[vocabId] === undefined) {
selectedVocabs[vocabId] = vocabId;
} else if (selectedVocabs[vocabId] !== undefined) {
delete selectedVocabs[vocabId];
}
updateVocabParam();
},

But onChange is not triggered by the select all option. You need to define a separate onSelectAll event handler.

Copy link
Member

@osma osma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested this and couldn't make it not work :) Looks good, apart from the issue that "select all" doesn't set the cookie, which you already noted. I suggested a fix in the above comment. It's also possible to consider that out of scope and file a separate issue about it.

@sonarcloud
Copy link

sonarcloud bot commented Sep 13, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@joelit
Copy link
Contributor Author

joelit commented Sep 13, 2022

The fix was a pretty simple one; removing the selected vocabularies and calling the updateVocabParam() function seemed to do the trick, as the empty selectedVocabs JS variable and no SKOSMOS_SELECTED value in the cookie is interpreted as having all the vocabularies chosen. :)

@joelit joelit requested a review from osma September 13, 2022 12:34
Copy link
Member

@osma osma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good and works perfectly! 🎉

Before merging, I suggest changing the PR title to something more descriptive. This fixes many problems related to global search, not just setting cookies.

@joelit joelit changed the title Setting the cookie for multiple vocabularies in global search Fix header bar search form Sep 13, 2022
@joelit joelit merged commit 63ebd6e into master Sep 13, 2022
@joelit joelit deleted the issue1345-fix-global-search branch September 13, 2022 12:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Search function does not limit the search to a single vocabulary
2 participants