B 21442 int #13872

begrohmann · 2024-10-10T15:48:48Z

Summary

The purpose of this BL is to add AK locations to us_post_region_cities table. In doing so, we also created several parent tables with city, state and zip data with associated ids to us_post_region_cities and addresses tables:

re_states
re_us_post_regions
re_cities

We created the following tables for OCONUS rate area associations and transit times:

re_oconus_rate_areas
re_intl_transit_times

We created a new view v_locations to query by city, state, zip, country and/or country and see all associated ids.

Verification Steps for the Author

These are to be checked by the author.

Tested in the Experimental environment (for changes to containers, app startup, or connection to data stores)
Have the Agility acceptance criteria been met for this change?

Verification Steps for Reviewers

These are to be checked by a reviewer.

Has the branch been pulled in and checked out?
Have the BL acceptance criteria been met for this change?
Was the CircleCI build successful?
Has the code been reviewed from a standards and best practices point of view?

How to test

Database

Open the database.
Check that the following tables exist with data:

re_states
re_cities
re_us_post_regions
re_oconus_rate_areas
re_intl_transit_times

Check that us_post_region_cities table has the following new columns added and populated with data:

us_post_regions_id
cities_id

Check that addresses table has the us_post_region_cities_id column added. We have populated most of the associated data but there is some fallout where there is no county, the city does not match what is in re_cities or the combination of the data does not match up with us_post_region_cities. We will handle this in an upcoming BL when FK constraints are added. Data cleanup will be case by case.

Check that the v_locations view was created and you can query data.

Any new migrations/schema changes:

Follows our guidelines for Zero-Downtime Deploys.
Have been communicated to #g-database.
Secure migrations have been tested following the instructions in our docs.

Screenshots

…nscom/mymove into B-21442

…ate re_cities insert

danieljordan-caci · 2024-10-23T15:08:02Z

pkg/models/re_international_transit_time.go

+}
+
+func (ReInternationalTransitTime) TableName() string {
+	return "re_international_transit_times"


This table name is incorrect - the migration is re_intl_transit_times

Please change the file & struct name as well to match

Changed filename to be plural and removed the Re from the struct name.

danieljordan-caci · 2024-10-23T15:10:07Z

pkg/models/service_member.go

+			if serviceMember.BackupMailingAddress.Country != nil {
+				country := serviceMember.BackupMailingAddress.Country
+				if country.Country != "US" || country.Country == "US" && serviceMember.BackupMailingAddress.State == "AK" || country.Country == "US" && serviceMember.BackupMailingAddress.State == "HI" {
+					boolTrueVal := true
+					serviceMember.BackupMailingAddress.IsOconus = &boolTrueVal
+				} else {
+					boolFalseVal := false
+					serviceMember.BackupMailingAddress.IsOconus = &boolFalseVal
+				}
+			} else if serviceMember.BackupMailingAddress.CountryId != nil {
+				country, err := FetchCountryByID(appCtx.DB(), *serviceMember.BackupMailingAddress.CountryId)
+				if err != nil {
+					return err
+				}
+				if country.Country != "US" || country.Country == "US" && serviceMember.BackupMailingAddress.State == "AK" || country.Country == "US" && serviceMember.BackupMailingAddress.State == "HI" {
+					boolTrueVal := true
+					serviceMember.BackupMailingAddress.IsOconus = &boolTrueVal
+				} else {
+					boolFalseVal := false
+					serviceMember.BackupMailingAddress.IsOconus = &boolFalseVal
+				}
+			} else {
+				boolFalseVal := false
+				serviceMember.BackupMailingAddress.IsOconus = &boolFalseVal
+			}


Wondering why this is not in the code... It was def merged in this PR

Looking at the history in INT, looks like this commit added it but in the next commit its not there.

@danieljordan-caci Can you verify this is in the correct spot in the code since git placed it here.

Actually this should be removed. The same check gets done at the line above:

// Evaluate address and populate addresses isOconus value isOconus, err := IsAddressOconus(appCtx.DB(), *serviceMember.BackupMailingAddress) if err != nil { responseError = err return err } serviceMember.BackupMailingAddress.IsOconus = &isOconus

danieljordan-caci · 2024-10-23T15:12:41Z

pkg/services/address/address_updater.go

+	// use the data we have first, if it's not nil
+	if mergedAddress.State != originalAddress.State && mergedAddress.Country != nil {
+		country := mergedAddress.Country
+		if country.Country != "US" || country.Country == "US" && mergedAddress.State == "AK" || country.Country == "US" && mergedAddress.State == "HI" {
+			boolTrueVal := true
+			mergedAddress.IsOconus = &boolTrueVal
+		} else {
+			boolFalseVal := false
+			mergedAddress.IsOconus = &boolFalseVal
+		}
+	} else if mergedAddress.State != originalAddress.State && mergedAddress.CountryId != nil {
+		country, err := models.FetchCountryByID(appCtx.DB(), *mergedAddress.CountryId)
+		if err != nil {
+			return nil, err
+		}
+		if country.Country != "US" || country.Country == "US" && mergedAddress.State == "AK" || country.Country == "US" && mergedAddress.State == "HI" {
+			boolTrueVal := true
+			mergedAddress.IsOconus = &boolTrueVal
+		} else {
+			boolFalseVal := false
+			mergedAddress.IsOconus = &boolFalseVal
+		}
+	}
+


This shouldn't be in here - please remove. The line above it does the exact same check.

// Evaluate address and populate addresses isOconus value isOconus, err := models.IsAddressOconus(appCtx.DB(), mergedAddress) if err != nil { return nil, err } mergedAddress.IsOconus = &isOconus

This was pulled in from your commit
😄
Glad your reviewing these. Updating it now.

danieljordan-caci · 2024-10-23T15:13:38Z

pkg/services/address/address_creator.go

@@ -57,6 +57,7 @@ func (f *addressCreator) CreateAddress(appCtx appcontext.AppContext, address *mo
 		}
 		transformedAddress.Country = &country
 		transformedAddress.CountryId = &country.ID
+		transformedAddress.Country = &country


Remove this line - it's a duplicate

thanks git lol

removed the duplicate, this was from a git merge so its only in the INT branch and not on the main.

danieljordan-caci · 2024-10-23T15:14:35Z

pkg/services/address/address_creator_test.go

@@ -71,6 +72,21 @@ func (suite *AddressSuite) TestAddressCreator() {
 		suite.Equal("- the country GB is not supported at this time - only US is allowed", err.Error())
 	})

+	suite.Run("Transforms Country to nil when no country name is specified", func() {


The title of this test is incorrect - you're testing that if a nil country is provided and checking that an error is returned. Please change the title to be more descriptive to what's being tested.

These weren't changes that we made, assuming they came from your branch since it states Country. I'll go change it here.

Yeah some funky stuff goin' on

Updated the description

MAIN B-21073

…B-21442-INT

danieljordan-caci · 2024-10-23T20:35:38Z

pkg/models/re_international_transit_times.go

+}
+
+func (InternationalTransitTime) TableName() string {
+	return "re_international_transit_times"


This table name is still wrong. It's re_intl_transit_times in your migration

It wasn't at one point 😄 . I fixed it, good catch

Had discussions on validators and since we made the models read only validators aren't needed. The only test to be added just returns a string and isn't covered in the test coverage. We will add more tests as we start transferring addresses model over to using these new tables.

deandreJones and others added 30 commits October 2, 2024 09:02

add location objects

9faad2e

not yet

18128ab

countries

08ab824

states

6555f17

populate states and postRegions

7621003

remove the trash

38bdb35

missed a test

3ab77d3

adding new tables, updating data

4426030

changes- corrections...

2346654

Merge remote-tracking branch 'origin/MAIN-B-21073' into B-21435

7befb77

remove comment

254326f

updated cities, remove re_countries constraint

598b658

transit time and oconus rate areas

2a26df1

updated cities list, adding us_post_regions_cities_id to addresses

0db6f83

Merge branch 'main' of github.com:transcom/mymove into B-21442

05daba9

Merge branch 'main' of github.com:transcom/mymove into B-21442

4895481

comment out update post region id for now

b323078

Merge branch 'B-21163-transportation-offices-AK-HI' of github.com:tra…

fa6abe9

…nscom/mymove into B-21442

B-21435 missing files

57b55a3

reorder manifest files, use copy instead of insert

7eef1c9

moving up AK HI transpo file

7d63290

changing filenames

11bd918

remove drop column of preferred city

9359776

updates for rate areas, transit times tables

4fd95cf

adding models

b7fb4b0

add AK HI before update

b7ffe59

B-21442 - add create re_oconus_rate_areas, re_intl_transit_times, upd…

bd1c72c

…ate re_cities insert

B-21442 insert us_post_region_cities

79b662c

Update 20241007184230_insertReOconusRateAreas.up.sql

edc5998

Updates

3e34f40

danieljordan-caci requested changes Oct 23, 2024

View reviewed changes

danieljordan-caci reviewed Oct 23, 2024

View reviewed changes

r-mettler and others added 13 commits October 23, 2024 10:56

removing duplicate from merge

9a796c4

code cleanup

527df61

Merge pull request #13838 from transcom/MAIN-B-21073

b2b1ebb

MAIN B-21073

Merge branch 'main' of github.com:transcom/mymove into B-21442

17f1c01

Merge branch 'main' of github.com:transcom/mymove into B-21442

5322850

removing unneeded check

aaa1230

Merge branch 'B-21442' of github.com:transcom/mymove into B-21442-INT

4f84556

Merge branch 'integrationTesting' of github.com:transcom/mymove into …

870e44a

…B-21442-INT

removing Re from structs

7c2903e

Merge branch 'integrationTesting' of github.com:transcom/mymove into …

35bd0e0

…B-21442-INT

Merge branch 'B-21442' of github.com:transcom/mymove into B-21442-INT

24f4053

making active not null

535913a

Merge branch 'B-21442' of github.com:transcom/mymove into B-21442-INT

8462f52

r-mettler requested a review from danieljordan-caci October 23, 2024 20:10

deandreJones approved these changes Oct 23, 2024

View reviewed changes

danieljordan-caci requested changes Oct 23, 2024

View reviewed changes

r-mettler added 2 commits October 23, 2024 22:29

updating table name

19bd3e0

Merge branch 'B-21442' of github.com:transcom/mymove into B-21442-INT

11b0526

danieljordan-caci approved these changes Oct 24, 2024

View reviewed changes

Merge branch 'integrationTesting' into B-21442-INT

7b48cb8

Merge branch 'integrationTesting' into B-21442-INT

b233d42

r-mettler merged commit 7456470 into integrationTesting Oct 24, 2024
28 of 29 checks passed

r-mettler deleted the B-21442-INT branch October 24, 2024 15:11

r-mettler mentioned this pull request Oct 24, 2024

B 21442 #14005

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

B 21442 int #13872

B 21442 int #13872

begrohmann commented Oct 10, 2024

danieljordan-caci Oct 23, 2024

r-mettler Oct 23, 2024

danieljordan-caci Oct 23, 2024

r-mettler Oct 23, 2024 •

edited

Loading

r-mettler Oct 23, 2024

danieljordan-caci Oct 23, 2024

r-mettler Oct 23, 2024

danieljordan-caci Oct 23, 2024 •

edited

Loading

r-mettler Oct 23, 2024

r-mettler Oct 23, 2024

danieljordan-caci Oct 23, 2024

r-mettler Oct 23, 2024

r-mettler Oct 23, 2024

danieljordan-caci Oct 23, 2024

r-mettler Oct 23, 2024

danieljordan-caci Oct 23, 2024

r-mettler Oct 23, 2024

danieljordan-caci Oct 23, 2024

r-mettler Oct 23, 2024

B 21442 int #13872

B 21442 int #13872

Conversation

begrohmann commented Oct 10, 2024

Summary

Verification Steps for the Author

Verification Steps for Reviewers

How to test

Database

Any new migrations/schema changes:

Screenshots

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

r-mettler Oct 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danieljordan-caci Oct 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

r-mettler Oct 23, 2024 •

edited

Loading

danieljordan-caci Oct 23, 2024 •

edited

Loading