🌸 Cherry pick request for #25470 into #25319 (Approved) #25473
Description
Cherry-pick request
| Issue | PR | Production? | RC? | Release issue |
|---|---|---|---|---|
| #25432 | #25470 | YES | YES | #25319 |
Why does this issue meet the cherry-pick criteria?
P0 amp-form regression -- broken redirects on form submission disrupts user actions (i.e. shopping on e-commerce websites)
Mini-postmortem
Summary
We refactored some form submission methods in <amp-form> assuming that form-submission follow-up actions such as (1) redirecting and (2) triggering analytics should be done only if the response is successfully parseable to JSON. If the response is invalid JSON it errors out.
Prior to this change, websites whose responses were sending invalid JSON still provided the experience of complete user journeys because <amp-form> would throw the invalid JSON error and still continue on with actions 1 & 2 above.
Both examples reported in the originating P0 experienced their redirects breaking and showed such invalid JSON errors in the console upon submission.
| Users affected | Impact |
|---|---|
| Some users who submit a form expecting a redirect (i.e. "Add to cart and continue shopping") | Only users of websites whose form submissions (1) send an unparseable JSON response that (2) contains a redirect, likely < 30% of redirected amp-form users. This may have also impacted analytics coverage for actions falling under this edge case. |
Root Causes
- Refactoring form submission without explicit consideration for what the error-case user perceived behavior would be prior to and after the change.
Action Items
| Action Item | Type | Owner | PR # |
|---|---|---|---|
| Revert offending PR | Mitigate | @caroqliu | #25470 |
| Investigate original #25242 | Investigate | @caroqliu | N/A |
| Add unit tests for pre-existing redirect behavior to prevent unexpected consequences of refactoring | Prevent | @caroqliu | #25515 |
Lessons Learned
Refactoring code in production requires consideration for what the behavior already is rather than what it is expected to be. The user journeys that are allowed by our code, explicitly or not, ultimately become the behavior that people rely on. As such maintaining existing behavior (in this scenario, unexpected error-case behavior) should be a priority over retroactively implicitly enforcing new guidelines, which was a natural consequence of the original breaking change that assumed non-error prone code paths when refactoring.
Things that went well
- @nainar promptly caught and triaged the incoming issue as well as relayed the revert and cherry pick to appropriate approvers. Thank you!
- @erwinmombay was incredibly patient and helpful with investigation, revert, and cherry pick. He monitored the change to production while also advising on the opt-in process for verifying the fix. Thank you!
Things that went wrong
- Not having comprehensive unit tests to confirm that a refactor was in fact neutral to pre-existing behavior that publishers have come to rely on.
/cc @ampproject/wg-approvers @ampproject/cherry-pick-approvers