Skip to content

Fix date filter bugs#1138

Merged
tdonohue merged 9 commits intoDSpace:mainfrom
atmire:w2p-78991_Issue-1112_Fix-date-filter-bugs
May 4, 2021
Merged

Fix date filter bugs#1138
tdonohue merged 9 commits intoDSpace:mainfrom
atmire:w2p-78991_Issue-1112_Fix-date-filter-bugs

Conversation

@ybnd
Copy link
Copy Markdown
Member

@ybnd ybnd commented Apr 29, 2021

References

Description

Fix filtering by date range

Instructions for Reviewers

List of changes in this PR:

  • _custom_variables.scss: Initialize --ds-slider-handle-width: 18px; so the handles show up as handles instead of just lines

  • Add an 'equals' operator to range filters.

    This used to be optional, but now the backend returns 422 if no operator is specified.

  • Set the fallback max date to the current year (used to be 2018)

  • Fix horizontal position of vertical lines in range handles (were forced all the way to the left)

  • Fix .search-filter-wrapper getting stuck with .closed when actually in open state

Reviewing

  1. Search on https://demo7.dspace.org/ or a local instance
  2. Filter by date (range) -- results should load and be filtered by the specified date (range).
  3. Drag the 'max' handle all the way to the right -- the right input box should show the current year

Checklist

  • My PR is small in size (e.g. less than 1,000 lines of code, not including comments & specs/tests), or I have provided reasons as to why that's not possible.
  • My PR passes TSLint validation using yarn run lint
  • My PR doesn't introduce circular dependencies
  • My PR includes TypeDoc comments for all new (or modified) public methods and classes. It also includes TypeDoc for large or complex private methods.
  • My PR passes all specs/tests and includes new/updated specs or tests based on the Code Testing Guide.
  • If my PR includes new, third-party dependencies (in package.json), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.

@artlowel artlowel added bug component: Discovery related to discovery search or browse system e/3 Estimate in hours high priority testathon Reported by a tester during Community Testathon 1 APPROVAL pull request only requires a single approval to merge labels Apr 29, 2021
@artlowel artlowel added this to the 7.0 milestone Apr 29, 2021
@tdonohue tdonohue self-requested a review April 29, 2021 14:33
Copy link
Copy Markdown
Member

@tdonohue tdonohue left a comment

Choose a reason for hiding this comment

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

@ybnd : I tried testing this today, but it's not working for me. I pulled down this PR, and ran yarn start. Everything builds fine.
Search works, & all of the other filters work fine.

But as soon as I use the date filter, the page just says "Loading search results...". In Chrome DevTools, I see that all the date filter requests are returning a "CORS error", which is odd because none of the other filter requests do so. It almost seems like the request itself isn't being sent properly, because the error console has many errors that look like this

Access to XMLHttpRequest at 'http://localhost:8080/server/api/discover/facets/has_content_in_original_bundle?page=0&size=2&sort=score,DESC&page=0&size=10&query=test&f.dateIssued=[1963%20TO%201992]' from origin 'http://localhost:4000' has been blocked by CORS policy: Response to preflight request doesn't pass access control check: No 'Access-Control-Allow-Origin' header is present on the requested resource.

@artlowel
Copy link
Copy Markdown
Member

artlowel commented May 3, 2021

@tdonohue I can't reproduce your issue when I test it against api7.dspace.org. I suspect It may be an issue with your rest server

@artlowel artlowel requested a review from tdonohue May 3, 2021 14:35
@tdonohue
Copy link
Copy Markdown
Member

tdonohue commented May 3, 2021

@artlowel and @ybnd : I looked into this more today on my end. In the end, the error is not actually from CORS & it's not coming from our REST API

Instead, the underlying problem is that my Tomcat (v8.5) is throwing this error on every date filter query:

java.lang.IllegalArgumentException: Invalid character found in the request target.

By default, it appears (at least some versions of) Tomcat doesn't like unescaped square brackets ([]) in URL query params. See, for example, this StackOverflow question: https://stackoverflow.com/q/44011774/3750035 and this one https://stackoverflow.com/q/11490326/3750035

It appears the the frontend is sending square brackets unescaped...which can result in inconsistent behavior (it may or may not work depending on how you have Tomcat configured, or even which version of Tomcat you use). My best guess is that api7.dspace.org is either running a different version of Tomcat (which handles this better), or has configured Tomcat to handle this better.

If, however, I change your date filter query to send escaped square brackets, then everything seems to work for me.

So, this query throws an error in my Tomcat

http://localhost:8080/server/api/discover/facets/author?page=0&size=5&sort=score,DESC&page=0&size=10&query=test&f.dateIssued=[1992%20TO%20*],equals

But, this one works fine (and valid results are returned). The only difference is [ is encoded as %5B and ] is encoded as %5D.

http://localhost:8080/server/api/discover/facets/author?page=0&size=5&sort=score,DESC&page=0&size=10&query=test&f.dateIssued=%5B1992%20TO%20*%5D,equals

So, in my opinion, this should be fixed on the frontend (where it can be done more easily). A backend fix would be harder as it may end up being specific for different versions of Tomcat, which would be ugly to document, etc.

@artlowel
Copy link
Copy Markdown
Member

artlowel commented May 3, 2021

You're right. We'll fix it

ybnd added 4 commits May 4, 2021 10:34
* Extract "selective encoding" ops into separate methods
* Add comments to clarify regex
* Use hasValue() instead of just checking on 'match'
* Leave only the last comma of filter values unencoded
Copy link
Copy Markdown
Member

@tdonohue tdonohue left a comment

Choose a reason for hiding this comment

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

👍 Re-tested after the updates, and it's all working now for me (and I see the queries are now url encoded properly)! Thanks @ybnd !

@tdonohue tdonohue merged commit 9dc9b5a into DSpace:main May 4, 2021
kosarko pushed a commit to ufal/dspace-angular that referenced this pull request Jan 7, 2026
Co-authored-by: Matus Kasak <matus.kasak@dataquest.sk>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

1 APPROVAL pull request only requires a single approval to merge bug component: Discovery related to discovery search or browse system e/3 Estimate in hours high priority testathon Reported by a tester during Community Testathon

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Date filter no longer works in Search

3 participants