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

Fix 3702 order refunding number input #3826

Merged
merged 11 commits into from Feb 27, 2018

Conversation

Projects
None yet
3 participants
@nnnnat
Copy link
Member

commented Feb 22, 2018

Resolves #3702
Impact: major
Type: bugfix

Issue

The NumberTypeInput wasn't aware of input value changes that didn't come from the up/down arrow buttons.

The inputs were also not displaying properly in Firefox
screen shot 2018-02-21 at 5 14 50 pm

Solution

  • I fixed the Firefox issue by updating it's flex property to be 0 0 auto so the input won't try to fill the available space.
  • Updated the handleChange method to check if the param value is undefined. If it is set the value to be event.target.value and then update the components state. Now the component will be aware of any value changes from the keyboard.
  • Removed the handler binding in the constructor since all the handler methods are using arrow function I figured the binding wasn't needed.
  • Added a disabled property to the down arrow button that disables the button if the input's value and minValue are equal.

Testing

  • Create an order with multiple quantities of the same variant / option
  • As an Admin begin to process the order
  • Open the individual item processing flow by clicking the product image inside the processing dashboard
  • Attempt to change the quantity by typing a number in the input box

Notes
RC test passed
Was able to complete acceptance test

nnnnat added some commits Feb 22, 2018

fix: handleChange method wasn't dealing with value changes coming fro…
…m the keyboard I've updated the method to grab the value from event.target if the param value is undefinded
fix: updated handleChange method to check if param value is undefined…
… to prevent false positives when param value is 0
refactor: added a disabled property to the down arrow button that dis…
…ables the button if the state.value and state.minValue are equal

@nnnnat nnnnat requested a review from kieckhafer Feb 22, 2018

@spencern

This comment has been minimized.

Copy link
Member

commented Feb 22, 2018

@nnnnat @Akarshit I'd like to make sure that this currency input is using the same component that was added/fixed in #3808

@kieckhafer
Copy link
Member

left a comment

This seems to work if I highlight a number and replace it, however, if I delete the number, prior to adding a new one, I still see the NaN error:

basic_reaction_product

nnnnat added some commits Feb 23, 2018

refactor: updated numberTypeInput to use the TextField component inst…
…ead of an html number input, updated numberTypeInput styles to work with the TextInput component
refactor: updated TextField components get value method to not return…
… an empty string if the props.value is the number 0
refactor: updated TextField component to take min & max props for num…
…ber type inputs, updated numberTypeInput to better deal with different value returns
refactor: cleaned up numberTypeInput code removed console.logs added …
…in comments, updated invoiceConatiner to not push an editedItem if the refundQauntity is 0
@nnnnat

This comment has been minimized.

Copy link
Member Author

commented Feb 24, 2018

@kieckhafer I was able to resolve the issue, the component won't send any NaNs back to the form now and it will correct invalid values (empty strings, string, negative numbers, etc).

@spencern I was able to replace the NumberTypeInput component's HTML input with the updated TextField component, I had to update the TextField slightly to deal with returning the number 0 and accepting a min/max input attribute.

I also updated the invoceConatiner to make sure the refundQuantity wasn't 0 before pushing the new total because of a weird false positive that would display a refunded amount of $0.00.

Everything seems to be working still, was able to do all acceptance test and all RC test pass.

@kieckhafer

This comment has been minimized.

Copy link
Member

commented Feb 27, 2018

Changes look good, everything seems to work now 👍

@aaronjudd aaronjudd changed the base branch from master to release-1.8.2 Feb 27, 2018

@spencern spencern merged commit bb13972 into release-1.8.2 Feb 27, 2018

4 checks passed

Codacy/PR Quality Review Good work! A positive pull request.
Details
WIP ready for review
Details
ci/circleci Your tests passed on CircleCI!
Details
security/snyk No dependency changes
Details

@spencern spencern deleted the fix-3702-nnnnat-order-refunding branch Feb 27, 2018

@spencern spencern referenced this pull request Feb 28, 2018

Merged

Release 1.8.2 #3855

@spencern spencern referenced this pull request Mar 9, 2018

Merged

Release 1.9.0 #3941

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.