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 vocabulary search using "0" as the search string #1267

Conversation

miguelvaara
Copy link
Contributor

@miguelvaara miguelvaara commented Jan 26, 2022

Reasons for creating this PR

Fix the remaining cases in #1260 ; the REST API search using "0" has been fixed in #1261 and #1276, but searches for "0" in the web UI are still not working until this PR.

Link to relevant issue(s), if any

Description of the changes in this PR

  • fix ConceptSearchParameters.getSearchTerm() so it looks at the right URL parameter q even when its value is "0"
  • rename the test provider method testSearchWithZeroData into provideSearchWithZeroData to avoid PHPUnit considering it a test case and complaining that it's "Risky" because it doesn't perform any assertions

Known problems or uncertainties in this PR

n/a

Checklist

  • phpUnit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works (if not, explain why below)
  • The PR doesn't introduce unintended code changes (e.g. empty lines or useless reindentation)

…tation code but not in the middle of it (YKL). At the moment it causes problems in searches related to the vocabularies not using notation code - prefLaber combination (such as Juho).
… related to the dataProvider for searching with zero (API) fixed.
@codecov
Copy link

codecov bot commented Feb 16, 2022

Codecov Report

Merging #1267 (e5b066d) into master (3a1902b) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1267   +/-   ##
=========================================
  Coverage     69.36%   69.36%           
  Complexity     1650     1650           
=========================================
  Files            32       32           
  Lines          4047     4047           
=========================================
  Hits           2807     2807           
  Misses         1240     1240           
Impacted Files Coverage Δ
model/sparql/GenericSparql.php 92.24% <ø> (ø)
model/ConceptSearchParameters.php 85.55% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3a1902b...e5b066d. Read the comment docs.

@miguelvaara miguelvaara self-assigned this Feb 16, 2022
@miguelvaara miguelvaara marked this pull request as ready for review February 16, 2022 16:41
@osma
Copy link
Member

osma commented Feb 17, 2022

Can you please update the PR title and description so it corresponds to current status?

@kinow
Copy link
Collaborator

kinow commented Feb 17, 2022

Hadn't seen this PR, I think it's also related to the zeroes in the beginning of the search, sorry! Feel free to close mine and centralize on this one if you prefer, or use anything from that PR too 👍

@osma
Copy link
Member

osma commented Feb 17, 2022

Need to fix the conflicts caused by #1276.

@osma osma self-assigned this Mar 3, 2022
@osma osma changed the title Generates a search result containing zeros in the beginning of the no… Fix vocabulary search using "0" as the search string Mar 3, 2022
@sonarcloud
Copy link

sonarcloud bot commented Mar 3, 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

@osma
Copy link
Member

osma commented Mar 3, 2022

I fixed the conflicts, cleaned up the PR (removed useless test data), edited the PR title, rewrote the PR description, and tested this. I think it should be good to go now. Will wait for QA jobs, do a final check, then merge if there are no problems.

@osma osma added this to the 2.14 milestone Mar 3, 2022
@osma osma merged commit 2a61720 into master Mar 3, 2022
@osma osma deleted the issue1260-reopened-vocabulary-search-using-0-as-the-query-argument-throws-an-error branch March 3, 2022 13:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

Vocabulary search using "0" as the query argument throws an error
3 participants