Fix completion-screen override handling (blank button + uninterpolated {tokens}) - #157
Fix completion-screen override handling (blank button + uninterpolated {tokens})#157gcutrini wants to merge 4 commits into
Conversation
Two problems on the completion screen when marketing overrides are set:
- A button/title override passed as a present-but-undefined prop slipped
through the `!isEmptyString(...)` guard, because isEmptyString only
treated strings as empty. The order-complete button then rendered
blank. isEmptyString now treats null/undefined as empty, so the button
and title overrides fall back to their translated defaults; the
redundant `typeof !== 'undefined'` checks are removed.
- An override paragraph was inserted verbatim, so {attendee}/{adv}/{button}
tokens in custom copy printed literally. Add an interpolate() helper and
run the override paragraphs through it with the same values the built-in
strings use.
Adds unit tests for isEmptyString and interpolate, plus regression tests
for the blank button and the paragraph interpolation.
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: Comment |
| // Replaces {token} placeholders in a template string with values from `vars`. | ||
| // Used so marketing-override copy supports the same {attendee}/{adv}/{button} | ||
| // tokens the built-in i18n strings do. Unknown tokens are left untouched. | ||
| export const interpolate = (template, vars = {}) => { |
There was a problem hiding this comment.
maybe you can avoid looping over the template over and over again with something like this:
const TOKEN = /\{(\w+)\}/g;
export const interpolate = (template, vars = {}) => {
if (typeof template !== 'string') return template;
return template.replace(TOKEN, (match, key) =>
key in vars ? String(vars[key]) : match
);
};```
There was a problem hiding this comment.
Done in 8acd6e6, thanks. Single regex pass now.
It also fixes a bug: with the old loop, a value containing a {token} got expanded by the next pass. Added a test for it.
Replace the per-variable split/join loop with one regex pass. A value
that contains a {token} is now inserted as written, not expanded again.
|
@smarcet all my open PRs have the ref: header now, each one pointing to its ClickUp ticket. |
| !attendeeIsSomeoneElse && rest.hasOwnProperty('initialOrderComplete1stParagraph') && typeof rest.initialOrderComplete1stParagraph !== 'undefined' ? | ||
| rest.initialOrderComplete1stParagraph | ||
| interpolate(rest.initialOrderComplete1stParagraph, paragraphVars) | ||
| : | ||
| T.translate('purchase_complete_step.initial_order_complete_1st_paragraph_label', | ||
| { | ||
| attendee: `${attendeeIsSomeoneElse ? ` ${currentTicket.owner.email}` : 'you'}`, | ||
| adv: `${attendeeIsSomeoneElse ? `${currentTicket.owner.email}` : 'your'}`, | ||
| button: orderCompleteButtonText | ||
| } | ||
| ) | ||
| T.translate('purchase_complete_step.initial_order_complete_1st_paragraph_label', paragraphVars) | ||
| : | ||
| rest.hasOwnProperty('orderComplete1stParagraph') && typeof rest.orderComplete1stParagraph !== 'undefined' ? |
There was a problem hiding this comment.
@gcutrini This guard still only checks typeof rest.X !== 'undefined', so a marketing override present with null (or '') takes the override branch and blanks the paragraph — the same bug class this PR just fixed for orderCompleteButton/orderCompleteTitle via isEmptyString, one field over. interpolate(null, vars) returns null unchanged (line 138/143), and <span>{null}</span> renders nothing.
This is reachable in practice: initialOrderComplete1stParagraph is passed through unfiltered from the embedder's props (registration-form/index.js:539), the same marketing-override data source that produces the "present-but-undefined" values this PR already had to guard against for the button. PropTypes.string (non-required, no defaultProps entry — see registration-modal/index.js:85-91) won't warn on null either, so it fails silently.
Suggested fix: swap typeof rest.X !== 'undefined' for !isEmptyString(rest.X) in both branches (line 137 and 142), matching the fix already applied to orderCompleteButtonText/orderCompleteTitle above.
There was a problem hiding this comment.
Fixed in 3df171e, you were right.
I found it in four places, not two. The 2nd paragraph has the same guard in both its branches, so initialOrderComplete2ndParagraph and orderComplete2ndParagraph had the same bug. All four now use !isEmptyString(rest.X).
Tests first in 768de47: null and empty string, on both paragraphs. They fail on the old guards and pass on the new ones.
A paragraph override present as null or an empty string must fall back to the default, the same as the button and title overrides.
A paragraph override present as null or an empty string took the override branch and rendered nothing. Use the same guard as the button and title overrides, on both paragraphs and both their branches.
3 participants
ref: https://app.clickup.com/t/86bbm1pzq
Problem
On the purchase-complete screen, marketing overrides were mishandled two ways:
passed the
!isEmptyString(...)guard (isEmptyString only treated strings asempty), so the order-complete button rendered with no text.
{attendee}/{adv}/{button}tokens in custom copy printed literally, while thebuilt-in i18n strings interpolated them.
Fix
isEmptyStringnow treatsnull/undefinedas empty, so every override guard(both buttons + title) falls back to its default. Redundant
typeofchecks removed.interpolate()helper; override paragraphs run through it with the same{attendee}/{adv}/{button}values the defaults use.Tests
isEmptyString(null/undefined/whitespace) andinterpolate.{button}in anoverride paragraph resolves.