diff --git a/pkg/handlers/internalapi/moving_expense.go b/pkg/handlers/internalapi/moving_expense.go index 3511feb46b4..aea7b29f4ab 100644 --- a/pkg/handlers/internalapi/moving_expense.go +++ b/pkg/handlers/internalapi/moving_expense.go @@ -9,11 +9,9 @@ import ( "github.com/transcom/mymove/pkg/appcontext" "github.com/transcom/mymove/pkg/apperror" - "github.com/transcom/mymove/pkg/db/utilities" movingexpenseops "github.com/transcom/mymove/pkg/gen/internalapi/internaloperations/ppm" "github.com/transcom/mymove/pkg/handlers" "github.com/transcom/mymove/pkg/handlers/internalapi/internal/payloads" - "github.com/transcom/mymove/pkg/models" "github.com/transcom/mymove/pkg/services" ) @@ -186,39 +184,10 @@ func (h DeleteMovingExpenseHandler) Handle(params movingexpenseops.DeleteMovingE // Make sure the service member is not modifying another service member's PPM ppmID := uuid.FromStringOrNil(params.PpmShipmentID.String()) - var ppmShipment models.PPMShipment - err := appCtx.DB().Scope(utilities.ExcludeDeletedScope()). - EagerPreload( - "Shipment.MoveTaskOrder.Orders", - "MovingExpenses", - ). - Find(&ppmShipment, ppmID) - if err != nil { - if err == sql.ErrNoRows { - return movingexpenseops.NewDeleteMovingExpenseNotFound(), err - } - return movingexpenseops.NewDeleteMovingExpenseInternalServerError(), err - } - if ppmShipment.Shipment.MoveTaskOrder.Orders.ServiceMemberID != appCtx.Session().ServiceMemberID { - wrongServiceMemberIDErr := apperror.NewSessionError("Attempted delete by wrong service member") - appCtx.Logger().Error("internalapi.DeleteMovingExpenseHandler", zap.Error(wrongServiceMemberIDErr)) - return movingexpenseops.NewDeleteMovingExpenseForbidden(), wrongServiceMemberIDErr - } movingExpenseID := uuid.FromStringOrNil(params.MovingExpenseID.String()) - found := false - for _, lineItem := range ppmShipment.MovingExpenses { - if lineItem.ID == movingExpenseID { - found = true - break - } - } - if !found { - mismatchedPPMShipmentAndMovingExpenseIDErr := apperror.NewSessionError("Moving expense does not exist on ppm shipment") - appCtx.Logger().Error("internalapi.DeleteMovingExpenseHandler", zap.Error(mismatchedPPMShipmentAndMovingExpenseIDErr)) - return movingexpenseops.NewDeleteMovingExpenseNotFound(), mismatchedPPMShipmentAndMovingExpenseIDErr - } - err = h.movingExpenseDeleter.DeleteMovingExpense(appCtx, movingExpenseID) + + err := h.movingExpenseDeleter.DeleteMovingExpense(appCtx, ppmID, movingExpenseID) if err != nil { appCtx.Logger().Error("internalapi.DeleteMovingExpenseHandler", zap.Error(err)) diff --git a/pkg/handlers/internalapi/moving_expense_test.go b/pkg/handlers/internalapi/moving_expense_test.go index e8002c158f5..640eb5f0249 100644 --- a/pkg/handlers/internalapi/moving_expense_test.go +++ b/pkg/handlers/internalapi/moving_expense_test.go @@ -425,6 +425,7 @@ func (suite *HandlerSuite) TestDeleteMovingExpenseHandler() { mockDeleter.On("DeleteMovingExpense", mock.AnythingOfType("*appcontext.appContext"), mock.AnythingOfType("uuid.UUID"), + mock.AnythingOfType("uuid.UUID"), ).Return(err) // Use createS3HandlerConfig for the HandlerConfig because we are required to upload a doc diff --git a/pkg/services/mocks/MovingExpenseDeleter.go b/pkg/services/mocks/MovingExpenseDeleter.go index 43aa2e3e321..c1d0bb16fb4 100644 --- a/pkg/services/mocks/MovingExpenseDeleter.go +++ b/pkg/services/mocks/MovingExpenseDeleter.go @@ -14,13 +14,13 @@ type MovingExpenseDeleter struct { mock.Mock } -// DeleteMovingExpense provides a mock function with given fields: appCtx, movingExpenseID -func (_m *MovingExpenseDeleter) DeleteMovingExpense(appCtx appcontext.AppContext, movingExpenseID uuid.UUID) error { - ret := _m.Called(appCtx, movingExpenseID) +// DeleteMovingExpense provides a mock function with given fields: appCtx, ppmID, movingExpenseID +func (_m *MovingExpenseDeleter) DeleteMovingExpense(appCtx appcontext.AppContext, ppmID uuid.UUID, movingExpenseID uuid.UUID) error { + ret := _m.Called(appCtx, ppmID, movingExpenseID) var r0 error - if rf, ok := ret.Get(0).(func(appcontext.AppContext, uuid.UUID) error); ok { - r0 = rf(appCtx, movingExpenseID) + if rf, ok := ret.Get(0).(func(appcontext.AppContext, uuid.UUID, uuid.UUID) error); ok { + r0 = rf(appCtx, ppmID, movingExpenseID) } else { r0 = ret.Error(0) } diff --git a/pkg/services/moving_expense.go b/pkg/services/moving_expense.go index 75a97275e6d..ffc897b26a1 100644 --- a/pkg/services/moving_expense.go +++ b/pkg/services/moving_expense.go @@ -25,5 +25,5 @@ type MovingExpenseUpdater interface { // //go:generate mockery --name MovingExpenseDeleter type MovingExpenseDeleter interface { - DeleteMovingExpense(appCtx appcontext.AppContext, movingExpenseID uuid.UUID) error + DeleteMovingExpense(appCtx appcontext.AppContext, ppmID uuid.UUID, movingExpenseID uuid.UUID) error } diff --git a/pkg/services/moving_expense/moving_expense_deleter.go b/pkg/services/moving_expense/moving_expense_deleter.go index 106c16b41ca..2205b13828f 100644 --- a/pkg/services/moving_expense/moving_expense_deleter.go +++ b/pkg/services/moving_expense/moving_expense_deleter.go @@ -1,10 +1,15 @@ package movingexpense import ( + "database/sql" + "github.com/gofrs/uuid" + "go.uber.org/zap" "github.com/transcom/mymove/pkg/appcontext" + "github.com/transcom/mymove/pkg/apperror" "github.com/transcom/mymove/pkg/db/utilities" + "github.com/transcom/mymove/pkg/models" "github.com/transcom/mymove/pkg/services" ) @@ -15,7 +20,40 @@ func NewMovingExpenseDeleter() services.MovingExpenseDeleter { return &movingExpenseDeleter{} } -func (d *movingExpenseDeleter) DeleteMovingExpense(appCtx appcontext.AppContext, movingExpenseID uuid.UUID) error { +func (d *movingExpenseDeleter) DeleteMovingExpense(appCtx appcontext.AppContext, ppmID uuid.UUID, movingExpenseID uuid.UUID) error { + var ppmShipment models.PPMShipment + err := appCtx.DB().Scope(utilities.ExcludeDeletedScope()). + EagerPreload( + "Shipment.MoveTaskOrder.Orders", + "MovingExpenses", + ). + Find(&ppmShipment, ppmID) + if err != nil { + if err == sql.ErrNoRows { + return apperror.NewNotFoundError(movingExpenseID, "while looking for MovingExpense") + } + return apperror.NewQueryError("MovingExpense fetch original", err, "") + } + + if ppmShipment.Shipment.MoveTaskOrder.Orders.ServiceMemberID != appCtx.Session().ServiceMemberID { + wrongServiceMemberIDErr := apperror.NewForbiddenError("Attempted delete by wrong service member") + appCtx.Logger().Error("internalapi.DeleteMovingExpenseHandler", zap.Error(wrongServiceMemberIDErr)) + return wrongServiceMemberIDErr + } + + found := false + for _, lineItem := range ppmShipment.MovingExpenses { + if lineItem.ID == movingExpenseID { + found = true + break + } + } + if !found { + mismatchedPPMShipmentAndMovingExpenseIDErr := apperror.NewNotFoundError(movingExpenseID, "Moving expense does not exist on ppm shipment") + appCtx.Logger().Error("internalapi.DeleteMovingExpenseHandler", zap.Error(mismatchedPPMShipmentAndMovingExpenseIDErr)) + return mismatchedPPMShipmentAndMovingExpenseIDErr + } + movingExpense, err := FetchMovingExpenseByID(appCtx, movingExpenseID) if err != nil { return err diff --git a/pkg/services/moving_expense/moving_expense_deleter_test.go b/pkg/services/moving_expense/moving_expense_deleter_test.go index 8e025bb8af3..cbe0148cc8e 100644 --- a/pkg/services/moving_expense/moving_expense_deleter_test.go +++ b/pkg/services/moving_expense/moving_expense_deleter_test.go @@ -65,9 +65,10 @@ func (suite *MovingExpenseSuite) TestDeleteMovingExpense() { } suite.Run("Returns an error if the original doesn't exist", func() { notFoundMovingExpenseID := uuid.Must(uuid.NewV4()) + ppmID := uuid.Must(uuid.NewV4()) deleter := NewMovingExpenseDeleter() - err := deleter.DeleteMovingExpense(suite.AppContextWithSessionForTest(&auth.Session{}), notFoundMovingExpenseID) + err := deleter.DeleteMovingExpense(suite.AppContextWithSessionForTest(&auth.Session{}), ppmID, notFoundMovingExpenseID) if suite.Error(err) { suite.IsType(apperror.NotFoundError{}, err) @@ -81,11 +82,12 @@ func (suite *MovingExpenseSuite) TestDeleteMovingExpense() { suite.Run("Successfully deletes as a customer's moving expense", func() { originalMovingExpense := setupForTest(nil, true) - deleter := NewMovingExpenseDeleter() suite.Nil(originalMovingExpense.DeletedAt) - err := deleter.DeleteMovingExpense(suite.AppContextWithSessionForTest(&auth.Session{}), originalMovingExpense.ID) + err := deleter.DeleteMovingExpense(suite.AppContextWithSessionForTest(&auth.Session{ + ServiceMemberID: originalMovingExpense.Document.ServiceMemberID, + }), originalMovingExpense.PPMShipmentID, originalMovingExpense.ID) suite.NoError(err) var movingExpenseInDB models.MovingExpense