-
Notifications
You must be signed in to change notification settings - Fork 35
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
B 21934 int #14629
base: integrationTesting
Are you sure you want to change the base?
B 21934 int #14629
Conversation
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.
Hm... Getting this error when adding orders as the SC:
2025-01-21T23:18:49.729Z ERROR ghcapi/orders.go:276 Data received from requester is bad: BAD_DATA: Error saving entitlement {"git_branch": "e-06167-av-polling-int", "git_commit": "eb6f4c304cb43bc605394e17033eb62b18ddd201", "host": "officelocal:3000", "milmove_trace_id": "6a2666f1-4202-422e-adfa-aaa8b854ebe5", "session_id": "p-tva2Q5W5_TJiB41HJEJB2QpysMcqcj9wXuwlAZLqU"}
github.com/transcom/mymove/pkg/handlers/ghcapi.(*CreateOrderHandler).Handle.CreateOrderHandler.Handle.func1
/Users/daniel.jordan_cn/mymove/pkg/handlers/ghcapi/orders.go:276
github.com/transcom/mymove/pkg/handlers.(*Config).AuditableAppContextFromRequestWithErrors.func1
/Users/daniel.jordan_cn/mymove/pkg/handlers/config.go:152
github.com/transcom/mymove/pkg/appcontext.(*appContext).NewTransaction.func1
/Users/daniel.jordan_cn/mymove/pkg/appcontext/context.go:67
I did run make db_dev_fresh
and start everything fresh. Looks like you might need to add AdminRestrictedWeightLocation
to the payload in pkg/handlers/ghcapi/orders.go:262
entitlement := models.Entitlement{
DependentsAuthorized: payload.HasDependents,
DBAuthorizedWeight: models.IntPointer(weight),
StorageInTransit: models.IntPointer(sitDaysAllowance),
ProGearWeight: weightAllotment.ProGearWeight,
ProGearWeightSpouse: weightAllotment.ProGearWeightSpouse,
AccompaniedTour: payload.AccompaniedTour,
DependentsUnderTwelve: dependentsUnderTwelve,
DependentsTwelveAndOver: dependentsTwelveAndOver,
UBAllowance: &weightAllotment.UnaccompaniedBaggageAllowance,
}
if saveEntitlementErr := appCtx.DB().Save(&entitlement); saveEntitlementErr != nil {
err = apperror.NewBadDataError("Error saving entitlement")
appCtx.Logger().Error(err.Error())
return orderop.NewCreateOrderUnprocessableEntity(), err
}
GunSafe: gunSafe, | ||
AdminRestrictedWeightLocation: adminRestrictedWeightLocation, | ||
WeightRestriction: weightRestriction, | ||
ETag: etag.GenerateEtag(entitlement.UpdatedAt), |
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.
Can you update a test to reflect these changes please?
StorageInTransit: sit, | ||
TotalDependents: totalDependents, | ||
TotalWeight: totalWeight, | ||
WeightRestriction: &weightRestriction, | ||
ETag: etag.GenerateEtag(entitlement.UpdatedAt), |
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.
Can you update a test to reflect these changes please?
pkg/services/order/order_updater.go
Outdated
if payload.AdminRestrictedWeightLocation != nil { | ||
order.Entitlement.AdminRestrictedWeightLocation = *payload.AdminRestrictedWeightLocation | ||
} | ||
|
||
if payload.WeightRestriction != nil { | ||
weightRestriction := int(*payload.WeightRestriction) | ||
order.Entitlement.WeightRestriction = &weightRestriction | ||
} |
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.
Can you update a test to reflect these changes please?
<div className={visualCuesStyle}> | ||
<dt>Admin Restricted Weight Location</dt> | ||
<dd data-testid="adminRestrictedWtLoc">{info.adminRestrictedWeightLocation ? 'Yes' : 'No'}</dd> | ||
</div> | ||
<div className={visualCuesStyle}> | ||
<dt>Weight Restriction</dt> | ||
<dd data-testid="weightRestriction"> | ||
{info.weightRestriction ? formatWeight(info.weightRestriction) : DEFAULT_EMPTY_VALUE} | ||
</dd> | ||
</div> |
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.
Can you update a test please?
what branch are you on... this works fine for me on my main branch and the int version (B-21934-int) |
I was on your branch |
interesting... ill add to that payload, but Im not getting that error- tried a few diff scenarios |
Hm. I'll give it another crack. |
Attaching video with server output. DB is fresh, haven't done anything. This happens for CONUS & OCONUS moves created by SC. Screen.Recording.2025-01-22.at.1.11.52.PM.mov |
Getting this when doing it as the customer:
Here's the customer side: Screen.Recording.2025-01-22.at.1.14.26.PM.mov |
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.
{"git_branch": "INT-B-21838-payment-request-rejection-reason" your branch shows this in the error ^^ |
swagger-def/ghc.yaml
Outdated
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.
really wish we had a linter rule set up for single or double quotes on these yaml files... someday
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 do need to add that to my list- thanks for the reminder
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.
What the.. How weird. Trying again. I was def on your branch. Might be losing my mind. |
ALTER TABLE entitlements | ||
ADD COLUMN IF NOT EXISTS admin_restricted_weight_location BOOLEAN; |
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.
Recommending remove this boolean column and instead basing whether or not the entitlement is restricted based on entitlements.weight_restriction
being null or not null.
Reason for recommendation is because this bool column would just be storing whether or not weight restriction is null or not and also we end up with the possibility of true
weight restriction without any integer provided
Screen.Recording.2025-01-23.at.8.59.38.AM.mov
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.
Thinking.. im going to read up on the feature and weigh the pros/cons
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.
@transcom/mountain-movers would also love others thoughts on this
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 wouldn't be opposed to just enforcing the constraight of if bool true that weight restriction int must also be present - but we'll want some method of enforcing that a restriction exists
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.
actually.. I don't hate it.. refactoring now
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.
ill just remove that column and variable, and the UI will read from the weightRestriction value being null or not
and if they check it.. they have to enter a value
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.
Yeah I agree with this approach! Nice catch Cam!
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.
Also agree! Seems clean
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.
done - give it a re-review
I do like this much better
B-21934
Summary
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
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