MB-13125 - handle state for weightTickets #8873

ronaktruss · 2022-07-08T19:33:55Z

Jira ticket for this change

Summary

Adds weightTicket info into the redux state. This change is still dependent on the integration to ensure that appropriate API calls are being made.

Setup to Run Your Code

💻 You will need to use two separate terminals to test this locally.

Terminal 1

Start the UI locally.

make client_run

Terminal 2

Start the Go server locally.

make server_run

Additional steps

Access the customer app
Login as [email protected]
When navigating from the about page to the weight ticket page you can verify that the redux state is updated with the fake data that is provided by the createWeightTicket function.
To test a hard refresh scenario you have to mock an ADD_ENTITIES action that has some weightTicket info.

Example action:

{
  type: 'ADD_ENTITIES',
  entities: {
    mtoShipments: {
      '2ed2998e-ae36-46cd-af83-c3ecee55fe3e': {
        createdAt: '2022-07-01T01:10:51.224Z',
        eTag: 'MjAyMi0wNy0xMVQxODoyMDoxOC43MjQ1NzRa',
        id: '2ed2998e-ae36-46cd-af83-c3ecee55fe3e',
        moveTaskOrderID: '26b960d8-a96d-4450-a441-673ccd7cc3c7',
        ppmShipment: {
          actualDestinationPostalCode: '30813',
          actualMoveDate: '2022-07-31',
          actualPickupPostalCode: '90210',
          advanceAmountReceived: null,
          advanceAmountRequested: 598700,
          approvedAt: '2022-04-15T12:30:00.000Z',
          createdAt: '2022-07-01T01:10:51.231Z',
          destinationPostalCode: '30813',
          eTag: 'MjAyMi0wNy0xMVQxODoyMDoxOC43NTIwMDNa',
          estimatedIncentive: 1000000,
          estimatedWeight: 4000,
          expectedDepartureDate: '2020-03-15',
          hasProGear: true,
          hasReceivedAdvance: false,
          hasRequestedAdvance: true,
          id: 'b9ae4c25-1376-4b9b-8781-106b5ae7ecab',
          netWeight: null,
          pickupPostalCode: '90210',
          proGearWeight: 1987,
          reviewedAt: null,
          secondaryDestinationPostalCode: '30814',
          secondaryPickupPostalCode: '90211',
          shipmentId: '2ed2998e-ae36-46cd-af83-c3ecee55fe3e',
          sitEstimatedCost: null,
          sitEstimatedDepartureDate: null,
          sitEstimatedEntryDate: null,
          sitEstimatedWeight: null,
          sitExpected: false,
          spouseProGearWeight: 498,
          status: 'WAITING_ON_CUSTOMER',
          submittedAt: null,
          updatedAt: '2022-07-11T18:20:18.752Z',
          weightTickets: [
            {
              id: "1111",
              emptyDocumentId: "2222",
            },
          ],
        },
        shipmentType: 'PPM',
        status: 'APPROVED',
        updatedAt: '2022-07-11T18:20:18.724Z'
      }
    }
  }
}

Verification Steps for Author

These are to be checked by the author.

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 Jira acceptance criteria been met for this change?

Verification Steps for Reviewers

These are to be checked by a reviewer.

Frontend

User facing changes have been reviewed by design.
There are no aXe warnings for UI.
This works in Supported Browsers and their phone views (Chrome, Firefox, IE, Edge).
There are no new console errors in the browser devtools
There are no new console errors in the test output
If this PR adds a new component to Storybook, it ensures the component is fully responsive, OR if it is intentionally not, a wrapping div using the officeApp class or custom min-width styling is used to hide any states the would not be visible to the user.

Screenshots

robot-mymove · 2022-07-08T19:43:57Z

	Messages
📖	🔗 MB-13125

Generated by 🚫 dangerJS against fceb84e

ronaktruss · 2022-07-11T21:39:49Z

src/pages/MyMove/PPM/Closeout/WeightTickets/WeightTickets.jsx

+          if (mtoShipment?.ppmShipment?.weightTickets) {
+            mtoShipment?.ppmShipment?.weightTickets.push(resp);
+          } else {
+            mtoShipment.ppmShipment.weightTickets = [resp];
+          }


This logic may not be necessary if weightTickets defaults to returning an empty array

thought: We return an empty array for other things like orders and MTOShipments, so I feel like we should probably remain consistent with that for weight tickets.

I'll leave this here for now, since we can't ensure that within this PR. But we can likely remove this during the integration story.

ronaktruss · 2022-07-11T21:41:23Z

src/pages/MyMove/PPM/Closeout/WeightTickets/WeightTickets.jsx

  const mtoShipment = useSelector((state) => selectMTOShipmentById(state, mtoShipmentId));
-  // TODO add selector for selecting weight ticket from Redux store when data changes are solidified
+  const currentWeightTicket = useSelector((state) => selectWeightTicketById(state, mtoShipmentId, weightTicketId));


it may be worth having a selector for getting all weightTickets and then another selector for getting the current weight ticket

ronaktruss · 2022-07-11T22:04:36Z

src/pages/MyMove/PPM/Closeout/WeightTickets/WeightTickets.test.jsx

-  it('displays an error if the createWeightTicket request fails', async () => {
-    createWeightTicket.mockRejectedValueOnce('an error occurred');
-
-    render(<WeightTickets />, { wrapper: MockProviders });
-
-    await waitFor(() => {
-      expect(screen.getByText('Failed to create trip record')).toBeInTheDocument();
-    });
-  });
-


I don't think this test matches what the user would see. If we were to change the mockRejectedValueOnce to mockRejectedValue you would see that the test would fail. On the UI we wouldn't show the error message because the page would still be in the state where we show the loading screen.

We can likely update the UI to conditionally show the error msg or the loading in this circumstance.

praise: nice catch!

…load

duncan-truss · 2022-07-12T15:58:53Z

src/pages/MyMove/PPM/Closeout/WeightTickets/WeightTickets.jsx

@@ -120,7 +124,7 @@ const WeightTickets = () => {
      trailerMeetsCriteria,
    };

-    patchWeightTicket(mtoShipment.id, weightTicket.id, payload, weightTicket.eTag)
+    patchWeightTicket(mtoShipment.id, currentWeightTicket.id, payload, currentWeightTicket.eTag)
      .then(() => {


Could you add in what the updating of the shipment weight ticket would look like?

Added. There are quite a few ways to handle this, happy to discuss alternatives.

To test locally relax the UI restrictions(remove yup validation/disabled condition) and ensure that the create and patch call return promises with the same ID.

duncan-truss · 2022-07-13T14:44:03Z

What is the plan for when the user adds a file and that request completes will you add that to the document uploads array in Redux? How about for the scenario where an upload is deleted?

I think my suggestion would still be to have weight tickets broken out into their own entities in Redux and be fetched separately in an endpoint that returns all weight tickets for a move/shipment instead of having that nested in the shipment response. That way not every fetch or update of a shipment will be required to re-fetch all of the weight tickets/pro-gear/expenses uploads every time. And like we talked about the customer and office apps share a service object and adding on those fetched associations will mean you have a lot of office endpoints that now need to return closeout docs when it's not needed for most functionality.

src/pages/MyMove/PPM/Closeout/WeightTickets/WeightTickets.jsx

ronaktruss · 2022-07-13T15:58:18Z

What is the plan for when the user adds a file and that request completes will you add that to the document uploads array in Redux? How about for the scenario where an upload is deleted?

Yeah that's likely the plan so we don't have to make a separate API call on each upload to fetch the latest info. Let me add examples of this.

I think my suggestion would still be to have weight tickets broken out into their own entities in Redux and be fetched separately in an endpoint that returns all weight tickets for a move/shipment instead of having that nested in the shipment response. That way not every fetch or update of a shipment will be required to re-fetch all of the weight tickets/pro-gear/expenses uploads every time. And like we talked about the customer and office apps share a service object and adding on those fetched associations will mean you have a lot of office endpoints that now need to return closeout docs when it's not needed for most functionality.

I think that's a reasonable approach though I have some concerns about the tradeoff between time and performance. Will schedule ~20 min to chat if that works for you.

ronaktruss · 2022-07-13T17:33:37Z

I think my suggestion would still be to have weight tickets broken out into their own entities in Redux and be fetched separately in an endpoint that returns all weight tickets for a move/shipment instead of having that nested in the shipment response. That way not every fetch or update of a shipment will be required to re-fetch all of the weight tickets/pro-gear/expenses uploads every time. And like we talked about the customer and office apps share a service object and adding on those fetched associations will mean you have a lot of office endpoints that now need to return closeout docs when it's not needed for most functionality.

@duncan-truss an I just had a chat about the above. There are two options that were discussed:

Have weight tickets/pro gear/expenses be their own entities
Have these nested within existing entities

Summary

I'm leaning towards going with 2 since imo its likely less time consuming and it provides us a way to get there and iterate as we come across pain points. This approach means that we likely need to make some modifications/work arounds when we get to the office portion.

Additional Context (examples of what each approach may look like, implementation details could change):

Have weight tickets/pro gear/expenses be their own entities
- Having useEffects on the associated pages to ensure that the correct information is available in state on each page
- This may means adding 4 endpoints (1 for each of the three entities and 1 for all of them together)
- Add additional entities to Redux state
- We will still need to have local redux logic to update the specific index of the entity (ie. Updating the correct weight ticket if we have multiple)
- We need to have redux logic manipulating the upload information to avoid having to make an additional API call
- We have a shared handler that can be used by both the office app
Have these nested within existing entities (this is the approach used in this PR)
- Have more significantly more logic to manipulate redux state (most of it will be abstracted as selectors)
- Makes components more reliant on state
- We need to have redux logic manipulating the upload information to avoid having to make an additional API call
- We will need to have to manage the office app separately (we may need to move the logic of the mto_shipment_fetcher that eagerly loads information into the service or handler so that its not shared by the office)

felipe-lee · 2022-07-13T18:45:48Z

I think my suggestion would still be to have weight tickets broken out into their own entities in Redux and be fetched separately in an endpoint that returns all weight tickets for a move/shipment instead of having that nested in the shipment response. That way not every fetch or update of a shipment will be required to re-fetch all of the weight tickets/pro-gear/expenses uploads every time. And like we talked about the customer and office apps share a service object and adding on those fetched associations will mean you have a lot of office endpoints that now need to return closeout docs when it's not needed for most functionality.

@duncan-truss an I just had a chat about the above. There are two options that were discussed:
1. Have weight tickets/pro gear/expenses be their own entities

2. Have these nested within existing entities
Summary

I'm leaning towards going with 2 since imo its likely less time consuming and it provides us a way to get there and iterate as we come across pain points. This approach means that we likely need to make some modifications/work arounds when we get to the office portion.

Additional Context (examples of what each approach may look like, implementation details could change):
1. Have weight tickets/pro gear/expenses be their own entities
   
   * Having useEffects on the associated pages to ensure that the correct information is available in state on each page
   * This may means adding 4 endpoints (1 for each of the three entities and 1 for all of them together)
   * Add additional entities to Redux state
   * We will still need to have local redux logic to update the specific index of the entity (ie. Updating the correct weight ticket if we have multiple)
   * We need to have redux logic manipulating the upload information to avoid having to make an additional API call
   * We have a shared handler that can be used by both the office app

2. Have these nested within existing entities (this is the approach used in this PR)
   
   * Have more significantly more logic to manipulate redux state (most of it will be abstracted as selectors)
   * Makes components more reliant on state
   * We need to have redux logic manipulating the upload information to avoid having to make an additional API call
   * We will need to have to manage the office app separately (we may need to move the logic of the mto_shipment_fetcher that eagerly loads information into the service or handler so that its not shared by the office)

I think for the sake of time, I'm leaning toward option 2 as well. I wonder if we do need to add workarounds for the office or if the extra calls will add much since they will only really affect PPMShipments. Like maybe we should check to see how much that actually affects things.

If we chose not to go with option 2, I think another option would be to add the GET method to the /mto-shipments/{mtoShipmentId} endpoint so we can get specific shipments. It's faster to put up a single API endpoint, it's less costly to fetch a single shipment than all shipments, we already have a redux action to update a single shipment, and it would still contain the info we need. The main downside to this is that weight tickets would sometimes be in redux and sometimes not (since the /mto_shipments call wouldn't return the weight tickets). So maybe not a good option 🤷🏼

…edux

src/pages/MyMove/PPM/Closeout/WeightTickets/WeightTickets.jsx

ronaktruss · 2022-07-14T13:09:54Z

src/pages/MyMove/PPM/Closeout/WeightTickets/WeightTickets.jsx

@@ -96,9 +108,27 @@ const WeightTickets = () => {
  };

  const handleUploadDelete = (uploadId, fieldName, values, setFieldTouched, setFieldValue) => {
-    const remainingUploads = values[`${fieldName}`]?.filter((upload) => upload.id !== uploadId);


Is there a reason that something like values[`${fieldName}`] is used over values[fieldName]?

It's usually to avoid eslint detect-object-injection errors because fieldName, if provided by the user or other malicious source, might not be a string eslint-community/eslint-plugin-security#21

duncan-truss · 2022-07-15T22:03:40Z

src/pages/MyMove/PPM/Closeout/WeightTickets/WeightTickets.jsx

        history.push(generatePath(customerRoutes.SHIPMENT_PPM_REVIEW_PATH, { moveId, mtoShipmentId }));
+        dispatch(updateMTOShipment(mtoShipment));


I don't know if this is reachable if it comes after history.push(), it might need moved before the history call.

When I tested this, it seemed to work as expected. I'll double check the behavior during the integration.

scarequotes

Thumbs up.

WIP, action is not a plain object error

c992558

ronaktruss added 6 commits July 11, 2022 18:38

Fix dispatch issue and remove local logging

1311e8d

Clean up comments

36080d7

Update tests to use new selector

570c2e9

Add back accidentally removed code

f67297a

Remove unnecesary awaits

59a6581

Clean up test diffs

8595730

ronaktruss commented Jul 11, 2022

View reviewed changes

ronaktruss changed the title ~~action is not a plain object error~~ MB-13125 - handle state for weightTickets Jul 11, 2022

ronaktruss added the A-Team label Jul 11, 2022

ronaktruss requested review from duncan-truss, felipe-lee, jenniferlynparsons, NamibiaTorres and pearl-truss July 11, 2022 22:06

ronaktruss marked this pull request as ready for review July 11, 2022 22:10

Add functionality to show error msg when create fails during initial …

b8c6131

…load

duncan-truss reviewed Jul 12, 2022

View reviewed changes

ronaktruss added 2 commits July 12, 2022 18:59

Add logic for how redux state would be updated during an update

e93a5fb

Add test for selector

d5c4a53

pearl-truss reviewed Jul 13, 2022

View reviewed changes

src/pages/MyMove/PPM/Closeout/WeightTickets/WeightTickets.jsx Outdated Show resolved Hide resolved

Remove unnecessary optional chaining

66de399

ronaktruss added 2 commits July 14, 2022 12:50

Add implementation details for how to handle uploads and deletes in R…

12c13e5

…edux

Refactor selector name

98c994b

ronaktruss commented Jul 14, 2022

View reviewed changes

src/pages/MyMove/PPM/Closeout/WeightTickets/WeightTickets.jsx Outdated Show resolved Hide resolved

ronaktruss commented Jul 14, 2022

View reviewed changes

ronaktruss added 4 commits July 14, 2022 14:24

Add minimal updates to fix tests

b66fe15

Rename variablesto remove additional check

2443572

Merge branch 'master' into rp-MB-13125-handle-state-weight-tickets

8ba24db

Update storybook files with the name changes for document fields

d389589

ronaktruss requested a review from a team July 14, 2022 17:20

Update comment to remove uncertainty

3d6e244

duncan-truss reviewed Jul 15, 2022

View reviewed changes

duncan-truss approved these changes Jul 15, 2022

View reviewed changes

scarequotes approved these changes Jul 18, 2022

View reviewed changes

ronaktruss added the ready-to-merge label Jul 18, 2022

Merge branch 'master' into rp-MB-13125-handle-state-weight-tickets

fceb84e

mergify bot merged commit 5c1530f into master Jul 18, 2022

mergify bot deleted the rp-MB-13125-handle-state-weight-tickets branch July 18, 2022 17:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MB-13125 - handle state for weightTickets #8873

MB-13125 - handle state for weightTickets #8873

ronaktruss commented Jul 8, 2022 •

edited

Loading

robot-mymove commented Jul 8, 2022 •

edited

Loading

ronaktruss Jul 11, 2022

felipe-lee Jul 13, 2022

ronaktruss Jul 15, 2022

ronaktruss Jul 11, 2022

ronaktruss Jul 11, 2022 •

edited

Loading

felipe-lee Jul 13, 2022

duncan-truss Jul 12, 2022

ronaktruss Jul 12, 2022

duncan-truss commented Jul 13, 2022

ronaktruss commented Jul 13, 2022 •

edited

Loading

ronaktruss commented Jul 13, 2022 •

edited

Loading

felipe-lee commented Jul 13, 2022

Summary

Additional Context (examples of what each approach may look like, implementation details could change):

ronaktruss Jul 14, 2022

duncan-truss Jul 14, 2022

duncan-truss Jul 15, 2022

ronaktruss Jul 18, 2022

scarequotes left a comment

		history.push(generatePath(customerRoutes.SHIPMENT_PPM_REVIEW_PATH, { moveId, mtoShipmentId }));
		dispatch(updateMTOShipment(mtoShipment));

MB-13125 - handle state for weightTickets #8873

MB-13125 - handle state for weightTickets #8873

Conversation

ronaktruss commented Jul 8, 2022 • edited Loading

Jira ticket for this change

Summary

Setup to Run Your Code

Terminal 1

Terminal 2

Additional steps

Verification Steps for Author

Verification Steps for Reviewers

Frontend

Screenshots

robot-mymove commented Jul 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ronaktruss Jul 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

duncan-truss commented Jul 13, 2022

ronaktruss commented Jul 13, 2022 • edited Loading

ronaktruss commented Jul 13, 2022 • edited Loading

Summary

Additional Context (examples of what each approach may look like, implementation details could change):

felipe-lee commented Jul 13, 2022

Summary

Additional Context (examples of what each approach may look like, implementation details could change):

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scarequotes left a comment

Choose a reason for hiding this comment

ronaktruss commented Jul 8, 2022 •

edited

Loading

robot-mymove commented Jul 8, 2022 •

edited

Loading

ronaktruss Jul 11, 2022 •

edited

Loading

ronaktruss commented Jul 13, 2022 •

edited

Loading

ronaktruss commented Jul 13, 2022 •

edited

Loading