-
Notifications
You must be signed in to change notification settings - Fork 35
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
MAIN B-21073 #13838
Open
danieljordan-caci
wants to merge
39
commits into
main
Choose a base branch
from
MAIN-B-21073
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
MAIN B-21073 #13838
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…how circleCI likes it
…the createAddress service object
danieljordan-caci
requested review from
a team,
deandreJones,
TevinAdams,
cameroncaci and
traskowskycaci
October 22, 2024 21:41
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
NOTE
I apologize in advance for this PR - it is quite a mess. There are three separate INT commits and I had to make commits only in INT due to some differences in payload/model conversions and then those were merged into
main
and had to handle dealing with those changes in here.The first INT PR also held changes for the Go upgrade, so... just do your best. If all tests are passing then all should be well, but it might be worth testing this locally just to be sure we are good to go.
INT PR 1
INT PR 2
INT PR 3
Agility ticket
Summary
For the last twenty years I have been working on this BL item and its beautiful personality and quirks. After fighting a timeout issue likely due to so many concurrency issues happening when building addresses (and also adding countries to the db), I updated the
db-truncate
file to NOT clear the test db ofre_countries
data so we can retain that and avoid unnecessary inserts into the test database when building test data.OKAY.
So the point of this BL item was to add a single source of truth for country information, which required a
re_countries
table with populated data.In addition to adding that data, we had to then refactor how we are handling country data in the backend and adding the
Country
object to the struct and then making that data accessible to the UI.This PR does the following:
re_countries
table and refactors theaddresses
table to use theid
column of there_countries
in the newcountry_id
column - drops thecountry
columnEager
/EagerPreload
instances to use"Addresses.Country"
now that theCountry
object holds the necessary dataAddress
definition for Swagger to only be two characters (we will handle this later on when international rolls out)US
countryHow to test
make server_run
and see the migrations runre_countries
table, as well as theaddresses
table having a removedcountry
column and addedcountry_id
columnCountry
object attached to the addressesCountry
is importantAgility ticket
Summary
This is adding a little cleanup + one missed oopsie of validating the country when the Prime attempts to create an address that contains a country code that is not supported.
Since we do not support international moves at this time, I added in a check that IF the Prime provides a country code that is not
US
, then we are going to send back an error message saying the country code they provided is not supported.How to test
/prime/v3/mto-shipments
Country
value of something that is NOTUS
- should get back a validation errorAgility ticket
Summary
Discovered that the service member profile creation/update flow does not use the expected service objects and instead uses a function in the models file. Updating that function to include
country_id
when saving the service member.How to test
addresses
table and ensure that thecountry_id
is populated