Implement order confirmation page #430

Starotitorov · 2018-10-30T10:04:35Z

This PR is a:

[x] New feature
[ ] Enhancement/Optimization
[ ] Refactor
[ ] Bugfix
[ ] Test for existing code
[ ] Documentation

Summary

Implement order confirmation page #267

Additional information

vercel · 2018-10-30T10:04:39Z

This pull request is automatically deployed with Now.
To access deployments, click Details below or on the icon next to each push.

coveralls · 2018-10-30T10:06:29Z

Coverage increased (+0.4%) to 16.012% when pulling 313bd4c on feature/order-confirmation-page into 83fcd76 on release/2.0.

Starotitorov · 2018-11-01T12:01:41Z

@zetlen, @jimbo I grouped all code related to order confirmation page by file type in the last commit , so you can compare both approaches. @zetlen I need to know what do you think about grouping code by features. I received an email with a response on this question from @jimbo, so I am waiting for the response from you.

zetlen · 2018-11-05T19:21:17Z

@Starotitorov Thank you for your patience with this pull request. Because it is large, and the scope of the changes is more than just order confirmation page implementation, it's difficult to review. I've tested the UI flow itself and it looks great, but there are some discrepancies in the changeset:

There seem to be many whitespace changes that Prettier should have caught. I'll push a change to fix them.
There are conflicts with the base branch, since our review has taken a long time. I'll merge and push a change to fix that as well.
There appear to be changes unrelated to order confirmation in the changeset. I am open to talking about a restructure of our client-side logic to group by features, but not in the same pull request as a feature implementation meant to fulfill a user story. Changes of that size and scope have to be independent, so we can track, discuss, and adjust them separately from a particular story or feature.

After I've pushed some changes, I'll add a more complete review.

zetlen

This pull request is full of interesting ideas. I'm not criticizing the ideas themselves when I say that the pull request is not in an acceptable state. As a team, we should be clearer about our expectations surrounding pull requests--we seem to be relying on unspoken assumptions and we owe it to you to clarify them. Nevertheless: it is an industry-wide courtesy to limit pull requests to the narrowest scope that meets acceptance criteria.

A pull request called "Implement order confirmation page" must not contain significant work apart from implementing the order confirmation page. It should not include a new routing concept unless strictly necessary to reach the acceptance criteria of the story. We can see that it's not strictly necessary to add routing in order to make a "Create Account" button which displays a CreateAccount form, because it was not necessary to do so in #293, which is already merged and functional. So it was inappropriate to include it.

You may have legitimate objections to the implementation in #293 or anything else that is merged to the default branch. You may have an idea for a better pattern. That is normal, and welcome. I've encouraged you and the rest of the EPAM team to open issues and pull requests for architectural concerns. However, this pull requests is the wrong venue for an architecture change. Those must be evaluated independently to accurately judge their effect. Yet when I load the deployed instance of this branch and attempt to do UAT, I can't tell what behavior comes from the feature, what comes from the reorganization, and what comes from the routing change. Our team priority is to review and merge a contribution to resolve #267.

Furthermore, this practice is unfair to your other proposals. In order to consider your ideas clearly and neutrally, we need to see them by themselves. When you combine a proposed refactor, a proposed code reorganization, and a new feature into a single changeset, you inadvertently obscure your own ideas. You introduce more regressions and you cause delays in review,

Your PR asks code reviewers to distinguish between your three separate intentions as they read each line of code. As I read it, I had to frequently ask myself "Is this code part of the organization-by-feature, part of the routing change, or part of the order confirmation page?" This slowed my review down by a factor of three, which is the number of separate purposes in this changeset that I can identify. Your PR has compelled me to examine lower-priority code changes before I can review the feature itself for completeness. Furthermore, some of the functionality isn't even working--the routed CreateAccount page has validation problems that the original does not.

So I can't approve this pull request; in fact, I recommend closing this pull request and reorganizing your changeset into three separate branches. I recommend that you first create a branch starotitorov/order-confirmation-page with the minimal order confirmation changeset which adheres to current code conventions, and open a new PR for that.

Before that PR is even merged, you can branch from it: git checkout -b starotitorov/organize-venia-by-feature starotitorov/order-confirmation-page. In that branch, you can add code reorganization by feature. When we merge starotitorov/order-confirmation-page, the changeset in starotitorov/organize-venia-by-feature will get smaller automatically, because the default branch has "caught up". Queueing pull requests by individual idea in this way can keep you unblocked even when code review is delayed. If a pull request in your queue is rejected, you may have to do a difficult rebase, but at least your other ideas are preserved in a separate branch. But perhaps most importantly, a queue of minimal changesets is much easier for code reviewers to think about and help you. Please create minimal PRs and queue larger ideas in downstream branches from now on.

I know you have been waiting ten days for this, which feels like a long time in your process. But this delay comes directly from the choices you made in this PR. I know because I have been guilty of this mistake myself--and on this very project, in the year 2018. In my pull request:

I included multiple features that I considered to be related
I added over 8000 lines of code
It took 41 days to merge

So this kind of mistake is not rare, and I'm sure you can correct it quickly. I'm marking this PR as "Request changes", and if you wish you can reduce the changes to the minimal order confirmation page, but I do recommend closing the PR and opening a new one from a fresh branch.

Thanks again for all your effort to improve our project, all your creative thinking, and your patience. I hope you can change your practices for the next submission. Let me know if you have any questions.

packages/venia-concept/venia-upward.yml

zetlen · 2018-11-06T22:48:12Z

pwa-devdocs/src/_drafts/venia-pwa-concept/component/order-confirmation/index.md

@@ -0,0 +1,22 @@
+---


I mentioned this before in a prior technical review meeting when I saw you had written pwa-devdocs in this pull request. Thank you for your diligence! However, in our process, we don't write documentation directly in pwa-devdocs. The fault here is ours for failing to communicate that policy clearly to you, so I've just opened #463 to add clarity.

@zetlen, the pull request with this changes was approved by James Calcaben #404. We definitely need to clarify this.

https://github.com/magento-research/pwa-studio/wiki/Documentation-Guidelines According to the documentation guidelines, pwa-devdocs/src/_drafts is preferred folder to put docs in. Could you, please, update this document in case there are any changes.

@zetlen, @jcalcaben Could you, please, update documentation guidelines according to the new requirements to writing docs.

@Starotitorov We have updated those guidelines here..

Sorry, @zetlen, I guess you need to discuss this with @jcalcaben, because initially he approved this pull requests.

packages/venia-concept/src/util/queryStringHelpers.js

zetlen · 2018-11-06T23:26:41Z

packages/venia-concept/src/actions/checkoutReceipt/asyncActions.js

+
+    await dispatch(resetCheckout());
+
+    history.push(`/create-account${objectToQueryString(accountInfo)}`);


You can replace this with:

history.push(`/create-account?${new URLSearchParams(accountInfo)}`);

But perhaps more importantly, when I click the Create Account button from the confirmation page, the form never validates--the submit button stays gray. When I examine the state of the form, the inputs are unbound. This does not happen when I go through the navigation menu.

The bug is related to our common Input component, I have already create an issue on github #416 . Our create account form in navigation drawer has exactly the same validation bug.

Steps to reproduce https://magento-venia.now.sh/:

Open navigation drawer.

Fill email field in sing in form.

Press create account button.

Email field should be prefilled.

Fill other form fields.

You can see that create account button is disabled.

packages/venia-concept/src/components/CreateAccountPage/helpers.js

packages/venia-concept/src/index.css

packages/peregrine/src/Router/Router.js

packages/venia-concept/src/actions/checkout/asyncActions.js

packages/venia-concept/src/actions/user/asyncActions.js

packages/venia-concept/src/reducers/checkoutReceipt.js

Starotitorov · 2018-11-07T08:36:37Z

@zetlen, thank you for the code review. Actually, Each feature has it separate branch, this branch was merged into feature/order-confirmation-page, I opened a pull request for each branch. This pull requests was not reviewed, we was blocked, we needed to proceed working on the feature, so I had to merge that pull requests.

Starotitorov · 2018-11-07T08:55:32Z

@zetlen, so this branch is a branch for all code related to implementation of order confirmation story, in thjis story I affected routing, create account form and added code related only to order confirmation page. When you rebase branches on each other, you build a big chain and it becomes difficult to work on the feature taking into account the fact that pull requests for each separate branches are not reviewed quickly.

Starotitorov · 2018-11-07T09:06:29Z

@zetlen our create account form in navigation drawer has exactly the same validation bug, you reproduce this using the next steps https://magento-venia.now.sh/:

Fill email field in sing in form.
Press create account button.
Email field should be prefilled.
Fill other form fields.
You can see that create account button is disabled.

I create an issue on github. The problem is with handling initial values in our common Input component #416

Starotitorov · 2018-11-08T16:46:07Z

@zetlen, @jimbo, I resolved merge conflicts today.

* Connect order confirmation page to Redux * Add tests to checkoutReceipt redux domain * Add handle create account test

…#404)

Starotitorov · 2018-11-15T11:34:21Z

I moved code to #505

jimbo

@Starotitorov Great work updating this PR in response to feedback. 👍

I've left some more comments now that I've had a detailed look. I will also try to set up stylelint next sprint to help codify some of our CSS conventions.

packages/venia-concept/src/actions/checkout/asyncActions.js

packages/venia-concept/src/actions/checkoutReceipt/asyncActions.js

packages/venia-concept/src/components/Checkout/Receipt/receipt.css

packages/venia-concept/src/components/Checkout/Receipt/receipt.js

packages/venia-concept/src/components/MiniCart/miniCart.js

packages/venia-concept/src/index.css

Starotitorov · 2018-11-15T19:03:55Z

@jimbo thanks for the review! Can I address this review comments in pull request #505, in one of the comments James asked to close this pull request and create a new one?

Starotitorov requested review from zetlen and jimbo October 30, 2018 10:04

Starotitorov force-pushed the feature/order-confirmation-page branch 5 times, most recently from 036e92b to 5a1f9e3 Compare November 1, 2018 11:48

Starotitorov force-pushed the feature/order-confirmation-page branch 2 times, most recently from 447bb9d to 724161f Compare November 1, 2018 15:03

Starotitorov force-pushed the feature/order-confirmation-page branch from cddd262 to 137218c Compare November 6, 2018 10:45

zetlen suggested changes Nov 7, 2018

View reviewed changes

zetlen force-pushed the feature/order-confirmation-page branch from a403111 to 0894118 Compare November 7, 2018 15:43

Starotitorov force-pushed the feature/order-confirmation-page branch 2 times, most recently from 89bd2f0 to fd1ffb4 Compare November 8, 2018 15:28

magento deleted a comment from PWAStudioBot Nov 8, 2018

Starotitorov force-pushed the feature/order-confirmation-page branch 7 times, most recently from b564e89 to 2e452d7 Compare November 13, 2018 12:06

Starotitorov changed the base branch from release/2.0 to feature/create-account-page November 13, 2018 12:07

Starotitorov force-pushed the feature/order-confirmation-page branch from 2e452d7 to 55cb996 Compare November 13, 2018 12:13

magento deleted a comment from PWAStudioBot Nov 13, 2018

Starotitorov force-pushed the feature/order-confirmation-page branch 4 times, most recently from dcbd624 to cbfbc74 Compare November 14, 2018 15:16

Starotitorov changed the base branch from feature/create-account-page to release/2.0 November 14, 2018 15:18

Starotitorov force-pushed the feature/order-confirmation-page branch 8 times, most recently from cc9b3d4 to b83cc68 Compare November 15, 2018 10:48

AlexeyKaryka and others added 3 commits November 15, 2018 13:51

Implement order confirmation page component

aa4375b

Connect order confirmation page to Redux (#371)

1214b8c

* Connect order confirmation page to Redux * Add tests to checkoutReceipt redux domain * Add handle create account test

Add draft docs for purchase history and order confirmation components (…

b130be0

…#404)

Starotitorov force-pushed the feature/order-confirmation-page branch from b83cc68 to 4600f50 Compare November 15, 2018 11:00

Finish order confirmation page

313bd4c

Starotitorov force-pushed the feature/order-confirmation-page branch from 4600f50 to 313bd4c Compare November 15, 2018 11:08

Starotitorov closed this Nov 15, 2018

jimbo reviewed Nov 15, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement order confirmation page #430

Implement order confirmation page #430

Starotitorov commented Oct 30, 2018 •

edited

Loading

vercel bot commented Oct 30, 2018

coveralls commented Oct 30, 2018 •

edited

Loading

Starotitorov commented Nov 1, 2018

zetlen commented Nov 5, 2018

zetlen left a comment

zetlen Nov 6, 2018

Starotitorov Nov 9, 2018

Starotitorov Nov 9, 2018 •

edited

Loading

Starotitorov Nov 12, 2018

zetlen Nov 12, 2018

Starotitorov Nov 14, 2018

zetlen Nov 6, 2018

zetlen Nov 7, 2018

Starotitorov Nov 7, 2018 •

edited

Loading

Starotitorov commented Nov 7, 2018

Starotitorov commented Nov 7, 2018

Starotitorov commented Nov 7, 2018 •

edited

Loading

Starotitorov commented Nov 8, 2018 •

edited

Loading

Starotitorov commented Nov 15, 2018

jimbo left a comment

Starotitorov commented Nov 15, 2018 •

edited

Loading


		await dispatch(resetCheckout());

		history.push(`/create-account${objectToQueryString(accountInfo)}`);

Implement order confirmation page #430

Implement order confirmation page #430

Conversation

Starotitorov commented Oct 30, 2018 • edited Loading

This PR is a:

Summary

Additional information

vercel bot commented Oct 30, 2018

coveralls commented Oct 30, 2018 • edited Loading

Starotitorov commented Nov 1, 2018

zetlen commented Nov 5, 2018

zetlen left a comment

Choose a reason for hiding this comment

zetlen Nov 6, 2018

Choose a reason for hiding this comment

Starotitorov Nov 9, 2018

Choose a reason for hiding this comment

Starotitorov Nov 9, 2018 • edited Loading

Choose a reason for hiding this comment

Starotitorov Nov 12, 2018

Choose a reason for hiding this comment

zetlen Nov 12, 2018

Choose a reason for hiding this comment

Starotitorov Nov 14, 2018

Choose a reason for hiding this comment

zetlen Nov 6, 2018

Choose a reason for hiding this comment

zetlen Nov 7, 2018

Choose a reason for hiding this comment

Starotitorov Nov 7, 2018 • edited Loading

Choose a reason for hiding this comment

Starotitorov commented Nov 7, 2018

Starotitorov commented Nov 7, 2018

Starotitorov commented Nov 7, 2018 • edited Loading

Starotitorov commented Nov 8, 2018 • edited Loading

Starotitorov commented Nov 15, 2018

jimbo left a comment

Choose a reason for hiding this comment

Starotitorov commented Nov 15, 2018 • edited Loading

Starotitorov commented Oct 30, 2018 •

edited

Loading

coveralls commented Oct 30, 2018 •

edited

Loading

Starotitorov Nov 9, 2018 •

edited

Loading

Starotitorov Nov 7, 2018 •

edited

Loading

Starotitorov commented Nov 7, 2018 •

edited

Loading

Starotitorov commented Nov 8, 2018 •

edited

Loading

Starotitorov commented Nov 15, 2018 •

edited

Loading