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

Checkout: Add thank you page for the domain only site flow #11702

Merged
merged 8 commits into from
Mar 8, 2017

Conversation

gziolo
Copy link
Member

@gziolo gziolo commented Mar 2, 2017

Implements #11452. Requires D4612-code.

This PR:

  • Creates ThankYouCard from PlanThankYouCard.
  • Refactors PlanThankYouCard to use ThankYouCard.
  • Uses ThankYouCard to display a domain-only thank you page.

Testing

PlanThankYouCard

New thank you page

  • Visit /start/domain-first?new=DOMAIN where DOMAIN is a domain like thisisaniquedomain123.live.
  • Select Just purchase a domain.
  • Purchase the domain in checkout.
  • Assert that you land on a thank you page with ThankYouCard displayed for the domain you purchased like:
    screen shot 2017-03-06 at 12 21 00

Review

  • Code
  • Copy
  • Design
  • Product

@matticbot
Copy link
Contributor

@matticbot matticbot added the [Size] XL Probably needs to be broken down into multiple smaller issues label Mar 2, 2017
@gziolo gziolo added this to the Tortuga MVP: Development milestone Mar 2, 2017
@gziolo gziolo force-pushed the update/domain-only-site-thank-you branch from 630d4c0 to 1a37150 Compare March 2, 2017 10:52
@matticbot matticbot added [Size] M Medium sized issue [Size] XL Probably needs to be broken down into multiple smaller issues and removed [Size] XL Probably needs to be broken down into multiple smaller issues [Size] M Medium sized issue labels Mar 2, 2017
@@ -234,7 +234,7 @@ module.exports = function() {

if ( config.isEnabled( 'upgrades/checkout' ) ) {
page(
'/checkout/thank-you/no-site/:receiptId',
'/checkout/thank-you/no-site/:receiptId?',
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for fixing this.

@@ -229,6 +229,17 @@ const CheckoutThankYou = React.createClass( {
);
}

if ( this.props.domainOnlySiteFlow && ! failedPurchases ) {
Copy link
Contributor

@drewblaisdell drewblaisdell Mar 2, 2017

Choose a reason for hiding this comment

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

Empty arrays are still truthy so we need to check the length here.

Copy link
Member Author

Choose a reason for hiding this comment

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

I had this locally:)

if ( this.props.domainOnlySiteFlow && failedPurchases.length === 0 && purchases.some( isDomainRegistration ) ) {

{ translate( 'Visit Your Site' ) }
</a>
</div>
<div className="plan-thank-you-card__heading">
Copy link
Contributor

@drewblaisdell drewblaisdell Mar 2, 2017

Choose a reason for hiding this comment

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

I think these styles should live in ThankYouCard instead of PlanThankYouCard because we want to use them in every (currently, both) version of the thank you card.

Unfortunately, this means we won't be able to use children which is a nice API for this component. The other options would be to:

  • Reference thank-you-card styles in plan-thank-you-card.
  • Duplicate the styles between domain-thank-you-card and plan-thank-you-card.

I don't think either of those are preferable to using props instead of props.children here. What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm perfectly fine with this approach. This was my first idea, but I quickly got distracted by the power of children :)

What you proposed fits what we need in this particular approach and removes code duplication. If it turns out we need something different inside body we can always extract this content and create another component and get back to the concept of using children.

@drewblaisdell drewblaisdell force-pushed the update/domain-only-site-thank-you branch from 2474044 to 414c641 Compare March 2, 2017 20:52
@matticbot matticbot added the [Size] XL Probably needs to be broken down into multiple smaller issues label Mar 2, 2017
heading={ this.translate( 'Thank you for your purchase!' ) }
description={ this.translate( "That looks like a great domain. Now it's time to get it all set up." ) }
buttonUrl={ domainManagementEdit( domainName, domainName ) }
buttonText={ this.translate( 'Manage Your Domain' ) }
Copy link

Choose a reason for hiding this comment

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

Hi! I've found a possible matching string that has already been translated 52 times:
translate( 'Manage Your Domains' ) ES Score: 11
See 1 additional suggestion in the PR translation status page

Help me improve these suggestions: react with 👎 if the suggestion doesn't make any sense, or with 👍 if it's a particularly good one (even if not implemented).

Copy link
Member Author

Choose a reason for hiding this comment

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

@ranh suggested in #11452 to use Go To Your Domain instead.

@drewblaisdell
Copy link
Contributor

@gziolo I think this is ready for review. :)

@drewblaisdell drewblaisdell added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress labels Mar 2, 2017
// this placeholder is using just wp logo here because two possible states do not share a common layout
if ( ! purchases && ! failedPurchases && ! this.isGenericReceipt() ) {
if ( ! purchases.length && ! failedPurchases.length && ! this.isGenericReceipt() ) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

That's personal preference but I like using isEmpty() in those cases since it makes the code reads like a sentence.


<ThankYouCard
name={ this.translate( 'Domain Registration' ) }
price={ purchase.displayPrice }
Copy link
Member Author

Choose a reason for hiding this comment

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

What about privacy protection?

@gziolo
Copy link
Member Author

gziolo commented Mar 3, 2017

@drewblaisdell thanks for finishing this PR. Everything looks awesome.

I had only two comments, one related to the different copy for the button. We also need to double check if privacy protection cost is included in the price displayed on this page.

@gziolo gziolo added [Status] Needs Author Reply [Status] Needs Copy Review Add this when you'd like to get a review / feedback from the Editorial team on your PR [Status] Needs Design Review Add this when you'd like to get a review / feedback from the Design team on your PR and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Mar 3, 2017
@gziolo gziolo requested review from ranh and danhauk March 3, 2017 07:05
@drewblaisdell drewblaisdell force-pushed the update/domain-only-site-thank-you branch from 1fb005a to 581a6d4 Compare March 7, 2017 01:46
@matticbot matticbot added the [Size] XL Probably needs to be broken down into multiple smaller issues label Mar 7, 2017
@gziolo
Copy link
Member Author

gziolo commented Mar 7, 2017

@drewblaisdell thanks for fixing the issue raised by @stephanethomas 👍
Code looks good now!

@gziolo
Copy link
Member Author

gziolo commented Mar 7, 2017

@stephanethomas can you try to reproduce the issue you discovered with the updated backend patch and client code? :)

@@ -288,10 +282,12 @@ const Checkout = React.createClass( {
}

if ( receipt && receipt.receipt_id ) {
const receiptId = receipt.receipt_id;
const receiptId = receipt.receipt_id,
displayPrice = receipt.display_price;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is where destructuring comes in handy:

const { receipt_id: receiptId, display_price: displayPrice } = receipt;

@stephanethomas
Copy link
Contributor

@gziolo: I've just tested it and had the same issue again with the Thank You page. Note this time the /me/transactions endpoint returned the display_price property, but it was equal to zero:

{
  "code": 200,
  "headers": [{
    "name": "Content-Type",
    "value": "application\/json"
  }],
  "body": {
    "success": true,
    "error_code": "",
    "error_message": "",
    "receipt_id": "22457622",
    "display_price": "$0.00",
    "purchases": {
      ...

@stephanethomas stephanethomas added [Status] Needs Author Reply and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Mar 7, 2017
@drewblaisdell drewblaisdell force-pushed the update/domain-only-site-thank-you branch from 581a6d4 to 52cd24b Compare March 7, 2017 20:08
@matticbot matticbot added the [Size] XL Probably needs to be broken down into multiple smaller issues label Mar 7, 2017
@drewblaisdell
Copy link
Contributor

@stephanethomas There was an issue where we were camelCasing keys that were expected to be raw from the API that I fixed in 52cd24b.

I tested several times with and without the privacy upgrade and never saw a display_price of "$0.00". Would you mind trying again and seeing where in the patch this fails for you? :) (We should probably move this discussion there)

@drewblaisdell drewblaisdell added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] Needs Author Reply labels Mar 7, 2017
@gziolo
Copy link
Member Author

gziolo commented Mar 8, 2017

It might be because @stephanethomas has tested it with credits. I'll double check it today with free credits :) I guess 0 price is sth expected in such case.

@gziolo
Copy link
Member Author

gziolo commented Mar 8, 2017

I see also 0 in the email receipt when paying with credits:

screen shot 2017-03-08 at 11 56 23

@gziolo
Copy link
Member Author

gziolo commented Mar 8, 2017

I tested also the case when paying with card, everything works properly. Product 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Post Checkout [Size] XL Probably needs to be broken down into multiple smaller issues [Type] Enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants