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 #3474 making validatedAddress object to prevent exception #3504

Merged
merged 2 commits into from Jan 24, 2018

Conversation

Akarshit
Copy link
Contributor

Fix for #3474

To test

  1. Run reaction
  2. Buy Example Product and go to the checkout sceen.
  3. Enter a invalid US address and click Save and Continue.
  4. There should be no exception in the console.

validatedAddress.failedValidation = false;
validatedAddress = {
failedValidation: false
};
}
}
const validationResults = { validated, fieldErrors: validationErrors, formErrors, validatedAddress };
Copy link
Collaborator

Choose a reason for hiding this comment

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

previously, this method returned the address passed to it, but with your change, it could be replaced with { failedValidation: false }. This could be fine (I have no idea what the downstream code looks like), but feels like a potentially breaking change.
To be safe, consider checking that validatedAddress is null/undefined,

// line 294:
if (!validatedAddress) {
  validatedAddress = {};
}
validatedAddress.failedValidation = false; // same as it was

Copy link
Collaborator

Choose a reason for hiding this comment

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

@pmn4 Isn't validatedAddress already being checked at line 285? This snippet is in the else clause of that check.

Copy link
Collaborator

Choose a reason for hiding this comment

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

oops! my bad. I thought this was the else for if (validator)

@zenweasel
Copy link
Collaborator

I know this isn't exactly in the scope of this bug, but it would seem to me like both places where failedValidation is set there it should be set to true. Probably some mixup when looking at validated and failedValidation which probably could have better named so as to not have similar but opposite names.

zenweasel
zenweasel previously approved these changes Jan 19, 2018
Copy link
Collaborator

@zenweasel zenweasel left a comment

Choose a reason for hiding this comment

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

This fixes this bug as far as it goes but I think we need to look at this validation again

validatedAddress.failedValidation = false;
validatedAddress = {
failedValidation: false
};
}
}
const validationResults = { validated, fieldErrors: validationErrors, formErrors, validatedAddress };
Copy link
Collaborator

Choose a reason for hiding this comment

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

@pmn4 Isn't validatedAddress already being checked at line 285? This snippet is in the else clause of that check.

Copy link
Collaborator

@zenweasel zenweasel left a comment

Choose a reason for hiding this comment

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

Tested. Verified fixed

@spencern spencern changed the base branch from master to release-1.6.6 January 24, 2018 00:34
@spencern spencern merged commit cacbda0 into release-1.6.6 Jan 24, 2018
@spencern spencern deleted the bug-3474-akarshit-tax-logic branch January 24, 2018 00:36
@spencern spencern mentioned this pull request Jan 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants