Skip to content
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

[MB-9736] PrimeUI Update Shipment Page #7595

Merged
merged 14 commits into from
Oct 20, 2021
Merged

[MB-9736] PrimeUI Update Shipment Page #7595

merged 14 commits into from
Oct 20, 2021

Conversation

jacquelineIO
Copy link
Contributor

@jacquelineIO jacquelineIO commented Oct 15, 2021

Description

This add an Update Shipment page to the Prime UI

Some fields will not be editable if they are already present, because once they are set the Prime API doesn't let the user update again. These fields are: Requested Pickup Date, Estimated Weight, Pickup and Destination Address (as seen in photo below)
Screen Shot 2021-10-15 at 12 50 03 PM

Fields that are allowed to updated multiple times are left in edit mode. And then those values are sent even if there is no change. If the user does update these values they are sent in an update.

If the shipment is a "new to the Prime shipment" a lot of the fields are editable and the user will be able to edit them and send the update.
Screen Shot 2021-10-15 at 12 56 30 PM

Requested Pickup Date and Pickup Address should usually be read-only because those are set when the customer goes through the onboarding app.

Update UI to show detail errors when the update fails:

Screen Shot 2021-10-15 at 4 57 12 PM

Screen Shot 2021-10-15 at 4 58 11 PM

Reviewer Notes

Current issue is that the destination address is not being set when using the update shipment call and I'm not sure why that is. I can see the body in the console and it is what I expect it to be.
Fixed this issue. When using the Prime API and re-using Office React components/pages, you will have to convert the data to and from camelCase to snake_case. This was causing the shipment address update to not work.

Setup

Add any steps or code to run in this section to help others prepare to run your code:

Login as Prime UI user and click on shipment.
Then click the Update Shipment button for the shipment that you wish to update.

Code Review Verification Steps

  • If the change is risky, it has been tested in experimental before merging.
  • 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 #g-database
    • Secure migrations have been tested following the instructions in our docs
  • There are no aXe warnings for UI.
  • This works in Supported Browsers and their phone views (Chrome, Firefox, IE, Edge).
  • Tested in the Experimental environment (for changes to containers, app startup, or connection to data stores)
  • User facing changes have been reviewed by design.
  • Request review from a member of a different team.
  • Have the Jira acceptance criteria been met for this change?

References

Screenshots

If this PR makes visible UI changes, an image of the finished UI can help reviewers and casual
observers understand the context of the changes. A before image is optional and
can be included at the submitter's discretion.

Consider using an animated image to show an entire workflow instead of using multiple images. You may want to use GIPHY CAPTURE for this! 📸

Please frame screenshots to show enough useful context but also highlight the affected regions.

@robot-mymove
Copy link

robot-mymove commented Oct 15, 2021

Messages
📖 🔗 MB-9736

Generated by 🚫 dangerJS against 840b529

@jacquelineIO jacquelineIO marked this pull request as ready for review October 15, 2021 22:54
@jacquelineIO
Copy link
Contributor Author

Opening this PR for review. I will be adding tests next week.

Copy link
Contributor

@rogeruiz rogeruiz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great work on this so far @jacquelineIO. I'm requesting changes on this because I think there are some show-stoppers in this PR as-is. But not all my comments need to be addressed in this PR.

The main bug I found is that I can't submit a fix if the server error 422 response comes from the Prime API.

src/pages/Office/index.jsx Show resolved Hide resolved
src/pages/PrimeUI/Shipment/PrimeUIShipmentForm.jsx Outdated Show resolved Hide resolved
src/pages/PrimeUI/Shipment/PrimeUIShipmentForm.jsx Outdated Show resolved Hide resolved
src/pages/PrimeUI/Shipment/PrimeUIShipmentForm.jsx Outdated Show resolved Hide resolved
src/pages/PrimeUI/Shipment/PrimeUIShipmentForm.jsx Outdated Show resolved Hide resolved
Copy link
Contributor

@YanZ777 YanZ777 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few comments on my end about the code. I think other people have better feedback about the implementation.

if (isLoading) return <LoadingPlaceholder />;
if (isError) return <SomethingWentWrong />;

const fromPrimeApiAddressFormat = (address) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think if other pages are going to need this later, it would be good to extract out, but you can leave this and other related functions in here for now.


// Not the Prime API address format
const isEmptyAddress = (address) => {
if (address.street_address_1 !== 'undefined' && address.street_address_1) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you could theoretically do and Object.keys here and check each value that way, but I can also see the benefit of being explicit about what values are being checked.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possible, I did attempt to use Object.keys but do not remember why but that wasn't working. It could be something I was doing though.

src/pages/PrimeUI/Shipment/PrimeUIShipmentForm.jsx Outdated Show resolved Hide resolved
createdAt: PropTypes.string,
approvedDate: PropTypes.string,
}).isRequired,
moveId: PropTypes.string.isRequired,
Copy link
Contributor

@duncan-truss duncan-truss Oct 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should be a moveTaskOrderID on the shipment, I think when these component props were added the field was just omitted. I think we have an existing shipment prop types shape somewhere too, but it may be for ghc address types.

Also I made similarly named component on my PR but didn't store it under src/pages/PrimeUI so it shouldn't conflict but we'll probably want to consolidate eventually.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@duncan-truss I was wondering if you had done. I have have some cleanup things. Since I'm bumping up to time to close the sprint, I will ask @donaldthai if we I can get a cleanup stories for some of the things mentioned in this PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

^ We're ahead of schedule so I don't mind if you want to split off some of the work here into a different story

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's definitely not blocking anything we can look at it in a future cleanup story.

@rogeruiz rogeruiz dismissed their stale review October 20, 2021 16:27

Dismissing this so I can review it again.

<Grid row>
<Grid col desktop={{ col: 8, offset: 2 }}>
{errorMessage?.detail && (
<div className={styles.errorContainer}>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is minor but I think I added these styles in my component's module.scss so I don't think the classname is getting found in CustomerContactInfoForm.module.scss.

Copy link
Contributor

@duncan-truss duncan-truss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was a tricky one, it worked well in my testing!

@jacquelineIO jacquelineIO merged commit 52ea6d7 into master Oct 20, 2021
@jacquelineIO jacquelineIO deleted the jm-MB-9736 branch October 20, 2021 23:14
@jacquelineIO
Copy link
Contributor Author

New Jira issue to address clean-up mentioned in this PR https://dp3.atlassian.net/browse/MB-9844

@jacquelineIO jacquelineIO mentioned this pull request Oct 22, 2021
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants