-
Notifications
You must be signed in to change notification settings - Fork 92
chore: remove v1 location endpoints from gateway #11129
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
base: develop
Are you sure you want to change the base?
Conversation
This comment has been minimized.
This comment has been minimized.
f475ef4 to
eb69f57
Compare
eb69f57 to
2ae48a1
Compare
makelicious
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
Let's add test cases so we know it works.
- Whatever we use externalId for
- data-seeder related changes.
|
|
||
| it('loads languages, facilities and locations on startup', async () => { | ||
| const loadFacilities = vi.spyOn(referenceApi, 'loadFacilities') | ||
| const loadContent = vi.spyOn(referenceApi, 'loadContent') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
content test might still have value?
| 'parentId', | ||
| 'validUntil', | ||
| 'locationType', | ||
| 'externalId' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test case needed for external id. what is the need?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Locations are read from csv file for seeding. id is generated during seeding. externalId is for referring to the csv row for each location.
When seeding user, their primary office is referred to as the externalId from the locations csv.
| }, | ||
| async primaryOffice(userModel: IUserModelData, _, { dataSources }) { | ||
| return dataSources.locationsAPI.getLocation(userModel.primaryOfficeId) | ||
| async primaryOffice(userModel: IUserModelData, _, { headers }) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have corresponding scopes? anything to check
| @@ -0,0 +1,7 @@ | |||
| -- Up Migration | |||
| ALTER TABLE locations ADD COLUMN external_id text UNIQUE; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What was the need, does administrative_areas need the same? We should have similar rows there
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
intentional?
| validUntil: null, | ||
| externalId: | ||
| location.externalId ?? | ||
| generateTrackingId(prng) + generateTrackingId(prng), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why plus?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Original externalIds are 11 characters long, TrackingIds are 6 characters.
| "build:clean": "rm -rf build", | ||
| "generate-db-types": "kanel && prettier src/storage/postgres/events/schema --write", | ||
| "generate-db-schema": "docker run --rm postgres:17.6 pg_dump postgres://events_migrator:[email protected]:5432/events -s > src/tests/postgres-migrations.sql --exclude-schema=analytics" | ||
| "generate-db-schema": "docker run --rm postgres:17.6 pg_dump postgres://events_migrator:migrator_password@$( [[ '$OSTYPE' == darwin* ]] && echo host.docker.internal || echo 172.17.0.1 ):5432/events -s > src/tests/postgres-migrations.sql --exclude-schema=analytics" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is the need?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
host.docker.internal does not work for linux.
| } | ||
| } | ||
|
|
||
| async function loadLocations(): Promise<ILocationDataResponse> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove? no reason to return empty.
… into ocrvs-11067
Description
Clearly describe what has been changed. Include relevant context or background.
Explain how the issue was fixed (if applicable) and the root cause.
Link this pull request to the GitHub issue (and optionally name the branch
ocrvs-<issue #>)Checklist