Bug 1490727: Various blockers for release. by joshJarr · Pull Request #5005 · mdn/kuma

joshJarr · 2018-09-27T18:11:03Z

In this PR We have addressed:

Copy changes, email, social share, thank you page.
Flow for the stripe error pages (needs copy)
Stripe data fixes - we now send the name of the contributor in the metadata and set the receipt email. This should be tested with the live stripe account.
Fixes email images being loaded with the wrong domain on staging.
Lazy load checkout.js using $.getScript() on all pages upon interaction with the popover (except /contribute).
Fix banner flashing bug after it has been closed.
Fix Analytics firing when the popover has been dismissed or is not shown on mobile.

Let me know if you have any feedback or if we have missed anything, Thanks!

…inking

…ent-on-mobile Check if the popover is hidden before sending the shown event

fix error handling in make_charge function of ContributionForm

…m-by-default Make the contribution popover default to hidden to prevent it from blinking

…upon-interaction Hotfix/load stripe checkout upon interaction

jwhitlock · 2018-09-27T18:29:21Z

@joshJarr thanks for the PR. Do you want to try to fix the flake8 issue and 2 test failures?

joshJarr · 2018-09-27T18:30:34Z

@jwhitlock taking a look now 👍

joshJarr · 2018-09-27T19:17:10Z

Got a fix, will test everything and push in a hour or two.

jwhitlock

This fixes the listed blockers - nice work! There's a small change to get tests working, see https://github.com/potatolondon/kuma/pull/22.

There's a few nits that you may want to work in at the same time. We can merge and deploy in the morning.

jwhitlock · 2018-09-28T01:33:26Z

kuma/contributions/jinja2/contributions/thank_you.html

                    </h2>
+                    <p>
+                        {% trans %}
+                        Please contact <a href="mailto:support@mozilla.com">support@mozilla.com</a>  for more information on this problem.


Future task: We'll need to move the email outside of the translated string, and use a real email address (unless support@mozilla.com is the real email address).

Hi John. Yep, I believe we're still waiting on the final support email address. As of yesterday, Kadir still hadn't received this so it might fall to you guys to update once you have it.

The email address is mdn-support@mozilla.com

jwhitlock · 2018-09-28T01:39:38Z

kuma/static/js/contribution-handler.js

+     * also handles errors when getting the resource
+     */
+    function getStripeCheckoutScript() {
+        $.getScript('https://checkout.stripe.com/checkout.js')


I think if stripeHandler is initialized (not null), then the script should not be reloaded. You can see the multiple loads by watching the network tab while collapsing and expanding the banner multiple times.

Also, you should add the following config:

$.ajaxSetup({ cache: true });

If that is not set, then the script will be loaded over the network each time it is requested, instead of loading it from browser cache.

Good suggestions, thank you. Please also see my comments around error handling improvements for the script loading as in the other comment as well.

Are we too consider this feedback launch blocking or is it something we can follow up with quickly next week?

If we get this into production by Wednesday, that should be fine. Before that we'll have a very limited test audience.

jwhitlock · 2018-09-28T01:44:38Z

kuma/static/js/contribution-handler.js

     * Expands the popover to show the full contents.
     */
-    function expandCta() {
+    function expandPopover() {


I think expanding the popover deserves to be tracked in analytics.

Hi John. Sounds like a good idea.

Is this something we can follow up with next week?

Oh, good catch John!

If we get this into production by Wednesday, that should be fine. Before that we'll have a very limited test audience.

jwhitlock · 2018-09-28T01:46:51Z

kuma/contributions/forms.py

+                    metadata={'name': self.cleaned_data['name']}
+                )
+                return True
+            except Exception as e:


I'm not a fan of generic exception handlers, but this worked for the card error for 4100000000000019, so it removes the launch blocker. I can adjust the error handler later.

Please let us know if you'd like us to adjust this.

bug 1490727: Fix tests for new call

schalkneethling · 2018-09-28T09:57:40Z

kuma/static/js/contribution-handler.js

        if (stripeHandler === null) {
            // initialise handler
-            stripeHandler = initStripeHandler();
+            // stripeHandler = initStripeHandler();


@joshJarr From the diff it looks like this line is commented out. So, if stripeHandler is null nothing is going to happen. Is this intentional? If so, and this is handled somewhere else in the code, then it is probably best to completely remove this piece of code?

Hi Schalk. Yep, I think this if statement is redundant and slipped through. stripeHandler will be set as part of getStripeCheckoutScript() when the popover is expanded. We then check if it's not null in onSubmit when the form is submitted.

I think, as a further improvement to this, we might consider showing an error message if the script fails to load for whatever reason or if it hasn't loaded by the time the user clicks submit, but I think this is an adjustment we can probably follow up with next week. Thoughts?

form my point of view, if we get this into production by Wednesday, that should be fine. Before that we'll have a very limited test audience. Without knowing too much of the context, I'd only show the error message when it' necessary for payment (eg the form submission)

matthewhall · 2018-09-28T11:45:58Z

Hi @jwhitlock @schalkneethling,

Thanks for the review. Michal has pushed a fix for the tests this morning: a57c89c. Thanks for bearing with us on that.

I've added a couple of comments in to your feedback above. Please could you let me know if any of these are launch blocking? If possible, and none of these are P0s, then it might be prudent to address and quickly follow up with fixes for them post-launch. Concerned of further changes today potentially causing bugs so close to launch and would be cool to give a little more time to address them and any other fixes needed and have a day next week to test before launching.

Please let me know your thoughts. Thanks!

schalkneethling · 2018-09-28T12:02:11Z

@atopal Thoughts?

atopal · 2018-09-28T12:28:12Z

Agreed, the performance and UX issues mentioned here should be dealt with before Wednesday, but I don't see them as blockers for Monday. I'll defer to John and Schalk for anything else.

jwhitlock

Thank you for fixing the tests. We can address nits and follow-on issues in the near future.

joshJarr and others added 22 commits September 26, 2018 13:27

Make the contribution popover default to hidden to prevent it from bl…

f30e706

…inking

Check if the popover is hidden before sending the shown event

705a254

fix error handling in make_charge function of ContributionForm

9289b97

added the stripe checkout.js on modal expand

7916613

fix typo

9e39931

added name and recipt email fields

eef6070

styling for err page

eca648c

address pr feedback

54e8e2d

fix indentation

439d5ef

Address PR feedback, cleanup code

c3a7c1b

Address PR feedback

5194de4

Merge pull request #18 from potatolondon/hotfix/dont-send-analytic-ev…

21ba4f0

…ent-on-mobile Check if the popover is hidden before sending the shown event

Merge pull request #19 from potatolondon/Handle-errors-for-stripe

7e123d5

fix error handling in make_charge function of ContributionForm

rm redundant class toggle.

9c8473b

Merge branch 'master' into hotfix/hide-contribution-form-by-default

b143a8e

update tab space

7938645

Merge pull request #17 from potatolondon/hotfix/hide-contribution-for…

149d7d3

…m-by-default Make the contribution popover default to hidden to prevent it from blinking

rm legacy checkout.js

c931199

update contribute form copy

286dcd1

update share copy

80a403a

update thankyou copy

4ff0de5

Merge pull request #21 from potatolondon/hotfix/load-stripe-checkout-…

c21b48c

…upon-interaction Hotfix/load stripe checkout upon interaction

Fix handler confusion

798d601

joshJarr and others added 2 commits September 27, 2018 23:32

trying to resolve test errors

73d7917

bug 1490727: Fix tests for new call

b5b432a

jwhitlock suggested changes Sep 28, 2018

View reviewed changes

Merge pull request #22 from jwhitlock/potatolondon-fix-tests

a57c89c

bug 1490727: Fix tests for new call

schalkneethling reviewed Sep 28, 2018

View reviewed changes

jwhitlock approved these changes Sep 28, 2018

View reviewed changes

jwhitlock merged commit b6bd9f7 into mdn:master Sep 28, 2018

jwhitlock added the payments label Oct 11, 2018

Conversation

joshJarr commented Sep 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jwhitlock commented Sep 27, 2018

Uh oh!

joshJarr commented Sep 27, 2018

Uh oh!

joshJarr commented Sep 27, 2018

Uh oh!

jwhitlock left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matthewhall Sep 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matthewhall Sep 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matthewhall commented Sep 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

schalkneethling commented Sep 28, 2018

Uh oh!

atopal commented Sep 28, 2018

Uh oh!

jwhitlock left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

joshJarr commented Sep 27, 2018 •

edited

Loading

matthewhall Sep 28, 2018 •

edited

Loading

matthewhall Sep 28, 2018 •

edited

Loading

matthewhall commented Sep 28, 2018 •

edited

Loading