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

Add shipment validation for workdays and remove GET validation #1637

Merged
merged 43 commits into from Feb 12, 2019

Conversation

6 participants
@mikena-truss
Copy link
Contributor

mikena-truss commented Jan 22, 2019

Description

Move dates on holidays or weekends currently throw an error (and subsequently a 500) when sending a GET request to the shipments API. Instead of handling that validation on the GET request, we should handle it when the shipment gets saved. This PR adds the validators and implements them in the shipment model. It also removes the code validating those dates on the shipment GET request.

Reviewer Notes

In line comments

Code Review Verification Steps

  • Code follows the guidelines for Logging
  • The requirements listed in
    Querying the Database Safely
    have been satisfied.
  • Any new migrations/schema changes:
    • Follow our guidelines for zero-downtime deploys (see Zero-Downtime Deploys)
    • Have been communicated to #dp3-engineering
    • Secure migrations have been tested using bin/run-prod-migrations
  • There are no aXe warnings for UI.
  • This works in IE.
  • Any new client dependencies (Google Analytics, hosted libraries, CDNs, etc) have been:
  • Tested in the Experimental environment (for changes to containers, app startup, or connection to data stores)
  • Request review from a member of a different team.
  • Have the Pivotal acceptance criteria been met for this change?

References

@mikena-truss mikena-truss added the wip label Jan 22, 2019

@akostibas akostibas self-requested a review Jan 22, 2019

@@ -32,6 +38,9 @@ func (suite *ModelSuite) Test_ShipmentValidations() {
"weight_estimate": []string{"-3 is less than zero."},
"progear_weight_estimate": []string{"-12 is less than zero."},
"spouse_progear_weight_estimate": []string{"-9 is less than zero."},
"requested_pickup_date": []string{"requested_pickup_date cannot be on a weekend or holiday, is 2019-01-20 00:00:00 +0000 UTC"},

This comment has been minimized.

@mikena-truss

mikena-truss Jan 23, 2019

Author Contributor

I felt like actual dates shouldn't be in here, because they're actual 😆 Maybe I'm wrong about that though.

@mikena-truss

This comment has been minimized.

Copy link
Contributor Author

mikena-truss commented Jan 23, 2019

I've gotta fix some tests that were on weekends too it looks like.

@@ -99,8 +99,8 @@ func (suite *AwardQueueSuite) Test_CheckShipmentDuringBlackOut() {
},
})

pickupDate := blackoutEndDate.AddDate(0, 0, 1)
deliveryDate := blackoutEndDate.AddDate(0, 0, 2)
pickupDate := blackoutEndDate.AddDate(0, 0, 2)

This comment has been minimized.

@mikena-truss

mikena-truss Jan 23, 2019

Author Contributor

Adding one day was a Sunday

This comment has been minimized.

@akostibas

akostibas Jan 29, 2019

Contributor

It might be helpful to add a NextWeekeday() helper to our testing tools, since it seems likely we'll be fighting this for forever now. Also, more clear.

This comment has been minimized.

@akostibas

akostibas Jan 29, 2019

Contributor

Oh! You added one to the calendar pkg. Can we use that?

@@ -42,10 +42,6 @@ func CreatePastMoveDates(startDate time.Time, numDays int, includeWeekendsAndHol
func CreateValidDatesBetweenTwoDates(startDate time.Time, endDate time.Time, includeWeekendsAndHolidays bool, allowEarlierOrSameEndDate bool, calendar *cal.Calendar) ([]time.Time, error) {
var dates []time.Time

if !calendar.IsWorkday(endDate) && !includeWeekendsAndHolidays {

This comment has been minimized.

@mikena-truss

mikena-truss Jan 23, 2019

Author Contributor

This is the code causing us trouble. I think there's more than can be refactored in this part of the code, but trying to keep this minimal for now.

This comment has been minimized.

@chrisgilmerproj

chrisgilmerproj Jan 23, 2019

Contributor

I don't actually think this is the issue. This validation I think is important (and we should probably be handling the error better wherever we call it). I think we should remove this code instead and rework how we get those dates: https://github.com/transcom/mymove/blob/master/pkg/handlers/internalapi/shipments.go#L52-L64

This comment has been minimized.

@chrisgilmerproj

chrisgilmerproj Jan 23, 2019

Contributor

Acutally, we have an API endpoint to get move dates. So throwing the move dates into the shipment payload just unnecessarily complicates the API call and makes it diverge from the DB model. I'd suggest we just find the place needing the move dates and make it do the additional API call instead of trying to put the kitchen sink into the Shipment payload.

This comment has been minimized.

@mikena-truss

mikena-truss Jan 23, 2019

Author Contributor

Yeah, I can write up a ticket for that in the bat team repo. I think it makes sense to keep this PR slim though, so we can move past the bug

This comment has been minimized.

@chrisgilmerproj

chrisgilmerproj Jan 23, 2019

Contributor

I mostly worry about this validation getting dropped. What will it affect?

This comment has been minimized.

@mikena-truss

mikena-truss Jan 23, 2019

Author Contributor

My understanding is that this function is just returning a set of dates from start (inclusive) to end (exclusive) excluding weekends and holidays. Removing this code makes it so that if the start date in the DB was already on a weekend or holiday, it will still be included. This seems ok to me.

This comment has been minimized.

@mikena-truss

mikena-truss Jan 23, 2019

Author Contributor

Also, I don't mind on holding off on this until @kimallen get's back tomorrow, because maybe she has a little more context.

}

calendar := dates.NewUSCalendar()
if s.RequestedPickupDate != nil {

This comment has been minimized.

@mikena-truss

mikena-truss Jan 23, 2019

Author Contributor

Does this look like an ok pattern for optional field validations?

This comment has been minimized.

@chrisgilmerproj

chrisgilmerproj Jan 23, 2019

Contributor

I like this pattern. But if there's a better one I'd love to hear about it.

This comment has been minimized.

@reggieriser

reggieriser Jan 23, 2019

Contributor

It looks like our validations for OptionalInt64IsPositive and OptionalPoundIsNonNegative above this just make Field a pointer and first check for its existence in the validator's IsValid. Is this case different? Or are we trying to make a validator that could also be used elsewhere when the field is required?

This comment has been minimized.

@mikena-truss

mikena-truss Jan 23, 2019

Author Contributor

I don't know how I missed this 🤦‍♀️ I think I was looking at another model for examples.

I kind of like not having it be OptionalIsWorkday, but then again, we could always use encapsulation and have an OptionalIsWorkDay and IsWorkday.

This comment has been minimized.

@mikena-truss

mikena-truss Jan 24, 2019

Author Contributor

@reggieriser I updated this to have an Optional field in the struct. That kind of seems nice to me? Maybe always having to use a pointer is a bad idea though 🤷‍♀️ LMK what you think.

This comment has been minimized.

@reggieriser

reggieriser Jan 24, 2019

Contributor

I'm good with the Optional bool in the struct (or Required -- whichever we think the zero bool value of false makes more sense to be).

However, I noticed a couple of things with the refactor:

  1. If Optional was false but you set DateIsWorkday.Field to nil, wouldn't IsValid fail with a nil dereference error when you do *v.Field?
  2. If you can fix number 1, then I think the Validate method in the model could become a little less complex -- you should just be able to add the three DateIsWorkday structs directly to the Validator array like the others.

This comment has been minimized.

@mikena-truss

mikena-truss Jan 24, 2019

Author Contributor

It looks like some of my later commits didn't make it (where I removed the nil checks in the model). However, I realized with the Optional field and nil checks it was complex enough to need some tests. When I started writing tests, it made it apparent that just using encapsulation made it prettier. I feel weird copy/pasting so many tests, but this doesn't seem like a terrible thing.

This comment has been minimized.

@mikena-truss

mikena-truss Jan 24, 2019

Author Contributor

Note that using encapsulation means we also don't need to pass a pointer when it's not optional, which saves us some nil checks and whatnot

This comment has been minimized.

@reggieriser

reggieriser Jan 25, 2019

Contributor

The reworked validator code and tests look good! 👍

@mikena-truss mikena-truss changed the title WIP add validator for workday Add shipment validation for workdays and remove GET validation Jan 23, 2019

@mikena-truss mikena-truss removed the wip label Jan 23, 2019

@mikena-truss mikena-truss requested a review from reggieriser Jan 23, 2019

@chrisgilmerproj
Copy link
Contributor

chrisgilmerproj left a comment

🌟 - Thanks for doing this. Let's also get a pivotal story up about removing the dates calculator from the shipment payload. That should get us into a good place.

@mikena-truss

This comment has been minimized.

Copy link
Contributor Author

mikena-truss commented Jan 23, 2019

&OptionalDateIsWorkday{
Field: s.OriginalDeliveryDate,
Name: "original_delivery_date",
Calendar: calendar},

This comment has been minimized.

@kimallen

kimallen Jan 24, 2019

Contributor

Do we need to also validate the pm_survey date for each of requested, pack, and delivery dates as well?
And I do think we need to also validate the actual dates. What does and does not happen on weekends/holidays, according to my memory of convo with SBD, should be consistent (shouldn't account for exceptions right now). The date ranges we create in the UI are dependent on that being consistent.

@kimallen

This comment has been minimized.

Copy link
Contributor

kimallen commented Jan 24, 2019

Hmm, I'm seeing comments in my email that I'm not finding here. To answer question about whether you can remove this code if !calendar.IsWorkday(endDate) && !includeWeekendsAndHolidays { return dates, errors.New("End date cannot be a weekend or holiday") }
I think it's safe to remove as long as you validate all the pm_survey and actual dates as well.

@reggieriser

This comment has been minimized.

Copy link
Contributor

reggieriser commented Jan 25, 2019

Seems like if we're going to validate all these dates in the model -- including the actual ones -- then we have to make sure all calendar widgets in the UI for these dates do not allow weekends or holidays to be selected (including the text box for manual date entry, if any). I know we make weekends/holidays non-selectable in the SM UI during the HHG wizard when a move date is selected (and there's no text box there), but I don't think we do the same in the office or TSP UI when actual dates are entered, do we? It's worth a test to see what happens with this code now when you set an actual delivery date to a weekend date, for instance.

@reggieriser

This comment has been minimized.

Copy link
Contributor

reggieriser commented Jan 25, 2019

I just tried the above scenario by delivering the ENTDEL move in the E2E data and put in 1/12/2019 (a Saturday) as the delivery date. The validations kicked in and the deliver endpoint failed with the data being bad, but I got no feedback at all in the UI.

@mikena-truss mikena-truss requested a review from sarboc Feb 5, 2019

mikena-truss added some commits Feb 5, 2019

@mikena-truss
Copy link
Contributor Author

mikena-truss left a comment

@sarboc @reggieriser @kimallen @chrisgilmerproj This should be ready for another review 🙏 . Changes from last time are:

  • vetting all dates (not just estimates)
  • added some frontend changes to handle the errors
@@ -10,3 +14,23 @@ func NewUSCalendar() *cal.Calendar {
cal.AddUsHolidays(usCalendar)
return usCalendar
}

// NextWorkday returns the next workday after the given date, using the given calendar
func NextWorkday(cal cal.Calendar, date time.Time) time.Time {

This comment has been minimized.

@mikena-truss

mikena-truss Feb 6, 2019

Author Contributor

IMO, was a toss up as to whether these should exist in a test package only. They seem like reasonable non test utils though.

@@ -39,8 +40,9 @@ func (suite *ModelSuite) TestFetchMove() {
order1 := testdatagen.MakeDefaultOrder(suite.DB())
order2 := testdatagen.MakeDefaultOrder(suite.DB())

pickupDate := time.Now()
deliveryDate := time.Now().AddDate(0, 0, 1)
calendar := dates.NewUSCalendar()

This comment has been minimized.

@mikena-truss

mikena-truss Feb 6, 2019

Author Contributor

Realizing that maybe these calendar objects should be part of the suite... getting a little exhausted of optimizing this PR, but if folks have strong opinions on that, happy to update.

@@ -135,6 +137,42 @@ func (s *Shipment) Validate(tx *pop.Connection) (*validate.Errors, error) {
&OptionalPoundIsNonNegative{Field: s.WeightEstimate, Name: "weight_estimate"},
&OptionalPoundIsNonNegative{Field: s.ProgearWeightEstimate, Name: "progear_weight_estimate"},
&OptionalPoundIsNonNegative{Field: s.SpouseProgearWeightEstimate, Name: "spouse_progear_weight_estimate"},
&OptionalDateIsWorkday{

This comment has been minimized.

@mikena-truss

mikena-truss Feb 6, 2019

Author Contributor

Someone please spot check all these values, because it's a lot <3

@@ -12,6 +12,7 @@ import { PanelSwaggerField } from 'shared/EditablePanel';
import { SwaggerField } from 'shared/JsonSchemaForm/JsonSchemaField';

import './office.css';
import { getRequestStatus } from 'shared/Swagger/selectors';

This comment has been minimized.

@mikena-truss

mikena-truss Feb 6, 2019

Author Contributor

Is there an ordering to imports? Maybe an IDE setting I can check?

This comment has been minimized.

@sarboc

sarboc Feb 6, 2019

Contributor

A stronger linter would care, but until we add that kind of check I think what you have is fine.

@@ -29,7 +29,7 @@ describe('DatesPanel tests', () => {
let store;

beforeEach(() => {
store = mockStore({});
store = mockStore({ requests: { lastErrors: [] } });

This comment has been minimized.

@mikena-truss

mikena-truss Feb 6, 2019

Author Contributor

Is this sufficient? It passes the tests, but is an incomplete mock.

This comment has been minimized.

@sarboc

sarboc Feb 6, 2019

Contributor

This looks good! If we were testing the error component, we might need to mock it out more fully, but whatever makes the tests pass is good, imo. Also, the initial state of the app looks like this, so testing that the page works on load is good, too.

.get('div.DayPicker-Month')
.contains('10')
.click();
.type('10/18/2018')

This comment has been minimized.

@mikena-truss

mikena-truss Feb 6, 2019

Author Contributor

Switching from the date picker to typing it in because relying on the current month default breaks things. I don't think this test was supposed to be date picker specific, but if it is, I think we can set current date in cypress. Seems like a smart thing to do as a convention?

We might see errors like this pop up at other points, but I'm not sure that's avoidable without a high overhead cost of manually vetting all tests...

This comment has been minimized.

@sarboc

sarboc Feb 6, 2019

Contributor

I think this is good for now. We've had lots of discussion around date issues without many solid conclusions. Now that we specifically need "good" dates, I think it makes sense to hardcode them.

@@ -104,12 +104,7 @@ const HHGTabContent = props => {
<Dates title="Dates" shipment={shipment} update={updatePublicShipment} />

This comment has been minimized.

@mikena-truss

mikena-truss Feb 6, 2019

Author Contributor

We could technically update more of these with errors, but once again, trying to keep this PR toned down a little. A lot of these are existing bugs it seems, and we can only solve so many at once...

@chrisgilmerproj
Copy link
Contributor

chrisgilmerproj left a comment

⭐️ for the go improvements. I would like someone else to look over the javascript changes.

@sarboc

sarboc approved these changes Feb 6, 2019

Copy link
Contributor

sarboc left a comment

Changes look good, and I think I addressed all of your comments. Awesome work, @mikena-truss! You'll be a frontend dev in no time. 😄

@@ -12,6 +12,7 @@ import { PanelSwaggerField } from 'shared/EditablePanel';
import { SwaggerField } from 'shared/JsonSchemaForm/JsonSchemaField';

import './office.css';
import { getRequestStatus } from 'shared/Swagger/selectors';

This comment has been minimized.

@sarboc

sarboc Feb 6, 2019

Contributor

A stronger linter would care, but until we add that kind of check I think what you have is fine.

@@ -120,7 +123,7 @@ export function PreMoveSurveyEditablePanelify(DisplayComponent, EditComponent, e
<React.Fragment>
{this.props.hasError && (
<Alert type="error" heading="An error occurred">
There was an error: <em>{this.props.errorMessage}</em>.
<em>{this.props.errorMessage}</em>.

This comment has been minimized.

@sarboc

sarboc Feb 6, 2019

Contributor

Since you removed some of the test here, and added a few other bits, can you do a walkthrough with design and make sure they're ok with the changes or add additional stories to make this better in the future?

@@ -29,7 +29,7 @@ describe('DatesPanel tests', () => {
let store;

beforeEach(() => {
store = mockStore({});
store = mockStore({ requests: { lastErrors: [] } });

This comment has been minimized.

@sarboc

sarboc Feb 6, 2019

Contributor

This looks good! If we were testing the error component, we might need to mock it out more fully, but whatever makes the tests pass is good, imo. Also, the initial state of the app looks like this, so testing that the page works on load is good, too.

.get('div.DayPicker-Month')
.contains('10')
.click();
.type('10/18/2018')

This comment has been minimized.

@sarboc

sarboc Feb 6, 2019

Contributor

I think this is good for now. We've had lots of discussion around date issues without many solid conclusions. Now that we specifically need "good" dates, I think it makes sense to hardcode them.

@codecov

This comment has been minimized.

Copy link

codecov bot commented Feb 12, 2019

Codecov Report

Merging #1637 into master will increase coverage by 0.34%.
The diff coverage is 75.6%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1637      +/-   ##
==========================================
+ Coverage      47%   47.35%   +0.34%     
==========================================
  Files         410      409       -1     
  Lines       17217    17258      +41     
  Branches     1587     1589       +2     
==========================================
+ Hits         8093     8172      +79     
+ Misses       8400     8363      -37     
+ Partials      724      723       -1
Flag Coverage Δ
#go 57.07% <100%> (+0.43%) ⬆️
#javascript 32.71% <23.07%> (+0.23%) ⬆️
Impacted Files Coverage Δ
pkg/dates/calculators.go 75% <ø> (+4.41%) ⬆️
src/scenes/Office/MoveInfo.jsx 34.1% <ø> (+0.26%) ⬆️
src/shared/EditablePanel/index.jsx 63.51% <ø> (ø) ⬆️
src/shared/Entities/modules/shipments.js 29.41% <0%> (-0.9%) ⬇️
src/shared/utils.js 42.59% <0%> (-5.33%) ⬇️
pkg/models/shipment.go 54.85% <100%> (+3.82%) ⬆️
pkg/dates/calendar.go 100% <100%> (ø) ⬆️
pkg/models/validators.go 75.92% <100%> (+4.81%) ⬆️
src/scenes/Office/PremoveSurvey.jsx 20.58% <11.11%> (-1.08%) ⬇️
src/shared/ShipmentDates/index.jsx 65.51% <55.55%> (-1.15%) ⬇️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7215b49...70e011e. Read the comment docs.

@@ -97,3 +97,17 @@ export function renderStatusIcon(status) {
return <FontAwesomeIcon className="icon approval-problem" icon={faExclamationCircle} />;
}
}

export function snakeCaseToCapitals(str) {
console.log(str);

This comment has been minimized.

@sarboc

sarboc Feb 12, 2019

Contributor

Console.log?

@mikena-truss mikena-truss merged commit 696cdea into master Feb 12, 2019

19 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
ci/circleci: acceptance_tests_experimental Your tests passed on CircleCI!
Details
ci/circleci: acceptance_tests_local Your tests passed on CircleCI!
Details
ci/circleci: acceptance_tests_staging Your tests passed on CircleCI!
Details
ci/circleci: build_app Your tests passed on CircleCI!
Details
ci/circleci: build_migrations Your tests passed on CircleCI!
Details
ci/circleci: build_tools Your tests passed on CircleCI!
Details
ci/circleci: client_test Your tests passed on CircleCI!
Details
ci/circleci: client_test_coverage Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_mymove Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_office Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_tsp Your tests passed on CircleCI!
Details
ci/circleci: pre_deps_golang Your tests passed on CircleCI!
Details
ci/circleci: pre_deps_yarn Your tests passed on CircleCI!
Details
ci/circleci: pre_test Your tests passed on CircleCI!
Details
ci/circleci: server_test Your tests passed on CircleCI!
Details
ci/circleci: server_test_coverage Your tests passed on CircleCI!
Details
codecov/patch 75.6% of diff hit (target 47%)
Details
codecov/project 47.35% (+0.34%) compared to 7215b49
Details

@mikena-truss mikena-truss deleted the 163038889-mw-fix-moves-invalid-dates-staging branch Feb 12, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment