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 multiple filtering bug in https://github.com/participedia/usersnaps/issues/960 #824

Merged
merged 2 commits into from Jan 16, 2020

Conversation

@zphingphong
Copy link
Collaborator

zphingphong commented Jan 13, 2020

No description provided.

@zphingphong zphingphong requested a review from ascott Jan 13, 2020
@@ -470,4 +470,5 @@ module.exports = {
"partial_editing",
"complete",
],
query: []

This comment has been minimized.

Copy link
@ascott

ascott Jan 13, 2020

Contributor

i'm not sure about this solution. this file is for storing the option keys for fields, and query isn't a field. i'm wondering if there is another solution here?

This comment has been minimized.

Copy link
@zphingphong

zphingphong Jan 14, 2020

Author Collaborator

@ascott thanks! how about this solution? 43d3b5f

@ascott

This comment has been minimized.

Copy link
Contributor

ascott commented Jan 13, 2020

@zphingphong moving your comment from the issue here for discussion:

The bug is

All query parameter filter are in shared-field-option.js.
Now, if there are multiple query. The filtering logic happened in handlebars-helpers.js@filterCollections().

And the error shows up because the query parameter query doesn't exist in shared-field-option.js.

To fix this I added a new item in the array for query to handle this line in shared-field-option.js
if ( sharedFieldOptions[keys[i]].includes(arr[x]) ) {

can you edit handlebars-helpers.js@filterCollections() to handle the case when the query param is present rather than adding it as a field in shared-field-option.js?

@zphingphong zphingphong requested a review from ascott Jan 14, 2020
@ascott

This comment has been minimized.

Copy link
Contributor

ascott commented Jan 15, 2020

thanks for the change @zphingphong, i think this makes more sense! 👍

@ascott
ascott approved these changes Jan 15, 2020
@zphingphong zphingphong merged commit 3ff7bb0 into master Jan 16, 2020
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.