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

Add multi geographies to search API #266

Merged
merged 3 commits into from
Dec 16, 2024
Merged

Add multi geographies to search API #266

merged 3 commits into from
Dec 16, 2024

Conversation

jamesgorrie
Copy link
Contributor

@jamesgorrie jamesgorrie commented Dec 9, 2024

Description

Part of https://linear.app/climate-policy-radar/issue/PDCT-1404/admin-service-support-multiple-geographies

Allows the API to receive geography as an list i.e. geography=zambia&geography=zimbabwe.

I've used the built in parsers for the geography qs - which breaks convention, but avoids us having to add new parsers to get_query_params_as_dict.

As a secondary refactor to this PR - we could lean heavier into that and be more explicit about out URL params.

Proposed version

  • Skip auto-tagging
  • Patch
  • Minor version
  • Major version

Type of change

  • New feature

How Has This Been Tested?

Integration tests

@jamesgorrie jamesgorrie requested a review from a team as a code owner December 9, 2024 15:48
Copy link

linear bot commented Dec 9, 2024

@jamesgorrie jamesgorrie marked this pull request as draft December 9, 2024 15:48
@jamesgorrie jamesgorrie force-pushed the multi_georgraphies branch 3 times, most recently from 0889ae6 to 31dc0dd Compare December 9, 2024 17:10
@jamesgorrie jamesgorrie changed the title feat: add multi geographies test Add multi geographies to API Dec 9, 2024
@jamesgorrie jamesgorrie marked this pull request as ready for review December 9, 2024 17:18
@jamesgorrie jamesgorrie changed the title Add multi geographies to API Add multi geographies to search API Dec 10, 2024
Copy link
Contributor Author

@jamesgorrie jamesgorrie left a comment

Choose a reason for hiding this comment

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

A couple notes to hopefully make the review easier.

tests/integration_tests/setup_db.py Show resolved Hide resolved
app/repository/family.py Show resolved Hide resolved
Copy link
Contributor

@katybaulch katybaulch left a comment

Choose a reason for hiding this comment

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

This adds the ability to search multiple geographies via the API, but doesn't add support for multi geographies in the admin backend (slight misinterpretation of the ticket perhaps - but this work still needed doing so I'm going to link the right ticket and merge this) :)

https://linear.app/climate-policy-radar/issue/PDCT-1775/make-geography-filter-on-family-list-page-support-multi-geos

@katybaulch katybaulch enabled auto-merge (squash) December 16, 2024 12:16
@katybaulch katybaulch merged commit c8df79c into main Dec 16, 2024
7 checks passed
@katybaulch katybaulch deleted the multi_georgraphies branch December 16, 2024 12:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants