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

amp-form issue in latest release #20594

Closed
leonahliang90 opened this issue Jan 30, 2019 · 15 comments · Fixed by #20601
Closed

amp-form issue in latest release #20594

leonahliang90 opened this issue Jan 30, 2019 · 15 comments · Fixed by #20601

Comments

@leonahliang90
Copy link
Contributor

leonahliang90 commented Jan 30, 2019

What's the issue?

Based on the PR, it blocks the GET request of action
#20362

How do we reproduce the issue?

Please visit any of our ShopCart / Detail page and Click on "Buy Now" Button, the button is not responding as all the GET requests are blocked.

What browsers are affected?

All browsers

Which AMP version is affected?

we are seeing this version 1901242049580 on Production but when we are checking it from release section, this version is categorized as pre-release.

image

@leonahliang90 leonahliang90 changed the title [Urgent] Users have been able to place order for 6 hours [Urgent] Users have not been able to place order for 6 hours Jan 30, 2019
@Hanrui
Copy link

Hanrui commented Jan 30, 2019

According to release tracking, this version 1901242049580 was pushed to production at 4:14 PM, Jan 29, 2019 UTC-5.

@aghassemi
Copy link
Contributor

Rolling back the release until this issue is resolved and will roll forward again. Apologies.

@dreamofabear
Copy link

I can repro on 011901242049580 but not on 011901241837330.

@aghassemi aghassemi changed the title [Urgent] Users have not been able to place order for 6 hours amp-form issue in latest release Jan 30, 2019
@aghassemi
Copy link
Contributor

@gmajoulet @torch2424 for now release was rolled back, let's fix, test and patch this and then roll forward again.

@aghassemi
Copy link
Contributor

sorry @Enriqe instead of @gmajoulet as the on-duty.

@aghassemi
Copy link
Contributor

(Canary would need to be patched or recut as well)

@Enriqe
Copy link
Contributor

Enriqe commented Jan 30, 2019

Sounds good, I will patch the fix for this as well as #20593 in the new release. Then I will roll forward.

@torch2424
Copy link
Contributor

@leonalicious

So the fix for this is now in PROD if I am not mistaken. 😄

However, I still need to implement the original PR (this time not breaking everything 😞). So I wanted to ask, how were you using amp-form? Looking at the site currently, seems like you were using a button and amp-bind to manually call submit on the form. For example: on:tap="... , form.submit".

Was the implementation that broke? Thanks!

@leonahliang90
Copy link
Contributor Author

@torch2424 Sorry for the late reply, I was away for the past few days.

We have been using the GET method to do the navigation, so, when the user clicks on form submission, we will submit all the amp-state as query params to be directed into another page.

@torch2424
Copy link
Contributor

@leonalicious Thank you for the response!

If you have the chance (and are familiar with the AMP dev channel), could you test the latest canary with your checkout flow? We've merged in: #20667 And want to make sure the new fix won't break your pages. I've gone ahead and manually tested this myself, but some testing on your end would be great. 👍

Thanks! 😄

@leonahliang90
Copy link
Contributor Author

Sorry for the delay, thank you @torch2424, both version 1902072121410 & 1902081532110 seems are working fine

@torch2424
Copy link
Contributor

@leonalicious

Thank you very very very much! 😄 Glad to hear it is working on you end as well.

@leonahliang90
Copy link
Contributor Author

thank you for the effort to make this possible. Btw, I am thinking, right now we are only able GET METHOD to do the page navigation, if we are planning to use POST METHOD, I presume the only way would be using the AMP-Redirect-To as Response Header, am I understanding it correctly?

@torch2424
Copy link
Contributor

@leonalicious For that, I'll ask for @cvializ help, as they are more familiar with AMP form than I am 😄

@cvializ
Copy link
Contributor

cvializ commented Feb 15, 2019

You are correct, the AMP-Redirect-To header is the only way to navigate the page after a <form method="POST" ...> successfully submits.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants