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

validate legal/search endpoint filters #6088

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from
Open

validate legal/search endpoint filters #6088

wants to merge 4 commits into from
+97 −38

Conversation

pkfec
Copy link
Contributor

@pkfec pkfec commented Dec 19, 2024 •
edited
Loading

Summary (required)

Required reviewers

2 developers

Impacted areas of the application

  • legal/search endpoints

How to test

on local:

  • git checkout feature/6024-legal-search-validate-dropdown-filters

  • pyenv activate

  • run elasticsearch service

  • run pytest

  • run python cli.py create_index ao_index

  • run python cli.py create_index arch_mur_index

  • run python cli.py create_index case_index

  • run python cli.py load_advisory_opinions 2023-01 (13 AO's upload to local ES service)

  • run python cli.py load_archived_murs

  • run python cli.py load_current_murs

  • run flask run

test filters

  1. type= (empty string):

Prod:

Local

  1. ao_requestor_type:

Prod:

Local

  1. primary_subject_id:

Prod:

Local

  1. secondary_subject_id:

Prod:

Local

@codecov Codecov
Copy link

codecov bot commented Dec 19, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.51%. Comparing base (f303b47) to head (769de77).
Report is 5 commits behind head on develop.

Additional details and impacted files 
@@             Coverage Diff             @@
##           develop    #6088      +/-   ##
===========================================
+ Coverage    88.48%   88.51%   +0.02%     
===========================================
  Files           82       82              
  Lines         9139     9156      +17     
===========================================
+ Hits          8087     8104      +17     
  Misses        1052     1052

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pkfec pkfec changed the title add validation to ao_requestor_type filter add validation to few dropdown filters Dec 19, 2024
@pkfec pkfec linked an issue Dec 19, 2024 that may be closed by this pull request
Make empty string behavior consistent on the legal search endpoint #6024
Open
1 task
@pkfec pkfec self-assigned this Dec 19, 2024
@pkfec pkfec changed the title add validation to few dropdown filters add validation to filters Dec 19, 2024
cnlucas
cnlucas reviewed Dec 20, 2024
View reviewed changes
Copy link
Member

@cnlucas cnlucas left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Great job dealing with a tricky issue, and thank you for cleaning up the code!

  1. My understanding was FE wants all of the filters to have the same behavior—should we confirm and open a follow-up ticket for the other filters? What about type, ao_is_pending, ao_citation_require_all, case_citation_require_all—can they be updated in this PR since they also have the option in swagger —?
  2. If someone includes an empty string and a string it throws a 500—should we update the check_filter_exists function to not throw a 500? IE http://localhost:5000/v1/legal/search/?type=advisory_opinions&ao_requestor_type=1&ao_requestor_type=

@fec-jli
Copy link
Contributor

fec-jli commented Dec 23, 2024

I agree with @cnlucas.
We should update the check_filter_exists function, something like if len(val) > 0 and '' not in val). Also the check_filter_exists function name is better like filter_validation(kwargs, filter)

@pkfec
Copy link
Contributor Author

pkfec commented Dec 27, 2024 •
edited
Loading

@cnlucas

  1. type: existing validation works well and throws 422 error incase an empty string or invalid string type is passed as a parameter which is why i didn't add any additional checks.
    Few examples:

ao_is_pending, ao_citation_require_all, case_citation_require_all: All three are boolean type filters
any value other than True/False, is already giving a 422 error. I don't think additional validation is required on these 3 filters
Few examples:

@pkfec
Copy link
Contributor Author

pkfec commented Dec 27, 2024 •
edited
Loading

@cnlucas

  1. ao_requestor_type filter is unique compared to other ones. I have added a custom validation check to return values even an invalid or empty string is passed on to this filter. Thanks for catching 500 error. I fixed the empty string validation error.

@pkfec pkfec force-pushed the feature/6024-legal-search-validate-dropdown-filters branch from da4fc87 to 2b3abbd Compare December 27, 2024 03:15
@pkfec pkfec requested a review from cnlucas December 27, 2024 03:31
@pkfec pkfec changed the title add validation to filters add validation to filters in legal/search endpoint Jan 6, 2025
@pkfec pkfec force-pushed the feature/6024-legal-search-validate-dropdown-filters branch 2 times, most recently from ff65d62 to a812c57 Compare January 8, 2025 20:56
@pkfec pkfec force-pushed the feature/6024-legal-search-validate-dropdown-filters branch 2 times, most recently from 1035699 to 9275a85 Compare January 15, 2025 00:22
@pkfec
Copy link
Contributor Author

pkfec commented Jan 15, 2025

After talking with the frontend, we agreed not to modify the existing BOOLEAN type filter validation on the backend for the following fields: ao_is_pending, ao_citation_require_all, and case_citation_require_all

@pkfec pkfec changed the title add validation to filters in legal/search endpoint [WIP]add validation to filters in legal/search endpoint Jan 15, 2025
@pkfec pkfec marked this pull request as draft January 15, 2025 19:02
@pkfec pkfec force-pushed the feature/6024-legal-search-validate-dropdown-filters branch from 9275a85 to 1290d3e Compare January 16, 2025 00:04
@pkfec pkfec changed the title [WIP]add validation to filters in legal/search endpoint add validation to filters in legal/search endpoint Jan 16, 2025
@pkfec pkfec force-pushed the feature/6024-legal-search-validate-dropdown-filters branch from 1290d3e to ac9817c Compare January 16, 2025 01:26
@pkfec pkfec marked this pull request as ready for review January 16, 2025 02:56
@pkfec pkfec changed the title add validation to filters in legal/search endpoint validate legal/search endpoint filters Jan 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fec-jli fec-jli Awaiting requested review from fec-jli

@tmpayton tmpayton Awaiting requested review from tmpayton

@cnlucas cnlucas Awaiting requested review from cnlucas

Assignees

@pkfec pkfec

Labels
Please review
Projects
Website project
Status: 👀 Ready
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Make empty string behavior consistent on the legal search endpoint
3 participants
@pkfec @fec-jli @cnlucas