Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Feature/pdct 1780 support multiple geos in update #278

Merged
merged 23 commits into from
Jan 9, 2025

Conversation

odrakes-cpr
Copy link
Contributor

Description

Update the update endpoint to allow for the editing of multiple geographies.

  • Added a separate check to the update intention which determines if the list of geo ids from the iso codes in the payload is diff from the associted family geo ids.
  • Added a validation step to the service layer that checks if the iso code in the payload exists in the db

Proposed version

Please select the option below that is most relevant from the list below. This
will be used to generate the next tag version name during auto-tagging.

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

Visit the Semver website to understand the
difference between MAJOR, MINOR, and PATCH versions.

Notes:

  • If none of these options are selected, auto-tagging will fail
  • Where multiple options are selected, the most senior option ticked will be
    used -- e.g. Major > Minor > Patch
  • If you are selecting the version in the list above using the textbox, make
    sure your selected option is marked [x] with no spaces in between the
    brackets and the x

Type of change

Please select the option(s) below that are most relevant:

  • Bug fix
  • New feature
  • Breaking change
  • GitHub workflow update
  • Documentation update
  • Remove legacy code
  • Dependency update

How Has This Been Tested?

Updated the integration tests and the service / repository layer tests where appropriate

Reviewer Checklist

  • DB_CLIENT DEPENDENCY IS ON THE LATEST VERSION
  • The PR represents a single feature (small drive-by fixes are also ok)
  • The PR includes tests that are sufficient for the level of risk
  • The code is sufficiently commented, particularly in hard-to-understand areas
  • Any required documentation updates have been made
  • Any TODOs added are captured in future tickets
  • No FIXMEs remain

Osneil Drakes added 8 commits January 7, 2025 13:39
- function that retrieves the list of geo ids from the geo repo, based
  on the iso codes thats provided in the payload. It validates these iso
  codes at the same time returning an error if the geography does not
  exist in the db
this will currently have a default value of none, so that it works with front end requests until the payload data is also updated
@odrakes-cpr odrakes-cpr requested a review from a team as a code owner January 8, 2025 11:01
Copy link

linear bot commented Jan 8, 2025

app/repository/family.py Outdated Show resolved Hide resolved
pyproject.toml Outdated 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.

Nice work! Couple of comments 💬

Osneil Drakes added 6 commits January 8, 2025 11:24
…oes not break any requests made by the front before multi geos is implemented
if the array of geo ids coming through is empty, i.e it was not passed int he payload as expected (the frontend hasnt been updted for multi geos we do nowt and carry on
@odrakes-cpr odrakes-cpr requested a review from katybaulch January 8, 2025 12:29
app/model/family.py Outdated Show resolved Hide resolved
app/repository/family.py Outdated Show resolved Hide resolved
app/repository/family.py Outdated Show resolved Hide resolved
app/repository/family.py Outdated Show resolved Hide resolved
@odrakes-cpr odrakes-cpr requested a review from katybaulch January 8, 2025 16:53
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.

Missing a RepoError raise for geography add but otherwise super great, well done!!!! 🦖

app/repository/family.py Show resolved Hide resolved
app/repository/family.py Outdated 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.

very nice!!!!

@odrakes-cpr odrakes-cpr merged commit 47c9067 into main Jan 9, 2025
10 checks passed
@odrakes-cpr odrakes-cpr deleted the feature/pdct-1780-support-multiple-geos-in-update branch January 9, 2025 13:03
@jamesgorrie jamesgorrie restored the feature/pdct-1780-support-multiple-geos-in-update branch February 6, 2025 15:43
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants