Feature/pdct 1440 implement multi geographies in the navigator backend #386

katybaulch · 2024-10-16T08:17:18Z

Description

Implement multiple navigators in the backend.

Clean up to come after validation on staging and the final update to the frontend to remove old references.

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:

How Has This Been Tested?

Please describe the tests that you added to verify your changes.

Reviewer Checklist

DB_CLIENT DEPENDENCY IS ON THE LATEST VERSION
The PR represents a single feature (small driveby 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

linear · 2024-10-16T08:17:21Z

PDCT-1440 Implement mulit-geographies in the navigator-backend

odrakes-cpr

I've not been deep in the backend as you so can't comment too much on functionality, but it all looks very sound otherwise :)

THOR300 · 2024-10-16T10:58:13Z

@katybaulch great to be getting this in!

Some questions as this is quite a big change:

To confirm where this sits against the following PR, my understanding is that this is us now passing in actual geographies not just a conversion of geography to an array of the geo like “fr’ -> [“fr”].
Re testing of this, have we spun up the local stack to test this before pushing to staging?
The main part I'm concerned about is the query changes for the creation of the pipeline input. As a minimum I would suggest we do the following to convince ourselves that the changes work as just looking at the code I don't feel like I can confirm fully that there will be no change in functionality other than the geographies field - I would suggest we spun up the stack locally with a staging dump, create the pipeline input (by invoking the endpoint) from the main built backend and compare this against the feature/pdct-14400... built backend.

Dismissing review until we've tested this locally post lunch.

katybaulch · 2024-10-16T11:05:56Z

@katybaulch great to be getting this in!

Some questions as this is quite a big change:

To confirm where this sits against the following PR, my understanding is that this is us now passing in actual geographies not just a conversion of geography to an array of the geo like “fr’ -> [“fr”].

Re testing of this, have we spun up the local stack to test this before pushing to staging?

The main part I'm concerned about is the query changes for the creation of the pipeline input. As a minimum I would suggest we do the following to convince ourselves that the changes work as just looking at the code I don't feel like I can confirm fully that there will be no change in functionality other than the geographies field - I would suggest we spun up the stack locally with a staging dump, create the pipeline input (by invoking the endpoint) from the main built backend and compare this against the feature/pdct-14400... built backend.

Just to clarify, we're now passing an array of geographies. Where there is only one geography, this will have the effect “fr" -> [“fr”]. Where there are multiple e.g., France and UK, it would pass ["fr", "uk"].

Sounds good. Let's catch up after lunch as I could really use a hand validating this!

THOR300 · 2024-10-16T12:56:25Z

@katybaulch great to be getting this in!
Some questions as this is quite a big change:

To confirm where this sits against the following PR, my understanding is that this is us now passing in actual geographies not just a conversion of geography to an array of the geo like “fr’ -> [“fr”].

Re testing of this, have we spun up the local stack to test this before pushing to staging?

The main part I'm concerned about is the query changes for the creation of the pipeline input. As a minimum I would suggest we do the following to convince ourselves that the changes work as just looking at the code I don't feel like I can confirm fully that there will be no change in functionality other than the geographies field - I would suggest we spun up the stack locally with a staging dump, create the pipeline input (by invoking the endpoint) from the main built backend and compare this against the feature/pdct-14400... built backend.

Just to clarify, we're now passing an array of geographies. Where there is only one geography, this will have the effect “fr" -> [“fr”]. Where there are multiple e.g., France and UK, it would pass ["fr", "uk"].

Sounds good. Let's catch up after lunch as I could really use a hand validating this!

Confirmed locally that there are some differences so iterating to fix before merge.

katybaulch · 2024-10-17T09:49:13Z

@katybaulch great to be getting this in!
Some questions as this is quite a big change:

To confirm where this sits against the following PR, my understanding is that this is us now passing in actual geographies not just a conversion of geography to an array of the geo like “fr’ -> [“fr”].

Re testing of this, have we spun up the local stack to test this before pushing to staging?

The main part I'm concerned about is the query changes for the creation of the pipeline input. As a minimum I would suggest we do the following to convince ourselves that the changes work as just looking at the code I don't feel like I can confirm fully that there will be no change in functionality other than the geographies field - I would suggest we spun up the stack locally with a staging dump, create the pipeline input (by invoking the endpoint) from the main built backend and compare this against the feature/pdct-14400... built backend.

Just to clarify, we're now passing an array of geographies. Where there is only one geography, this will have the effect “fr" -> [“fr”]. Where there are multiple e.g., France and UK, it would pass ["fr", "uk"].
Sounds good. Let's catch up after lunch as I could really use a hand validating this!

Confirmed locally that there are some differences so iterating to fix before merge.

I have removed the pipeline query updates from this PR so we can test in isolation on staging.

* Make pipeline query support multi geos * Make ordering of db objects the same * Update comment * Make sure query is more equivalent to incumbent * Create db_state_validator.py * Create way of validating two db state files * Revert "Create db_state_validator.py" This reverts commit a03f693. * Get most recent slug comments * Break formatting of db_state contents into separate function * Refactor generating db_state content into new function * Updating the script to use sdk models. (#389) Co-authored-by: Mark <[email protected]> * Add test cases for checking db_state content * Refactor write_documents_to_s3 for test isolation --------- Co-authored-by: NextGenEng <[email protected]> Co-authored-by: Mark <[email protected]>

katybaulch · 2024-10-17T16:34:34Z

@katybaulch great to be getting this in!
Some questions as this is quite a big change:

To confirm where this sits against the following PR, my understanding is that this is us now passing in actual geographies not just a conversion of geography to an array of the geo like “fr’ -> [“fr”].

Re testing of this, have we spun up the local stack to test this before pushing to staging?

The main part I'm concerned about is the query changes for the creation of the pipeline input. As a minimum I would suggest we do the following to convince ourselves that the changes work as just looking at the code I don't feel like I can confirm fully that there will be no change in functionality other than the geographies field - I would suggest we spun up the stack locally with a staging dump, create the pipeline input (by invoking the endpoint) from the main built backend and compare this against the feature/pdct-14400... built backend.

Just to clarify, we're now passing an array of geographies. Where there is only one geography, this will have the effect “fr" -> [“fr”]. Where there are multiple e.g., France and UK, it would pass ["fr", "uk"].
Sounds good. Let's catch up after lunch as I could really use a hand validating this!

Confirmed locally that there are some differences so iterating to fix before merge.

I have removed the pipeline query updates from this PR so we can test in isolation on staging.

This is the PR specifically for the pipeline changes with more comments from Mark on :) #388

olaughter

Mostly very minor comments from me below. Looking really good otherwise!

app/core/validation/util.py

app/repository/pipeline.py

app/repository/sql/pipeline.sql

tests/unit/app/core/test_pipeline.py

app/repository/sql/pipeline.sql

tests/non_search/core/validation/test_util.py

olaughter

lgtm!

katybaulch added 10 commits October 15, 2024 12:25

Update geo_subquery to support multiple geographies

799c877

Put code in fenced block

99f2745

Update DB client to 3.8.19

a8eb71d

Implement multiple geographies

7b074c8

Add multiple geo family to setup_helpers

2ef5d3a

Parametrize tests to test single and multi geo

0ff493d

Update comment with ticket number

859e0fe

Update docstrings with types

2c42141

Add ticket number to comment

3448e10

Bump to 1.18.1

db3a475

katybaulch added 2 commits October 16, 2024 10:46

Remove comment

2278a41

Split pipeline query into smaller queries for debug

1133ff1

katybaulch marked this pull request as ready for review October 16, 2024 10:10

katybaulch requested review from a team as code owners October 16, 2024 10:10

katybaulch added 2 commits October 16, 2024 11:22

Add missing param and return type in docstring

7012e0b

Fix line length on docstring

7a62351

odrakes-cpr previously approved these changes Oct 16, 2024

View reviewed changes

katybaulch added 2 commits October 16, 2024 13:58

Update stop & rm

c3d5ed3

Revert changes to pipeline query

acbf4d4

katybaulch and others added 2 commits October 17, 2024 17:31

Bump to 1.19.0

79ab35f

katybaulch added 4 commits October 21, 2024 12:37

Update CODEOWNERS

5092a59

Remove unused code

1f32317

Move pipeline.sql uner sql folder

0259773

Use raw SQL to execute pipeline query

6bb0541

katybaulch requested a review from olaughter October 21, 2024 12:31

katybaulch added 4 commits October 21, 2024 13:47

Add data_client as dependency to mock get_db

a87db82

Move pipeline tests under non_search

9117af5

Create test_flatten_pipeline_metadata.py

0de5a2f

Move file

8174a11

olaughter reviewed Oct 21, 2024

View reviewed changes

katybaulch added 13 commits October 21, 2024 16:35

Driveby: Use os for path reference

5ba9520

Add missing type hint

10df6d5

Remove title casing

fdcc0db

Add return type hint

586ebad

Add missing param in docstring

d39b65b

PR comment fixes

3beeff5

Fix languages

5fb4165

Use attribute accessor for series

62e3c64

Move file

b59f6b7

Format family category as its always CAPS in db

fe1fcbc

Format query

84f8636

Make casing consistent

b4e0c2d

Make published_date timezone aware

55dac51

katybaulch requested review from olaughter and diversemix October 21, 2024 17:25

olaughter reviewed Oct 22, 2024

View reviewed changes

app/repository/sql/pipeline.sql Show resolved Hide resolved

olaughter reviewed Oct 22, 2024

View reviewed changes

tests/non_search/core/validation/test_util.py Show resolved Hide resolved

olaughter approved these changes Oct 22, 2024

View reviewed changes

katybaulch merged commit 038163e into main Oct 22, 2024
12 checks passed

katybaulch deleted the feature/pdct-1440-implement-mulit-geographies-in-the-navigator-backend branch October 22, 2024 08:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/pdct 1440 implement multi geographies in the navigator backend #386

Feature/pdct 1440 implement multi geographies in the navigator backend #386

katybaulch commented Oct 16, 2024 •

edited

Loading

linear bot commented Oct 16, 2024

odrakes-cpr left a comment

THOR300 commented Oct 16, 2024 •

edited

Loading

katybaulch commented Oct 16, 2024

THOR300 commented Oct 16, 2024

katybaulch commented Oct 17, 2024

katybaulch commented Oct 17, 2024

olaughter left a comment

olaughter left a comment

Feature/pdct 1440 implement multi geographies in the navigator backend #386

Feature/pdct 1440 implement multi geographies in the navigator backend #386

Conversation

katybaulch commented Oct 16, 2024 • edited Loading

Description

Proposed version

Type of change

How Has This Been Tested?

Reviewer Checklist

linear bot commented Oct 16, 2024

odrakes-cpr left a comment

Choose a reason for hiding this comment

THOR300 commented Oct 16, 2024 • edited Loading

katybaulch commented Oct 16, 2024

THOR300 commented Oct 16, 2024

katybaulch commented Oct 17, 2024

katybaulch commented Oct 17, 2024

olaughter left a comment

Choose a reason for hiding this comment

olaughter left a comment

Choose a reason for hiding this comment

katybaulch commented Oct 16, 2024 •

edited

Loading

THOR300 commented Oct 16, 2024 •

edited

Loading