Fix completion-screen override handling (blank button + uninterpolated {tokens}) - #157
Fix completion-screen override handling (blank button + uninterpolated {tokens})#157gcutrini wants to merge 2 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: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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.
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.