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-4638 Storybook allowances viewer form authorized weight #5439

Merged
merged 25 commits into from
Dec 10, 2020

Conversation

donaldthai
Copy link
Contributor

@donaldthai donaldthai commented Dec 9, 2020

Description

For this story let’s focus on getting the form field ready to go in storybook. We’ll tackle getting it onto the ‘page’ when we are doing the backend story: MB-5846: TOO can change Authorized Weight in the allowances viewer (backend)UNSCHEDULED

As a TOO, I need to be able to update the Service Member’s authorized weight if it was entered incorrectly or differs from what I know it should be, so that billing problems aren’t encountered when this move is being paid for.

Given that I'm a TOO in the Allowances editor

  • When I click the Authorized weight text box
  • Then I can enter free form text input

Given that I'm a TOO in the Allowances editor

  • When I enter a new Authorized Weight and click the Save button
  • Then the input is validated as a numeric input, greater than zero,
    • and reject non-numeric input

Given that I'm a TOO in the Allowances editor

  • When I enter a new Authorized Weight and move focus from the input
  • Then weight is formated along the pattern "##,### lbs"

Reviewer Notes

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

Setup

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

make client_build
make storybook

Storybook
http://localhost:6006/?path=/story/too-tio-components-allowancesdetailform--basic

View allowances page

  1. Log in as TOO, officelocal
  2. Click on any move
  3. Scroll down and click on Edit Allowances button

Code Review Verification Steps

  • 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

Storybook
image

View allowances page
image

Number mask in action
Dec-09-2020 13-25-37

@@ -173,6 +174,7 @@
"lines": 40,
"statements": 40
}
}
},
"transformIgnorePatterns": ["node_modules/(?!(imask)/)/"]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Jest tests had trouble doing some transform for the imask library. Followed this thread that describes the solution uNmAnNeR/imaskjs#322

@robot-mymove
Copy link

robot-mymove commented Dec 9, 2020

Warnings
⚠️

It looks like you are attempting to bypass a linter rule, which is not within
security compliance rules.
** Contains a rule that is not in the permitted eslint list. You are free to disable only: (
no-underscore-dangle
,prefer-object-spread
,object-shorthand
,camelcase
,react/jsx-props-no-spreading
,react/destructuring-assignment
,react/forbid-prop-types
,react/prefer-stateless-function
,react/sort-comp
,import/no-extraneous-dependencies
,import/order
,import/prefer-default-export
,import/no-named-as-default
) **
Please remove the bypass code and address the underlying issue. cc: @transcom/Truss-Pamplemoose

Messages
📖 🔗 MB-4638

Generated by 🚫 dangerJS against e2e7003

@donaldthai donaldthai added the ttv label Dec 9, 2020
@donaldthai donaldthai changed the base branch from master to ds-mb-4635-display-allowances-in-sidebar-component December 9, 2020 18:47
@donaldthai donaldthai marked this pull request as ready for review December 9, 2020 18:49
@donaldthai donaldthai requested review from a team December 9, 2020 18:49
Base automatically changed from ds-mb-4635-display-allowances-in-sidebar-component to master December 9, 2020 22:47
…mb-4638-storybook-authorized-weight-form
const initialValues = { authorizedWeight: `${authorizedWeight}` };

const validationSchema = Yup.object({
authorizedWeight: Yup.number().required('Required'),
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 we need to add a validation to prevent the user from entering the number zero

@duncan-truss
Copy link
Contributor

@donaldthai My more general question is why if authorizedWeight is null in the database the value coming back in the payload is set to the weightAllotment +- dependents. Won't it make the user think they don't need to confirm the authorized weight because we show it already populated? Maybe that's part of @Ronolibert 's backend story making that change to not load that value by default.

@monfresh
Copy link
Contributor

@duncan-truss Under normal circumstances, the AuthorizedWeight is stored in the DB at the time the order is created during customer onboarding. The value is initially the same as the weight allowance based on rank and dependents. Our test data does not populate the AuthorizedWeight field, which is yet another example of creating factories with unrealistic data. In ghc_entitlements.go, if the AuthorizedWeight DB field is empty for some reason, then we fall back to the Weight allowance, which is why the payload is returning that.

@duncan-truss
Copy link
Contributor

@duncan-truss Under normal circumstances, the AuthorizedWeight is stored in the DB at the time the order is created during customer onboarding. The value is initially the same as the weight allowance based on rank and dependents. Our test data does not populate the AuthorizedWeight field, which is yet another example of creating factories with unrealistic data. In ghc_entitlements.go, if the AuthorizedWeight DB field is empty for some reason, then we fall back to the Weight allowance, which is why the payload is returning that.

Thanks for explaining that @monfresh

@@ -39,7 +53,17 @@ describe('AllowancesDetailForm', () => {
});

it('uses defaults for undefined values', () => {
const wrapperNoProps = mount(<AllowancesDetailForm entitlements={{}} />);
const wrapperNoProps = mount(
<Formik initialValues={{ authorizedWeight: '0' }} onSubmit={jest.fn()}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to set this to null instead of zero since authorizedWeight could have a null value in theory since it's a pointer.

describe('with masking', () => {
const wrapper = mount(
<TextMaskedInput
// value={"8000"}
Copy link
Contributor

Choose a reason for hiding this comment

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

looks like you added this below

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.

Works well for me! The library says it's IE11 compatible too.

Copy link

@brooksr4 brooksr4 left a comment

Choose a reason for hiding this comment

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

Looks good and seems to meet the reasonable acceptance criteria. (note: I think there's some left over acceptance criteria for when it's in the system that can't be covered here)

@donaldthai donaldthai merged commit a9fc011 into master Dec 10, 2020
@donaldthai donaldthai deleted the ttv-mb-4638-storybook-authorized-weight-form branch December 10, 2020 22:59
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.

5 participants