Skip to content

[filterable_datatable] Fix Query Param Filtering Bug #7245

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

Conversation

HenriRabalais
Copy link
Collaborator

@HenriRabalais HenriRabalais commented Dec 17, 2020

Brief summary of changes

This PR updates the functionality of the Filterable datatable by changing the way that it loads filters via the URL query params.

Instead of simply loading the query params into the filter object, upon component mount the query params will each trigger the update of the respective field they are associated with, if they exist.

The resolves the issue #7216 which was causing filters that were meant to be exact match filters to be treated as substring, or "partial", filters.

Testing instructions (if applicable)

  1. Go to any module with a reactified filterable datatable.
  2. Enter some filter values.
  3. Refresh the page and ensure that the exact same filters are applied.
  4. Do steps 2-3 with a select dropdown where the index for the value selected is a substring of other index values. (E.g. Select a subproject whose index is 1 when there are other subprojects in the list whose indexes are anywhere from 10-19).

Link to related issue

@ridz1208
Copy link
Collaborator

ridz1208 commented Dec 18, 2020

@HenriRabalais travis is still failing

@kongtiaowang can you help

@HenriRabalais
Copy link
Collaborator Author

@ridz1208 Yes, I tried make checkstatic but everything seems to be working. I'm also not clearly interpreting the messages from travis, so I'm unsure how to proceed.

@ridz1208
Copy link
Collaborator

ridz1208 commented Dec 18, 2020

Screen Shot 2020-12-18 at 1 30 43 PM

the error is the query selector (see image) its not a static error

@driusan
Copy link
Collaborator

driusan commented Dec 18, 2020

make checkstatic is passing on GitHub too.. it's the integration tests that are failing.

@kongtiaowang
Copy link
Contributor

@HenriRabalais I am looking into it. The integration test of media filter faild.

@kongtiaowang kongtiaowang added the State: Needs work PR awaiting additional work by the author to proceed label Dec 18, 2020
@kongtiaowang
Copy link
Contributor

kongtiaowang commented Dec 18, 2020

There is a bug found by Integration Test.
File Name filter doesn't work, it always shows no results.
This PR has fixed PSCID filter, not File Name filter.
屏幕快照 2020-12-18 下午3 48 58
屏幕快照 2020-12-18 下午3 49 47

@kongtiaowang kongtiaowang removed the "Help! I don't understand Travis!" PR is having a beef with TRAVIS. Someone needs to help label Dec 18, 2020
@HenriRabalais
Copy link
Collaborator Author

Thanks @kongtiaowang !!

@HenriRabalais HenriRabalais removed the State: Needs work PR awaiting additional work by the author to proceed label Jan 4, 2021
@HenriRabalais
Copy link
Collaborator Author

@ridz1208 needs re-review

@laemtl laemtl self-requested a review January 8, 2021 20:54
Copy link
Contributor

@laemtl laemtl left a comment

Choose a reason for hiding this comment

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

Works well for 1 filter, but not 2, where only the second is considered.

@HenriRabalais
Copy link
Collaborator Author

HenriRabalais commented Jan 12, 2021

Works well for 1 filter, but not 2, where only the second is considered.

@laemtl Could you provide a screenshot example?

@laemtl
Copy link
Contributor

laemtl commented Jan 12, 2021

@HenriRabalais

/battery_manager/?subproject=2&visitLabel=V1

Expected
Screenshot from 2021-01-12 15-10-35

Result
Screenshot from 2021-01-12 15-11-05

@HenriRabalais
Copy link
Collaborator Author

@laemtl should be fixed now!

@laemtl
Copy link
Contributor

laemtl commented Jan 20, 2021

@HenriRabalais Awesome, the problem is fixed! I found another small issue:

  • if you visit /battery_manager/?visitLabel=V1&subproject=1&stage=Approval, you will notice that the subproject filter (Stale) is ignored. The filters values are correctly populated in the form area now, but the results contain 2 entries for the CIE subproject, which are not there when the filters are selected manually.

Screenshot from 2021-01-20 14-53-21

@ridz1208
Copy link
Collaborator

ridz1208 commented Feb 6, 2021

@HenriRabalais please fix when possible

@ridz1208 ridz1208 removed this from the 24.0.0 milestone Feb 6, 2021
@laemtl laemtl added the Passed manual tests PR has been successfully tested by at least one peer label Feb 9, 2021
@laemtl laemtl self-requested a review February 9, 2021 21:39
@driusan driusan merged commit b52a8a9 into aces:23.0-release Feb 11, 2021
@ridz1208 ridz1208 added this to the 23.0.3 milestone Mar 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Passed manual tests PR has been successfully tested by at least one peer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[battery_manager] When loading the module with a predefined filter in the URL the filter is not applied
5 participants