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.

Sign up for GitHub

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

Jump to bottom

Update addFilter to take a map of filter type to field name #257

Merged
merged 7 commits into from
Jul 23, 2021
Merged

Update addFilter to take a map of filter type to field name #257

merged 7 commits into from
Jul 23, 2021

Conversation

avaleske
Copy link

@avaleske avaleske commented Jul 23, 2021 •
edited

Update addFilter to take a map of filter type to field name

Addresses #202

  • This change addresses the issue in full
  • This change addresses only certain aspects of the issue
  • This change is a dependency for another issue
  • This change has a dependency from another issue

Description

  • Replaces entity param in addFilter() with a map from filter name to field name, so we can filter on things that are in joined tables.
  • Fixes a bug where the same value was used for all WHERE clauses for filters of the same type
  • Makes all filterType comparisons and string matching case-insensitive
  • Using a map lets us enforce which filter types are allowed, and throw when an unsupported filter is used

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Prototype/POC (not to merge)
  • This change is a refactor/address technical debt
  • This change requires a documentation update
  • This change requires a SQL Script

How Can This Be Tested/Reviewed?

Test with paths like

  • /listings?filter[$comparison]=<>&filter[nEIGhborhood]=Foster%20cIty (1 result)
  • http://localhost:3100/listings?filter[$comparison]==&filter[name]=Test:%20Triton&filter[$comparison]=<>&filter[neighborhood]=Coliseum (1 result)
  • /listings?filter[$comparison]=<>&filter[neighborhood]=Coliseum&filter[$comparison]==&filter[neighborhood]=Foster%20City (1 result)

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have reviewed the changes in a desktop view
  • I have reviewed the changes in a mobile view
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules
  • I have assigned reviewers
  • I have updated the changelog to include a description of my changes

@avaleske avaleske linked an issue Jul 23, 2021 that may be closed by this pull request
Make addFilter generic so we can contribute upstream #202
Closed
@avaleske avaleske added the epic: filtering Tickets related to Filtering label Jul 23, 2021
@avaleske avaleske added this to the M8 milestone Jul 23, 2021
@avaleske avaleske requested a review from abbiefarr July 23, 2021 18:09
abbiefarr
abbiefarr approved these changes Jul 23, 2021
View reviewed changes
Copy link

@abbiefarr abbiefarr left a comment

Choose a reason for hiding this comment

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

This is great, thanks!

@avaleske avaleske merged commit 55a049e into main Jul 23, 2021
@avaleske avaleske mentioned this pull request Jul 26, 2021
Merged
27 tasks
seanmalbert pushed a commit that referenced this pull request Jun 23, 2022
@avaleske
Update addFilter to take a map of filter type to field name (#257)
c2581cc 
* initial refactor of filter logic into provided function

* replace generic function with map, rename filter type field

* fix comparison slicing

* make filterType check and where clause case-insensitive

* fix tests and add test for unimplemented filter

* reorder imports so there's not an unnecessary diff
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@abbiefarr abbiefarr abbiefarr approved these changes

Assignees
No one assigned
Labels
epic: filtering Tickets related to Filtering
Projects
None yet
Milestone
M8
Development

Successfully merging this pull request may close these issues.

Make addFilter generic so we can contribute upstream
2 participants
@avaleske @abbiefarr