From b58e5ea3a2ed255b68508f32483f6f58ae46aa79 Mon Sep 17 00:00:00 2001 From: Reggie Riser Date: Wed, 18 Nov 2020 15:39:07 +0000 Subject: [PATCH 1/5] Make unit test for failing scenario --- pkg/handlers/primeapi/move_task_order_test.go | 133 ++++++++++++++++++ 1 file changed, 133 insertions(+) diff --git a/pkg/handlers/primeapi/move_task_order_test.go b/pkg/handlers/primeapi/move_task_order_test.go index 4621cf6cf4e..97d9883cd83 100644 --- a/pkg/handlers/primeapi/move_task_order_test.go +++ b/pkg/handlers/primeapi/move_task_order_test.go @@ -7,6 +7,8 @@ import ( "testing" "time" + "github.com/go-openapi/swag" + mtoserviceitem "github.com/transcom/mymove/pkg/services/mto_service_item" "github.com/transcom/mymove/pkg/gen/primemessages" @@ -262,6 +264,137 @@ func (suite *HandlerSuite) TestListMoveTaskOrdersHandlerReturnsUpdated() { suite.Equal(moveTaskOrder.ID.String(), moveTaskOrdersPayload[0].ID.String()) } +func (suite *HandlerSuite) makeAvailableMoveWithAddress(addressToSet models.Address) models.Move { + address := testdatagen.MakeAddress(suite.DB(), testdatagen.Assertions{ + Address: addressToSet, + }) + + newDutyStation := testdatagen.MakeDutyStation(suite.DB(), testdatagen.Assertions{ + DutyStation: models.DutyStation{ + AddressID: address.ID, + Address: address, + }, + }) + + moveOrder := testdatagen.MakeOrder(suite.DB(), testdatagen.Assertions{ + Order: models.Order{ + NewDutyStationID: newDutyStation.ID, + NewDutyStation: newDutyStation, + }, + }) + + move := testdatagen.MakeMove(suite.DB(), testdatagen.Assertions{ + Move: models.Move{ + AvailableToPrimeAt: swag.Time(time.Now()), + Status: models.MoveStatusAPPROVED, + }, + Order: moveOrder, + }) + + return move +} + +func (suite *HandlerSuite) equalAddress(expected models.Address, actual *primemessages.Address) { + suite.Equal(expected.ID.String(), actual.ID.String()) + suite.Equal(expected.StreetAddress1, *actual.StreetAddress1) + suite.Equal(*expected.StreetAddress2, *actual.StreetAddress2) + suite.Equal(*expected.StreetAddress3, *actual.StreetAddress3) + suite.Equal(expected.City, *actual.City) + suite.Equal(expected.State, *actual.State) + suite.Equal(expected.PostalCode, *actual.PostalCode) + suite.Equal(*expected.Country, *actual.Country) +} + +func (suite *HandlerSuite) equalPaymentRequest(expected models.PaymentRequest, actual *primemessages.PaymentRequest) { + suite.Equal(expected.ID.String(), actual.ID.String()) + suite.Equal(expected.MoveTaskOrderID.String(), actual.MoveTaskOrderID.String()) + suite.Equal(expected.IsFinal, *actual.IsFinal) + suite.Equal(expected.Status.String(), string(actual.Status)) + suite.Equal(expected.RejectionReason, actual.RejectionReason) + suite.Equal(expected.PaymentRequestNumber, actual.PaymentRequestNumber) +} + +func (suite *HandlerSuite) TestFetchMTOUpdatesHandlerLoopIteratorPointer() { + // Create two moves with different addresses. + move1 := suite.makeAvailableMoveWithAddress(models.Address{ + StreetAddress1: "1 First St", + StreetAddress2: swag.String("Apt 1"), + StreetAddress3: swag.String("Suite A"), + City: "Augusta", + State: "GA", + PostalCode: "30907", + Country: swag.String("US"), + }) + + move2 := suite.makeAvailableMoveWithAddress(models.Address{ + StreetAddress1: "2 Second St", + StreetAddress2: swag.String("Apt 2"), + StreetAddress3: swag.String("Suite B"), + City: "Columbia", + State: "SC", + PostalCode: "29212", + Country: swag.String("United States"), + }) + + // Create two payment requests on the second move. + paymentRequest1 := testdatagen.MakePaymentRequest(suite.DB(), testdatagen.Assertions{ + Move: move2, + PaymentRequest: models.PaymentRequest{ + IsFinal: false, + SequenceNumber: 1, + }, + }) + + paymentRequest2 := testdatagen.MakePaymentRequest(suite.DB(), testdatagen.Assertions{ + Move: move2, + PaymentRequest: models.PaymentRequest{ + IsFinal: true, + SequenceNumber: 2, + }, + }) + + // Setup and call the handler. + request := httptest.NewRequest("GET", "/move-task-orders", nil) + params := movetaskorderops.FetchMTOUpdatesParams{HTTPRequest: request} + context := handlers.NewHandlerContext(suite.DB(), suite.TestLogger()) + handler := FetchMTOUpdatesHandler{ + HandlerContext: context, + MoveTaskOrderFetcher: movetaskorder.NewMoveTaskOrderFetcher(suite.DB()), + } + response := handler.Handle(params) + + suite.IsNotErrResponse(response) + moveTaskOrdersResponse := response.(*movetaskorderops.FetchMTOUpdatesOK) + moveTaskOrdersPayload := moveTaskOrdersResponse.Payload + + // Check the addresses across the two moves. + // NOTE: The payload isn't ordered, so I have to associate the correct move. + suite.FatalFalse(len(moveTaskOrdersPayload) != 2) + move1Payload := moveTaskOrdersPayload[0] + move2Payload := moveTaskOrdersPayload[1] + if move1Payload.ID.String() != move1.ID.String() { + move1Payload = moveTaskOrdersPayload[1] + move2Payload = moveTaskOrdersPayload[0] + } + + suite.equalAddress(move1.Orders.NewDutyStation.Address, move1Payload.MoveOrder.DestinationDutyStation.Address) + suite.equalAddress(move2.Orders.NewDutyStation.Address, move2Payload.MoveOrder.DestinationDutyStation.Address) + + // Check the two payment requests across the second move. + // NOTE: The payload isn't ordered, so I have to associate the correct payment request. + paymentRequestsPayload := move2Payload.PaymentRequests + suite.FatalFalse(len(paymentRequestsPayload) != 2) + paymentRequest1Payload := paymentRequestsPayload[0] + paymentRequest2Payload := paymentRequestsPayload[1] + if paymentRequest1Payload.ID.String() != paymentRequest1.ID.String() { + paymentRequest1Payload = paymentRequestsPayload[1] + paymentRequest2Payload = paymentRequestsPayload[0] + } + + suite.equalPaymentRequest(paymentRequest1, paymentRequest1Payload) + suite.equalPaymentRequest(paymentRequest2, paymentRequest2Payload) +} + func (suite *HandlerSuite) TestUpdateMTOPostCounselingInfo() { mto := testdatagen.MakeAvailableMove(suite.DB()) From 61cd101bb93cbc97c80a9c228197efcf7885f925 Mon Sep 17 00:00:00 2001 From: Reggie Riser Date: Wed, 18 Nov 2020 19:43:30 +0000 Subject: [PATCH 2/5] Fixed the G601 issue by copying the struct before getting its address --- .../primeapi/payloads/model_to_payload.go | 28 +++++++++---------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/pkg/handlers/primeapi/payloads/model_to_payload.go b/pkg/handlers/primeapi/payloads/model_to_payload.go index d76977f34c2..172474f6656 100644 --- a/pkg/handlers/primeapi/payloads/model_to_payload.go +++ b/pkg/handlers/primeapi/payloads/model_to_payload.go @@ -53,8 +53,8 @@ func MoveTaskOrders(moveTaskOrders *models.Moves) []*primemessages.MoveTaskOrder payload := make(primemessages.MoveTaskOrders, len(*moveTaskOrders)) for i, m := range *moveTaskOrders { - // #nosec G601 TODO needs review - payload[i] = MoveTaskOrder(&m) + copyOfM := m // Make copy to avoid implicit memory aliasing of items from a range statement. + payload[i] = MoveTaskOrder(©OfM) } return payload } @@ -215,8 +215,8 @@ func MTOAgents(mtoAgents *models.MTOAgents) *primemessages.MTOAgents { agents := make(primemessages.MTOAgents, len(*mtoAgents)) for i, m := range *mtoAgents { - // #nosec G601 TODO needs review - agents[i] = MTOAgent(&m) + copyOfM := m // Make copy to avoid implicit memory aliasing of items from a range statement. + agents[i] = MTOAgent(©OfM) } return &agents @@ -250,8 +250,8 @@ func PaymentRequests(paymentRequests *models.PaymentRequests) *primemessages.Pay payload := make(primemessages.PaymentRequests, len(*paymentRequests)) for i, p := range *paymentRequests { - // #nosec G601 TODO needs review - payload[i] = PaymentRequest(&p) + copyOfP := p // Make copy to avoid implicit memory aliasing of items from a range statement. + payload[i] = PaymentRequest(©OfP) } return &payload } @@ -291,8 +291,8 @@ func PaymentServiceItems(paymentServiceItems *models.PaymentServiceItems) *prime payload := make(primemessages.PaymentServiceItems, len(*paymentServiceItems)) for i, p := range *paymentServiceItems { - // #nosec G601 TODO needs review - payload[i] = PaymentServiceItem(&p) + copyOfP := p // Make copy to avoid implicit memory aliasing of items from a range statement. + payload[i] = PaymentServiceItem(©OfP) } return &payload } @@ -323,8 +323,8 @@ func PaymentServiceItemParams(paymentServiceItemParams *models.PaymentServiceIte payload := make(primemessages.PaymentServiceItemParams, len(*paymentServiceItemParams)) for i, p := range *paymentServiceItemParams { - // #nosec G601 TODO needs review - payload[i] = PaymentServiceItemParam(&p) + copyOfP := p // Make copy to avoid implicit memory aliasing of items from a range statement. + payload[i] = PaymentServiceItemParam(©OfP) } return &payload } @@ -393,8 +393,8 @@ func MTOShipments(mtoShipments *models.MTOShipments) *primemessages.MTOShipments payload := make(primemessages.MTOShipments, len(*mtoShipments)) for i, m := range *mtoShipments { - // #nosec G601 TODO needs review - payload[i] = MTOShipment(&m) + copyOfM := m // Make copy to avoid implicit memory aliasing of items from a range statement. + payload[i] = MTOShipment(©OfM) } return &payload } @@ -475,8 +475,8 @@ func MTOServiceItems(mtoServiceItems *models.MTOServiceItems) *[]primemessages.M var payload []primemessages.MTOServiceItem for _, p := range *mtoServiceItems { - // #nosec G601 TODO needs review - payload = append(payload, MTOServiceItem(&p)) + copyOfP := p // Make copy to avoid implicit memory aliasing of items from a range statement. + payload = append(payload, MTOServiceItem(©OfP)) } return &payload } From b6136a202eb2164086adc9d32d0454779b1f9d8d Mon Sep 17 00:00:00 2001 From: Reggie Riser Date: Wed, 18 Nov 2020 20:12:38 +0000 Subject: [PATCH 3/5] Add move with unique address to test data to visualize issue --- pkg/testdatagen/scenario/devseed.go | 35 +++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/pkg/testdatagen/scenario/devseed.go b/pkg/testdatagen/scenario/devseed.go index 9551167cb34..b30c815990e 100644 --- a/pkg/testdatagen/scenario/devseed.go +++ b/pkg/testdatagen/scenario/devseed.go @@ -2214,4 +2214,39 @@ func (e devSeedScenario) Run(db *pop.Connection, userUploader *uploader.UserUplo Move: move8, PrimeUploader: primeUploader, }, 35) + + // Make an available to prime move with a unique address. + address := testdatagen.MakeAddress(db, testdatagen.Assertions{ + Address: models.Address{ + StreetAddress1: "2 Second St", + StreetAddress2: swag.String("Apt 2"), + StreetAddress3: swag.String("Suite B"), + City: "Columbia", + State: "SC", + PostalCode: "29212", + Country: swag.String("US"), + }, + }) + + newDutyStation := testdatagen.MakeDutyStation(db, testdatagen.Assertions{ + DutyStation: models.DutyStation{ + AddressID: address.ID, + Address: address, + }, + }) + + moveOrder := testdatagen.MakeOrder(db, testdatagen.Assertions{ + Order: models.Order{ + NewDutyStationID: newDutyStation.ID, + NewDutyStation: newDutyStation, + }, + }) + + testdatagen.MakeMove(db, testdatagen.Assertions{ + Move: models.Move{ + AvailableToPrimeAt: swag.Time(time.Now()), + Status: models.MoveStatusAPPROVED, + }, + Order: moveOrder, + }) } From f18cd60b271b83476780953041cfd6ce87882634 Mon Sep 17 00:00:00 2001 From: Reggie Riser Date: Wed, 18 Nov 2020 22:28:49 +0000 Subject: [PATCH 4/5] Added explicit UUID for new dev seed move --- pkg/testdatagen/scenario/devseed.go | 1 + 1 file changed, 1 insertion(+) diff --git a/pkg/testdatagen/scenario/devseed.go b/pkg/testdatagen/scenario/devseed.go index b30c815990e..d2101f41967 100644 --- a/pkg/testdatagen/scenario/devseed.go +++ b/pkg/testdatagen/scenario/devseed.go @@ -2244,6 +2244,7 @@ func (e devSeedScenario) Run(db *pop.Connection, userUploader *uploader.UserUplo testdatagen.MakeMove(db, testdatagen.Assertions{ Move: models.Move{ + ID: uuid.FromStringOrNil("ecbc2e6a-1b45-403b-9bd4-ea315d4d3d93"), AvailableToPrimeAt: swag.Time(time.Now()), Status: models.MoveStatusAPPROVED, }, From f9b263149630e08385b8010a8ae17d43f20bee02 Mon Sep 17 00:00:00 2001 From: Reggie Riser Date: Wed, 18 Nov 2020 22:49:00 +0000 Subject: [PATCH 5/5] Added back new move to dev seed data --- pkg/testdatagen/scenario/devseed.go | 40 +++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/pkg/testdatagen/scenario/devseed.go b/pkg/testdatagen/scenario/devseed.go index fa6747fe6eb..916aa0dd517 100644 --- a/pkg/testdatagen/scenario/devseed.go +++ b/pkg/testdatagen/scenario/devseed.go @@ -1617,6 +1617,43 @@ func createMoveWithBasicServiceItems(db *pop.Connection, userUploader *uploader. ) } +func createMoveWithUniqueDestinationAddress(db *pop.Connection) { + address := testdatagen.MakeAddress(db, testdatagen.Assertions{ + Address: models.Address{ + StreetAddress1: "2 Second St", + StreetAddress2: swag.String("Apt 2"), + StreetAddress3: swag.String("Suite B"), + City: "Columbia", + State: "SC", + PostalCode: "29212", + Country: swag.String("US"), + }, + }) + + newDutyStation := testdatagen.MakeDutyStation(db, testdatagen.Assertions{ + DutyStation: models.DutyStation{ + AddressID: address.ID, + Address: address, + }, + }) + + moveOrder := testdatagen.MakeOrder(db, testdatagen.Assertions{ + Order: models.Order{ + NewDutyStationID: newDutyStation.ID, + NewDutyStation: newDutyStation, + }, + }) + + testdatagen.MakeMove(db, testdatagen.Assertions{ + Move: models.Move{ + ID: uuid.FromStringOrNil("ecbc2e6a-1b45-403b-9bd4-ea315d4d3d93"), + AvailableToPrimeAt: swag.Time(time.Now()), + Status: models.MoveStatusAPPROVED, + }, + Order: moveOrder, + }) +} + // Run does that data load thing func (e devSeedScenario) Run(db *pop.Connection, userUploader *uploader.UserUploader, primeUploader *uploader.PrimeUploader, logger Logger, storer *storage.Filesystem) { // PPM Office Queue @@ -1655,4 +1692,7 @@ func (e devSeedScenario) Run(db *pop.Connection, userUploader *uploader.UserUplo // This move below is a PPM move in DRAFT status. It should probably // be changed to an HHG move in SUBMITTED status to reflect reality. createMoveWithBasicServiceItems(db, userUploader) + // Sets up a move with a non-default destination duty station address + // (to more easily spot issues with addresses being overwritten). + createMoveWithUniqueDestinationAddress(db) }