MB-11601 delete shipment #8375

ronaktruss · 2022-04-04T17:28:35Z

Jira ticket for this change

Summary

Is there anything you would like reviewers to give additional scrutiny?

this article explains more about the approach used.

Setup to Run Your Code

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

Terminal 1

Start the Storybook locally.

make storybook

Terminal 2

Start the UI locally.

make client_run

Terminal 3

Start the Go server locally.

make server_run

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-04-04T17:37:46Z

	Warnings
⚠️	Files located in legacy directories (src/shared or src/scenes) have been edited. Are you sure you don’t want to also relocate them to the new file structure? View the frontend file org ADR for more information

	Messages
📖	🔗 MB-11601

Generated by 🚫 dangerJS against 7eaa0d1

felipe-lee · 2022-04-06T00:24:04Z

src/components/ShipmentList/index.jsx

@@ -60,16 +56,29 @@ export const ShipmentListItem = ({
          <span className={styles.warningText}>{isOverweight ? 'Over weight' : 'Missing weight'}</span>
        </div>
      )}
-      {canEdit ? <FontAwesomeIcon icon="pen" className={styles.edit} /> : <div className={styles.noEdit} />}


I'm glad we're getting rid of the icon-only button in favor of text 🎉

felipe-lee · 2022-04-06T00:24:54Z

src/components/ShipmentList/index.jsx

+          <Button className={styles['edit-btn']} onClick={onDeleteClick} type="button">
+            Delete
+          </Button>
+          <div>|</div>
+          <Button className={styles['edit-btn']} onClick={onShipmentClick} type="button">
+            Edit
+          </Button>


I'm not 100% sure if we should keep using buttons or not for things that are in effect, just links. It's come up in the past but I don't remember the decision in the end. I've started a thread in #prac-frontend asking if others remember what was decided.

…t of objects

ronaktruss · 2022-04-06T15:48:44Z

src/components/Customer/Home/Step/Step.module.scss

+.step-container:last-of-type {
+  margin-bottom: 0;
+}
+


Remove additional whitespace at the bottom of the home page

ronaktruss · 2022-04-06T15:50:01Z

src/pages/MyMove/Review/Review.jsx

  return {
    ...ownProps,
-    canMoveNext: true, // TODO: prevent incomplete PPM submission?
+    canMoveNext: !!Object.keys(mtoShipments).length, // TODO: prevent incomplete PPM submission?


Makes next button disabled when we delete the last shipment. We still need to handle the scenarios where a PPM is incomplete

Heads up I removed this property in my PR that's now in master. I did add a selector to return the shipments similar to what you're doing above. Probably just move this logic to inside the component in conjunction with the check for an incomplete PPM.

… a button

…-shipment

ronaktruss · 2022-04-08T17:56:35Z

src/components/ShipmentList/ShipmentList.test.jsx


    await waitFor(() => {
      checkShipmentClick('ID-3', 1, SHIPMENT_OPTIONS.NTS);
    });

-    userEvent.click(screen.getByRole('button', { name: /^nts-release /i }));
+    editBtn = queryByRole(screen.getAllByTestId('shipment-list-item-container')[3], 'button', { name: 'Edit' });

    await waitFor(() => {


@felipe-lee Is there a reason that we are using await here as opposed just triggering the click and using expects similar to what is done for the deletes below?

I think I had been getting console errors without the awaits, but now I don't remember the exact error. You can try removing it and see if you get them. If not, then feel free to change this up.

ronaktruss · 2022-04-08T17:56:40Z

src/containers/FlashMessage/FlashMessage.jsx

+  useEffect(
+    () => () => {
+      // Clear this flash message on unmount (this will happen on navigation or if flash state changes)
+      clearFlashMessage(flash?.key);
+    },
+    [clearFlashMessage, flash?.key],
+  );


This was changed because the previous implementation made it so that if you modified the message multiple times on the same page it would be cleared on alternating notifications. (eg. If you delete two records on the first you would see a message, but on the second the message would disappear. You would then alternate seeing and not seeing it on subsequent triggers.)

Does anyone have context on the reason for this original implementation? If the comment is correct I think this servers to better mimic that description.

thought: I don't have any context on this.

I don't know anything about the initial implementation, but I think having a dependency array in useEffect is a good practice, and like you said, seems to match what the comment indicates we want, so I think this change is probably for the best.

YanZ777

The changes look good to me, but I would want someone else who has more context of the work to review this.

…-shipment

pearl-truss · 2022-04-12T15:14:41Z

cypress/integration/office/servicescounseling/servicesCounselingNTSR.js

@@ -40,7 +40,7 @@ describe('Services counselor user', () => {

    cy.get('[data-testid="modal"]').should('be.visible');

-    cy.get('[data-testid="modal"] button').contains('Delete shipment').click();
+    cy.get('[data-testid="modal"] button').contains('Delete shipment').click({ force: true });


Thanks for adding this!

duncan-truss · 2022-04-12T19:52:09Z

src/components/ConfirmationModals/DestructiveShipmentConfirmationModal.stories.jsx

+    onClose={() => {}}
+    onSubmit={() => {}}


Try to define these stories using the newer args pattern so that these props become editable in the Storybook controls panel and you can also use actions so those click events show up in the actions tab too.

duncan-truss · 2022-04-12T19:58:09Z

src/components/Customer/Review/ShipmentCard/ShipmentCard.module.scss

      display: inline-flex;
      flex: 0;
      align-items: center;
      min-width: auto;
      @include u-padding-x(0);
      @include u-font('body', 'xs');
+
+      button {
+        padding: 12px;


I think it's good to avoid using pixels and use USWDS tokens whenever possible. You should be able to rewrite this as @include u-padding(1.5) https://designsystem.digital.gov/design-tokens/spacing-units/

duncan-truss · 2022-04-12T21:55:01Z

I've been testing and things look very good! One minor issue I see is when you delete all of the shipments on the Review page, the next button is still enabled for you to submit the move. I think if you just combine a check for mtoShipments length with the complete PPM conditional in Review.jsx you can fix that disabling.

… when no shipments are present

ronaktruss · 2022-04-12T23:02:20Z

I've been testing and things look very good! One minor issue I see is when you delete all of the shipments on the Review page, the next button is still enabled for you to submit the move. I think if you just combine a check for mtoShipments length with the complete PPM conditional in Review.jsx you can fix that disabling.

@duncan-truss Yeah I caught this as well when I was working on the cypress tests. I forgot to add this back in when I merged in your code that added the logic for the incomplete PPMs. See commit

felipe-lee · 2022-04-12T23:09:54Z

src/components/ShipmentList/ShipmentList.test.jsx


    await waitFor(() => {
      checkShipmentClick('ID-3', 1, SHIPMENT_OPTIONS.NTS);
    });

-    userEvent.click(screen.getByRole('button', { name: /^nts-release /i }));
+    editBtn = queryByRole(screen.getAllByTestId('shipment-list-item-container')[3], 'button', { name: 'Edit' });

    await waitFor(() => {


I think I had been getting console errors without the awaits, but now I don't remember the exact error. You can try removing it and see if you get them. If not, then feel free to change this up.

src/components/Customer/Review/ShipmentCard/PPMShipmentCard/PPMShipmentCard.test.jsx

src/components/ShipmentList/ShipmentList.test.jsx

felipe-lee · 2022-04-13T00:48:40Z

src/shared/Entities/reducer.js

+  if (action.type === 'UPDATE_MTO_SHIPMENTS_ENTITIY') {
+    return {
+      ...state,
+      mtoShipments: action.entities['mtoShipments'] || {},
+    };
+  }


Let me preface this by saying, I don't know much (i.e. next to nothing) about redux or how all these things tie together. But, looking at the existing code, is there a reason we need the new path for updating MTO shipments? It seems like the ADD_ENTITIES path is doing the same thing, at least in the end. It's doing a merge of the original and new, but if the new overwrites the whole of mtoShipments I feel like that should be fine? Again, I could be totally off and if you tried that and it didn't work, then nvm.

ADD_ENTITIES doesn't work because it is doing a merge of the original and new mtoShipments. When we merge it only adds or updates any of the updated or new records (it basically just overrides values in the old with the new if they have the same key or adds entries if the key is new). This means when we delete the original has all of the old values and the new values has one less value. The merge function then doesn't change since the new doesn't have any additional info.

I think it would clarify the purpose some and maybe make it a bit generalized if we called it REPLACE_ENTITY to reflect what it's doing now.

As is, I would be hesitant to call it REPLACE_ENTITY since this functionality doesn't work at a general level. It currently only works for mtoShipments.

Right, you have it hardcoded to the 'mtoShipments' key, you would need to make that use a param. I don't think it's a must have.

I started a semi-related thread in #prac-frontend about this and plan on bringing it up at tomorrow's check in.

src/sagas/entities.js

duncan-truss · 2022-04-13T14:35:31Z

src/pages/MyMove/Review/Review.jsx

@@ -62,7 +62,7 @@ const Review = ({ currentMove, mtoShipments, push, match }) => {
            <div className={formStyles.formActions}>
              <WizardNavigation
                onNextClick={handleNext}
-                disableNext={!hasCompletedPPMShipments}
+                disableNext={!hasCompletedPPMShipments || !Object.keys(mtoShipments).length}


mtoShipments should be an array of objects coming back from the selectMTOShipmentsForCurrentMove selector or assigned to an empty array by default. I don't think calling Object.keys is necessary here.

scarequotes

This PR works as expected for design.

One call-out: Deleted shipments work as expected on this page, but then at least sometimes appear on the review screen. I think there's an existing ticket to fix that as a separate story, but just a heads-up that that will need to be fixed at some point.

felipe-lee · 2022-04-13T18:39:39Z

src/containers/FlashMessage/FlashMessage.jsx

+  useEffect(
+    () => () => {
+      // Clear this flash message on unmount (this will happen on navigation or if flash state changes)
+      clearFlashMessage(flash?.key);
+    },
+    [clearFlashMessage, flash?.key],
+  );


I don't know anything about the initial implementation, but I think having a dependency array in useEffect is a good practice, and like you said, seems to match what the comment indicates we want, so I think this change is probably for the best.

felipe-lee · 2022-04-13T18:39:48Z

src/pages/MyMove/Home/index.jsx

+            {showDeleteSuccessAlert && (
+              <Alert slim type="success">
+                The shipment was deleted.
+              </Alert>
+            )}
+            {showDeleteErrorAlert && (
+              <Alert slim type="error">
+                Something went wrong, and your changes were not saved. Please try again later or contact your counselor.
+              </Alert>
+            )}


Sorry, you might have addressed this somewhere and I missed it, but do we need the separate alerts? Like does <ConnectedFlashMessage /> not work if you're staying on the same page?

It's not actually clear to me when we use Alert vs ConnectedFlashMessage. I'd assume and it does generally seem to be the case that ConnectedFlashMessage was used when the parent had the ConnectedFlashMessage and the child element would just dispatch the action to update the message. Alerts seem to be used when the update is happening within the same component. With the update above we should be able to use ConnectedFlashMessage if we want.

I think you're right about the general times we use each. I guess I'd opt for, if the page already has <ConnectedFlashMessage/> to use it, but maybe this is a discussion for FE check in. I'm fine with keeping this as is for now and if we decide to change it, it can be a follow-up later on.

I started a thread in #prac-frontend about this and plan on bringing it up at tomorrow's check in.

…ing the button wrapper

felipe-lee

Found a few prop errors in tests, though hopefully they're easy to fix

felipe-lee · 2022-04-13T18:47:00Z

src/components/Customer/Review/ShipmentCard/HHGShipmentCard/HHGShipmentCard.test.jsx

@@ -13,7 +13,7 @@ const defaultProps = {
  shipmentNumber: 1,
  shipmentId: '#ABC123K',
  shipmentType: 'HHG',
-  showEditBtn: false,
+  showEditAndDeleteBtn: false,


I'm getting a prop type error for tests in this file because they're missing the onDeleteClick prop:

felipe-lee · 2022-04-13T18:48:27Z

src/components/Customer/Review/ShipmentCard/NTSRShipmentCard/NTSRShipmentCard.test.jsx

@@ -9,7 +9,7 @@ import { formatCustomerDate } from 'utils/formatters';
 const defaultProps = {
  moveId: 'testMove123',
  onEditClick: jest.fn(),
-  showEditBtn: false,
+  showEditAndDeleteBtn: false,


Same comment about the missing onDeleteClick prop

felipe-lee · 2022-04-13T18:48:39Z

src/components/Customer/Review/ShipmentCard/NTSShipmentCard/NTSShipmentCard.test.jsx

@@ -11,7 +11,7 @@ const defaultProps = {
  onEditClick: jest.fn(),
  shipmentId: '#ABC123K',
  shipmentType: 'HHG_INTO_NTS_DOMESTIC',
-  showEditBtn: false,
+  showEditAndDeleteBtn: false,


Same comment about the missing onDeleteClick prop

felipe-lee · 2022-04-13T18:52:22Z

src/pages/MyMove/Home/index.test.jsx

@@ -109,7 +109,7 @@ describe('Home component', () => {
        mtoShipmentId: testProps.mtoShipments[1].id,
      });

-      wrapper.find('ShipmentListItem').at(0).simulate('click');
+      wrapper.find('ShipmentListItem').at(0).find('button').at(1).simulate('click');


I'm getting a missing prop error from one of the tests in this file, but I think it wasn't one you updated so I don't think it's a blocker for this PR. Just putting the info here in case you want to address it:

felipe-lee

Found a few prop errors in stories

felipe-lee · 2022-04-13T19:16:16Z

src/components/ConfirmationModals/DestructiveShipmentConfirmationModal.stories.jsx

+  content: 'Some sample description',
+  submitText: 'YES!',
+  closeText: 'NO',
+  onClose: { action: 'close button clicked' },


I'm getting a prop type error on this:

I think this other story shows an example of actions:

mymove/src/components/Customer/PPMBooking/DateAndLocationForm/DateAndLocationForm.stories.jsx

Lines 46 to 47 in 4322de3

onSubmit: action('submit button clicked'),

onBack: action('back button clicked'),

Ooops, this wasn't supposed to be checked in 😅. As a general note thanks for being thorough and providing references/sources.

felipe-lee · 2022-04-13T20:07:07Z

src/components/ShipmentList/ShipmentList.stories.jsx

+    <ShipmentList
+      shipments={[{ id: '0001', shipmentType: SHIPMENT_OPTIONS.PPM }]}
+      onShipmentClick={() => {}}
+      onDeleteClick={() => {}}


This has a prop error from the missing moveSubmitted prop:

felipe-lee · 2022-04-13T20:07:32Z

src/components/ShipmentList/ShipmentList.stories.jsx

@@ -18,6 +22,7 @@ export const Basic = () => (
        { id: '0003', shipmentType: SHIPMENT_OPTIONS.PPM },
      ]}
      onShipmentClick={() => {}}
+      onDeleteClick={() => {}}


This one also needs the moveSubmitted prop

felipe-lee

I think given that some things need to discussed at a FE group level, I'm fine with this as is for now. Well pending those prop error fixes, but I don't want to be a blocker on this once you push those fixes up.

ronaktruss added 2 commits April 4, 2022 17:26

Add basic functionality for delete shipment

c2f2199

Remove trailing whitespace

5a399bb

ronaktruss added the A-Team label Apr 4, 2022

ronaktruss changed the title ~~Rp mb 11601 delete shipment~~ MB-11601 delete shipment Apr 4, 2022

ronaktruss added 2 commits April 4, 2022 21:58

Resolve issue when deleting the last shipment

8dc935a

Add success and failure alerts on home page

5e8dd74

felipe-lee reviewed Apr 6, 2022

View reviewed changes

ronaktruss added 2 commits April 6, 2022 14:56

Add delete functionality on review page

ab7552e

Update logic to disable next button becuase mtoShipments was an objec…

d77bd6b

…t of objects

ronaktruss commented Apr 6, 2022

View reviewed changes

ronaktruss added 11 commits April 6, 2022 18:14

Remove additional div

bb40740

Make shipment confirmation modal slightly more generic and relocate

fa10f43

Refactor and fix broken component tests

a381b58

Update cypress tests that broke due to html structure change

4c553ef

Clean up imports

f92fe05

Adjust storybook styling and add missing functions

38ecbd4

Add delete specific tests and empty delete func for storybook

5396132

Update cypress test to account for the entire shipment list not being…

e65ba5f

… a button

Merge branch 'master' into rp-mb-11601-delete-shipment

aeb2400

Resolve issues from merge conflict

5895851

Merge branch 'pm-mb-11673-ppm-delete-backend' into rp-mb-11601-delete…

e73e705

…-shipment

ronaktruss commented Apr 8, 2022

View reviewed changes

ronaktruss marked this pull request as ready for review April 8, 2022 18:05

ronaktruss requested review from a team, duncan-truss, felipe-lee and pearl-truss April 8, 2022 18:05

YanZ777 approved these changes Apr 12, 2022

View reviewed changes

ronaktruss added 2 commits April 12, 2022 13:28

Merge branch 'pm-mb-11673-ppm-delete-backend' into rp-mb-11601-delete…

8686025

…-shipment

Add force attribute to make test more consistent

dc6a1af

pearl-truss reviewed Apr 12, 2022

View reviewed changes

Add force attribute to make test more consistent

65eeec0

duncan-truss reviewed Apr 12, 2022

View reviewed changes

Add tests that were removed during merging to ensure next is disabled…

dddeeeb

… when no shipments are present

felipe-lee reviewed Apr 13, 2022

View reviewed changes

ronaktruss added 3 commits April 13, 2022 13:24

Update styles to use spacing units over pixels

73f9aca

Update tests to use templates and refactor backText to closeText

bc248fc

Update tests based on feedback: remove uncessary queryBy and awaits

151fe65

duncan-truss reviewed Apr 13, 2022

View reviewed changes

Remove unnessary Object.keys call since it is an array

4ba610e

Base automatically changed from pm-mb-11673-ppm-delete-backend to master April 13, 2022 15:46

ronaktruss added 2 commits April 13, 2022 15:51

Merge branch 'master' into rp-mb-11601-delete-shipment

7c8bf1c

Center weights and clean up unit usages

026474e

scarequotes approved these changes Apr 13, 2022

View reviewed changes

felipe-lee reviewed Apr 13, 2022

View reviewed changes

Add centering to match previous styling that was removed due to remov…

734579f

…ing the button wrapper

felipe-lee reviewed Apr 13, 2022

View reviewed changes

Add missing props for component tests

bbd775f

felipe-lee approved these changes Apr 13, 2022

View reviewed changes

Update and refactor storybook stories

7eaa0d1

ronaktruss merged commit 788b170 into master Apr 14, 2022

ronaktruss deleted the rp-mb-11601-delete-shipment branch April 14, 2022 18:07

ronaktruss mentioned this pull request Apr 19, 2022

MB-11600 Block incomplete PPMs on frontend #8438

Merged

6 tasks

	onSubmit: action('submit button clicked'),
	onBack: action('back button clicked'),

MB-11601 delete shipment #8375

MB-11601 delete shipment #8375

Conversation

ronaktruss commented Apr 4, 2022 • edited

Jira ticket for this change

Summary

Setup to Run Your Code

Terminal 1

Terminal 2

Terminal 3

Verification Steps for Author

Verification Steps for Reviewers

Frontend

Screenshots

robot-mymove commented Apr 4, 2022 • edited

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YanZ777 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

duncan-truss Apr 12, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

duncan-truss commented Apr 12, 2022

ronaktruss commented Apr 12, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

duncan-truss Apr 13, 2022 • edited

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

Choose a reason for hiding this comment

scarequotes left a comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felipe-lee left a comment

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

Choose a reason for hiding this comment

felipe-lee left a comment

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

Choose a reason for hiding this comment

felipe-lee left a comment

Choose a reason for hiding this comment

ronaktruss commented Apr 4, 2022 •

edited

robot-mymove commented Apr 4, 2022 •

edited

duncan-truss Apr 12, 2022 •

edited

duncan-truss Apr 13, 2022 •

edited