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 #3518: AddressBook to react #4054

Merged
merged 61 commits into from Apr 4, 2018

Conversation

7 participants
@Akarshit
Copy link
Contributor

commented Mar 22, 2018

Resolves #3518
Impact: major
Type: feature|performance

Issue

The Address review workflow had to be redesigned.

Solution

Whenever the client enters address that can't be validated by Avalara, show client a warning and give a chance to correct the address.
Also did performance optimizations for preventing hitting the Avalara API many times and prevented re-rendering of the AddressBook, because of fake updates to the Cart.

Testing

Test without tax

  1. Login as admin
  2. But an item and go to checkout screen
  3. Try adding multiple addresses, changing default shipping and billing address
  4. Try editing already added address
  5. Everything should work.

Test addressBook in profile

  1. Login and go to profile section.
  2. Try repeating above steps.

Test with Avalara

  1. Enable Avalara
  2. Enable address validation
  3. Enable tax calculation.
  4. Add an item to cart and begin to checkout
  5. For address enter RC office address
  6. Observe it takes you to the review screen.
  7. Click on Edit entered address, it should take you back to the edit address screen.
  8. Again save the address with same mistakes.
  9. It should take you to the review screen again.
  10. Select the Suggested Address(should be selected by default)
  11. Click on Continue with selected address.
  12. It should save that address, and tax should be calulated.
  13. Again repeat the above process, but on the review screen select the Entered Address
  14. It should save the address but tax won't be calculated.
  15. After placing the order, login as admin(if you weren't).
  16. Go to the Orders from Action Menu, click on the above order, you should see it labelled Tax not collected.

nnnnat added some commits Mar 8, 2018

refacotr: created an AddressBook react component, exporting the new r…
…eact component in the client index, created a helper method on the profile blaze template to include the new react AddressBook, calling the new AddressBook component from the profile blaze html
refactor: got both the profile and the checkout pulling in the old bl…
…aze addressBook and the new React AddressBook components
refactor: created an AddressBookContainer and renamed the addressbook…
… component to addresssbookform, cleaned up AddressBookForm component
refactor: addressbookContainer now has the accountId and addessBook o…
…n props, passing addressBook to AddressBookGrid and rendering the address in the new react component
refactor: chaning shipping/billing address from AddressBookGrid now w…
…orks, removing address from AddressBookGrid now working, created a few helper methods in the the old addressBook blaze template will move them soon
refactor: addressBookGrid edit buttons open addressBookForm and add t…
…he edit address to the forms props, some clean up and comments
refacotr: addressbookform now has most fields and can add an address …
…to the address book, addresbookcontiner caan now add and address and handles the form gird toggle a little better, created a addAddress helper function that calls the meteor method
refactor: added defaultAddress to the AddressBookGrid state, this is …
…used to make the default billing and shipping selectiion appear faster than it is
refactor: addressBookGrid now updates it's defaultAddress state if a …
…new address is added and selected as the new default address
refactor: AddressBookForm region options working properly if country …
…changes, regiion field switchs from select to textField if no region options found
refactor: reated new addressbookcontainer and moved props and ruder m…
…ethods to the continer and out of the blaze template, AddressBook now working on profile page, clean up still needed
refactor: updated AddressBook component, clean up PropTypes and commm…
…ents, created a getter for the addressBook array that protects against possible null value

@Akarshit Akarshit changed the title [WIP] Fix #3518: address book review Fix #3518: AddressBook to react Mar 26, 2018

@Akarshit Akarshit requested a review from jshimko Mar 26, 2018

@Akarshit Akarshit changed the base branch from refactor-2053-nnnnat-address-book-react to master Mar 26, 2018

@Akarshit Akarshit changed the base branch from master to release-1.11.0 Mar 26, 2018

Akarshit added some commits Mar 26, 2018

@Akarshit

This comment has been minimized.

Copy link
Contributor Author

commented Mar 27, 2018

@rymorgan When tax is not calculated for an order, we show a badge saying "Tax not calculated" in the order summary for the admin to see.
The text is too long, so I thought maybe you would have a better design.
Also you once suggested to add High Risk tag for this, but "tax calculation failed" doesn't make a order high risk, so it might be better to show admin something more appropriate than High Risk.

@rymorgan

This comment has been minimized.

Copy link
Member

commented Mar 27, 2018

@Akarshit - Just to be clear I never suggested showing "high risk," I suggested that we use the same badge component/style to show that Tax had not been calculated. I was showing that high risk as an example of how we should handle it visually. Also, see below, I think this will fit fine. Unless there's something about this that I am missing. Please note, there's also a dot notification on the order in the sortable table.

orders - high risk flag copy

@rymorgan

This comment has been minimized.

Copy link
Member

commented Mar 27, 2018

In generally the UX here is a big improvement but we're missing some styling. See Cassy's style here for details on how it should look here:
#3518 (comment)

The text on the warning inline alert should be: @rui-warning-text and may need be corrected in the actual inline alert component as I've seen this multiple times.

See attached for missing/wrong styles:
avalara validation

@Akarshit

This comment has been minimized.

Copy link
Contributor Author

commented Mar 27, 2018

@rymorgan Oh, then I would have misunderstood you. I will implement the correct styling.

@Akarshit

This comment has been minimized.

Copy link
Contributor Author

commented Mar 28, 2018

@rymorgan I was trying to find the styles for these elements but was unable to do so.
I found @rui-warning-text and @rui-warning-bg but am not sure how to style the "inline red alert". Is there some ready-made class that I am supposed to use?

@jshimko jshimko referenced this pull request Mar 28, 2018

Merged

Shippo address validation #4086

@kieckhafer

This comment has been minimized.

Copy link
Member

commented Mar 28, 2018

@Akarshit to get the red alerts, replace warning with danger in all instances.

@rymorgan

This comment has been minimized.

Copy link
Member

commented Mar 28, 2018

for the entered address validation alert it should be these styles:
danger-alert

@jshimko
Copy link
Contributor

left a comment

There are a ton of PropTypes.String and PropTypes.Bool instances that should all be lowercase. As in PropTypes.string and PropTypes.bool.
https://reactjs.org/docs/typechecking-with-proptypes.html

let address = this.props.validationResults.enteredAddress;
if (!this.state.isEnteredSelected) {
address = this.props.validationResults.suggestedAddress;
Meteor.call("accounts/markAddressValidationBypassed", false, (error) => {

This comment has been minimized.

Copy link
@jshimko

jshimko Mar 29, 2018

Contributor

Ideally, any business logic like this should be passed in by a container component. UI components should always be presentational and not concerned with how the server interaction happens.

So for example, handleSelection should probably get passed this.props.onHandleSelection (or something like that).

// If the view template that's using the AddressBook has a
// heading object, set it as the AddressBook heading
if (!this.heading) {
const template = Template.instance();

This comment has been minimized.

Copy link
@jshimko

jshimko Mar 29, 2018

Contributor

Are we using Blaze API's here?

This comment has been minimized.

Copy link
@Akarshit

Akarshit Mar 29, 2018

Author Contributor

We pass down the data from the Blaze template. That's all I know at-least.

}

const wrapComponent = (Comp) => (
class AddressBookContainer extends Component {

This comment has been minimized.

Copy link
@jshimko

jshimko Mar 29, 2018

Contributor

Is this wrapComponent necessary? You should probably use the withProps HOC from recompose. That's the pattern we've generally adopted for passing in handler functions.

import { withProps } from "recompose";

const handlers = {
  addAddress,
  updateAddress,
  removeAddress,
  onError
};

registerComponent("AddressBook", AddressBook, [
  composeWithTracker(composer),
  withProps(handlers)
]);

export default compose(
  composeWithTracker(composer),
  withProps(handlers)
)(AddressBook);
} else {
Avalogger.error("Unknown error or error format");
Logger.error("Unknown Error", error);

This comment has been minimized.

Copy link
@jshimko

jshimko Mar 29, 2018

Contributor

Non-string objects should always be the first arg for Logger methods. Otherwise, they aren't parsed properly.

if (result.error.type === "apiError") {
// If we have a problem with the API there's no reason to tell the customer
// so let's consider this unvalidated but move along
Logger.error("API error, ignoring address validation");
Logger.info("API error, ignoring address validation");

This comment has been minimized.

Copy link
@jshimko

jshimko Mar 29, 2018

Contributor

Do we really want this to be info? This is a real error after all. It's really easy to miss info logs sometimes and I'd want to know if API calls to Avalara were failing.

_id: PropTypes.String,
fullName: PropTypes.String,
address1: PropTypes.String,
addresss2: PropTypes.String,

This comment has been minimized.

Copy link
@jshimko

jshimko Mar 29, 2018

Contributor

Typo... addresss2.

All of these String and Bool prop checks should be lowercase string and bool.

})),
/**
* selects an address to be edited and renders the AddressBookForm
*/

This comment has been minimized.

Copy link
@jshimko

jshimko Mar 29, 2018

Contributor

Not a big deal, but these single line comments should probably just be // instead of the multiline comment syntax.

{this.renderAddress(address)}
</div>
<div className="controls">
<button className="btn btn-default" onClick={() => { edit(_id); }}>

This comment has been minimized.

Copy link
@jshimko

jshimko Mar 29, 2018

Contributor

These should be <Components.Button>

});
}

registerComponent("AddressBookContainer", AddressBook, [

This comment has been minimized.

Copy link
@jshimko

jshimko Mar 29, 2018

Contributor

This should be named AddressBook (without the Container in the name). That decision was originally made because it's potentially a little confusing when you do something like...

replaceComponent("AddressBookContainer", CustomAddressBook);

That isn't actually replacing the container. It's replacing the component and inheriting the same container(s). So anything that gets registered and added to Components with registerComponent should have the name of the UI component.

*/
AddressBook() {
return {
component: Components.AddressBookContainer

This comment has been minimized.

Copy link
@jshimko

jshimko Mar 29, 2018

Contributor

See other comment about component name not having Container in it.

Template.addressBookAdd.onCreated(function () {
this.currentCountry = new ReactiveVar(null);
// hit Reaction's GeoIP server and try to determine the user's country
HTTP.get("https://geo.getreaction.io/json/", (err, res) => {

This comment has been minimized.

Copy link
@jshimko

jshimko Mar 29, 2018

Contributor

This HTTP call was hitting our GeoIP server to get the country the user is currently in and automatically set their country in the select field. It looks like this got ditched completely. Do we have a suitable alternative to maintain that UX?

This comment has been minimized.

Copy link
@Akarshit

Akarshit Mar 30, 2018

Author Contributor

@jshimko No there wasn't. Now I have added it.

@Akarshit

This comment has been minimized.

Copy link
Contributor Author

commented Mar 30, 2018

Working to incorporate all these changes.

Akarshit added some commits Mar 30, 2018

@Akarshit

This comment has been minimized.

Copy link
Contributor Author

commented Mar 30, 2018

@jshimko updated the PR with the fixes.

@spencern spencern merged commit 489c8f3 into release-1.11.0 Apr 4, 2018

9 checks passed

WIP ready for review
Details
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: docker-build Your tests passed on CircleCI!
Details
ci/circleci: docker-push Your tests passed on CircleCI!
Details
ci/circleci: dockerfile-lint Your tests passed on CircleCI!
Details
ci/circleci: eslint Your tests passed on CircleCI!
Details
ci/circleci: test-app Your tests passed on CircleCI!
Details
ci/circleci: test-unit Your tests passed on CircleCI!
Details
security/snyk No new issues
Details

@spencern spencern deleted the fix-3518-akarshit-address-book branch Apr 4, 2018

@spencern spencern referenced this pull request Apr 5, 2018

Merged

Release 1.11.0 #4151

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.