-
Notifications
You must be signed in to change notification settings - Fork 34
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
Int b 21480 full ppm counseling #14639
base: integrationTesting
Are you sure you want to change the base?
Conversation
…to MAIN-B-21480-Full-PPM-Counseling
…Full-PPM-Counseling
…' into INT-B-21480
pkg/services/transportation_office/transportation_office_fetcher.go
Outdated
Show resolved
Hide resolved
@brianmanley-caci i tested out your suggested changes and that fixed the broken test. I went ahead and applied the changes to the pr. Thanks |
@samaysofo I think you also need to apply the changes from that commit in transportation_office_fetcher.go for the error handling. I'm still seeing test failures in other tests which may be because that is missing. |
suite.Assertions.IsType(&handlers.ErrResponse{}, response) | ||
internalServerErrorResponse := response.(*handlers.ErrResponse) | ||
suite.Equal(internalServerErrorResponse.Code, http.StatusInternalServerError) | ||
suite.Equal(internalServerErrorResponse.Err.Error(), "failure saving move when routing move submission") |
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.
This test is failing. I believe the correct error message should be "Failed to find counseling office that provides counseling"
@samaysofo I'm seeing an error in move_router_test.go for "PPM moves are routed correctly and SignedCertification is created" now. That test probably needs to be updated as well. |
PR is up for review again, when someone gets the chance. Thanks |
err = suite.DB().Find(&move, move.ID) | ||
suite.NoError(err) | ||
suite.Equal(tt.moveStatus, move.Status) | ||
if err != nil { |
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.
I think you should be testing for an expected outcome. This existing test should test for no error like it was previously. Then you could add an additional test to test the "Failed to find counseling office..." case. Does that make sense?
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.
oh yes, that makes sense. will update to add separate cases.
suite.Equal(models.MoveStatusNeedsServiceCounseling, move.Status, "expected Needs Service Counseling") | ||
suite.Equal(models.MTOShipmentStatusSubmitted, move.MTOShipments[0].Status, "expected Submitted") | ||
suite.Equal(models.PPMShipmentStatusSubmitted, move.MTOShipments[0].PPMShipment.Status, "expected Submitted") | ||
if err != nil { |
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.
Same thought here with probably having different test for different expected outcomes.
B-21480
Summary
These changes finds the closest transportation office that provides counseling in the GBLOC of the given duty location for oconus/conus duty locations then assign them to an order with a full PPM shipment that do not provide counseling. It also disables the counseling fee checkbox for full ppm shipments.
Is there anything you would like reviewers to give additional scrutiny?
this article explains more about the approach used.
Verification Steps for the Author
These are to be checked by the author.
Verification Steps for Reviewers
These are to be checked by a reviewer.
Setup to Run the Code
NOTE: I had to set the duty location for zip code (32222) provides_counseling to false in other to get a duty location with that status.
Run this to get all duty_location:
select * from duty_locations ;
Get the Id of one of the duty_locations and run this :
UPDATE duty_locations SET provides_services_counseling = false WHERE id = 'Provide_The_ID_You_Wish_TO_Update'
Double check if the record was updated
select * from duty_locations dl where dl.id ='The_ID_You_Updated'
How to test
Frontend
officeApp
class or custommin-width
styling is used to hide any states the would not be visible to the user.Backend
Database
Any new migrations/schema changes:
Screenshots
https://github.com/user-attachments/assets/77d9a562-4a6b-4111-8ffc-0d64835ab4d5
https://github.com/user-attachments/assets/d769ae4d-ee2e-4409-9183-707f294a57eb