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

Use query parameters when loading checkout page #283

Merged
merged 7 commits into from May 21, 2019

Conversation

noisecapella
Copy link

@noisecapella noisecapella commented May 14, 2019

Pre-Flight checklist

  • Testing
    • Code is tested
    • Changes have been manually tested

What are the relevant tickets?

Fixes #121

What's this PR do?

Uses query parameters to populate the basket on checkout page load. This will allow links from the catalog pages to the checkout page.

How should this be manually tested?

Go to /checkout/?product_id=123456 where 123456 is a valid ProductVersion id. You should see that page loaded. If you go to ?product_id=123456&code=xyz for a valid coupon code, it should automatically apply the discount and show it on the checkout page.

If you have no query parameters, it should show you whatever your basket looked like previously.

If you have an invalid product id, the page should load with whatever the old basket content was and an error message should be displayed at the bottom of the screen. This is a rare case so the error message won't be more prominent than that.

If you have an invalid coupon code, the page should try again to patch the basket with the given product id from the query parameter and an error message should be displayed at the bottom of the screen, near the coupon code textbox.

@odlbot odlbot temporarily deployed to xpro-ci-pr-283 May 14, 2019 18:44 Inactive
@codecov-io
Copy link

codecov-io commented May 15, 2019

Codecov Report

Merging #283 into master will increase coverage by 0.08%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #283      +/-   ##
==========================================
+ Coverage   94.12%   94.21%   +0.08%     
==========================================
  Files         160      160              
  Lines        5400     5458      +58     
  Branches      312      312              
==========================================
+ Hits         5083     5142      +59     
+ Misses        261      260       -1     
  Partials       56       56
Impacted Files Coverage Δ
static/js/containers/pages/CheckoutPage_test.js 100% <100%> (ø) ⬆️
static/js/lib/ecommerce.js 100% <100%> (ø) ⬆️
static/js/containers/pages/CheckoutPage.js 100% <100%> (+1.44%) ⬆️
static/js/lib/form_test.js 100% <100%> (ø) ⬆️
static/js/lib/form.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 08d7574...1437846. Read the comment docs.

@noisecapella noisecapella temporarily deployed to xpro-ci-pr-283 May 15, 2019 16:08 Inactive
@noisecapella noisecapella temporarily deployed to xpro-ci-pr-283 May 15, 2019 21:07 Inactive
@noisecapella noisecapella temporarily deployed to xpro-ci-pr-283 May 15, 2019 21:09 Inactive
@noisecapella noisecapella temporarily deployed to xpro-ci-pr-283 May 16, 2019 14:52 Inactive
@noisecapella noisecapella temporarily deployed to xpro-ci-pr-283 May 16, 2019 15:05 Inactive
@noisecapella noisecapella temporarily deployed to xpro-ci-pr-283 May 16, 2019 15:39 Inactive
@noisecapella noisecapella temporarily deployed to xpro-ci-pr-283 May 16, 2019 17:10 Inactive
@noisecapella noisecapella temporarily deployed to xpro-ci-pr-283 May 16, 2019 18:01 Inactive
@gsidebo gsidebo self-assigned this May 17, 2019
: null
)
})
}
Copy link
Contributor

Choose a reason for hiding this comment

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

One comment off the top: I would rather have multiple test cases with some repeated code than a parametrized test case with this many lines and this many logical paths to reason about

Copy link
Contributor

@gsidebo gsidebo May 17, 2019

Choose a reason for hiding this comment

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

...actually I changed my mind. I think this can be cleaned up in a way that makes the parametrization a little more readable and maintainable

describe("mount", () => {
    const productError = "product error",
      couponError = "coupon error",
      couponCode = "codeforcoupon",
      productId = 12345
    let couponPayload,productPayload

    beforeEach(() => {
      couponPayload = {
        body: {coupons: [{code: couponCode}]},
        credentials: undefined,
        headers: {
          "X-CSRFTOKEN": null
        }
      }
      productPayload = {
        body: {items: [{id: productId}]},
        credentials: undefined,
        headers: {
          "X-CSRFTOKEN": null
        }
      }
    })

    //
    ;[
      [true, false, couponError],
      [true, true, null],
      [false, false, productError],
      [false, true, productError]
    ].forEach(
      ([hasValidProductId, hasValidCoupon, expError]) => {
        it(`...`, async () => {
          if (!hasValidProductId) {
            helper.handleRequestStub
              //...
              .returns({
                //...
                body: {
                  errors: productError
                }
              })
          }
          if (!hasValidCoupon) {
            helper.handleRequestStub
              //...
              .returns({
                //...
                body: {
                  errors: couponError
                }
              })
          }
          //...

          assert.equal(
            helper.handleRequestStub.calledWith(
              "/api/basket/",
              "PATCH",
              couponPayload
            ),
            hasValidProductId
          )

          assert.equal(
            inner.state().errors,
            expError
          )
        })
      }
    )
  })


const couponCode = params.code
if (couponCode) {
await this.updateBasket({ coupons: [{ code: couponCode }] })
Copy link
Contributor

Choose a reason for hiding this comment

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

If this isn't already possible, can this query/endpoint be rewritten to accept items and coupons in the same call? Executing 2 requests seems unnecessary here

Copy link
Author

Choose a reason for hiding this comment

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

This is a little tricky because the behavior we want is to update items then update coupons. If the coupons update fails the items update should still succeed. If both were sent as the same PATCH request and the coupons had a validation error but the items succeeded, I think the proper thing is to not make any changes and return a 400 error with an error message in the payload. What do you think?

@@ -57,6 +58,24 @@ const formatDateForRun = (dateString: ?string) =>
export const formatRunTitle = (run: CourseRun) =>
`${formatDateForRun(run.start_date)} - ${formatDateForRun(run.end_date)}`

export const formatErrors = (errors: string | Object) => {
if (!errors) {
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. Seems like this logic will be needed outside of ecommerce. Might be better to define it in static/js/lib/form.js, static/js/lib/api.js, etc.
  2. A return type should be added to this function
  3. This should have some tests, right? Especially since (1) we may end up enforcing some standards around how errors are returned in back end responses, and (2) it's likely that we'll want to make changes to the JSX that this produces
  4. If this function can only accept string and Object types, I don't think !errors can ever evaluate to true. We might want to use emptyOrNil if errors can be an empty object, empty string, or null/undefined

}
)
// wait for componentDidMount to resolve
await wait(15)
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like there's no nice way to avoid this, but I'm wondering about option 2 in this GH post: enzymejs/enzyme#1587 (comment). If that seems to work, I think I'd prefer it over the wait

@noisecapella noisecapella temporarily deployed to xpro-ci-pr-283 May 20, 2019 16:03 Inactive
@noisecapella
Copy link
Author

Ok, I think that's everything


import queries from "../../lib/queries"
import {
calculateDiscount,
calculatePrice,
formatPrice,
formatRunTitle
formatRunTitle,
formatErrors
Copy link
Contributor

Choose a reason for hiding this comment

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

This function doesn't exist in lib/ecommerce so this is breaking webpack

"PATCH",
couponPayload
)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer this if/else be changed to a single assert statement (assert.equal(helper.handleRequestStub.calledWith(...), hasValidProductId)) but you can take or leave that

@noisecapella noisecapella temporarily deployed to xpro-ci-pr-283 May 20, 2019 17:59 Inactive
Copy link
Contributor

@gsidebo gsidebo left a comment

Choose a reason for hiding this comment

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

LGTM! 👍

@noisecapella noisecapella merged commit 117423b into master May 21, 2019
@noisecapella noisecapella deleted the gs/checkout_query_params branch May 21, 2019 14:20
@odlbot odlbot mentioned this pull request May 22, 2019
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Checkout page should use query parameters to populate basket
4 participants