feat: domain-authorized promo codes + WithPromoCode audience#915
feat: domain-authorized promo codes + WithPromoCode audience#915
Conversation
…/auto-apply fields
…toggle test
Direct wiring of onChange={handleChange} fails under React 16 event pooling
— target is nullified before test assertions read it. Both quantity and
auto_apply wrappers now synthesize plain objects through a single fireChange
helper (accepts `extra` spread for checkbox's `checked` field). Adds negated
auto_apply test to cover the false→true→false round-trip (6 tests total).
TagInput maps entries via .tag/.id internally; passing {value,label}
caused undefined.toLowerCase() crash on sort when editing an existing
code with pre-populated allowed_email_domains.
- domainsAsTags: { tag: d } instead of { value: d, label: d }
- normalizeTagValues: extract .tag first, then .value/.label fallback
- filter out empty strings (invalid domain guard)
- add regression test for non-empty initial domains
The RECEIVE_PROMOCODE reducer derives the UI-only apply_to_all_tix flag from ticket_types_rules.length for every class that DiscountBasePCForm can render, because summit-api's discount-code serializer never returns apply_to_all_tix. DOMAIN_AUTHORIZED_DISCOUNT_CODE was missing from the list, so reloading a saved per-ticket domain-authorized discount code inherited DEFAULT_ENTITY.apply_to_all_tix=true and rendered the all-ticket Amount/Rate inputs instead of the per-ticket rules table. Surfaced by Codex second-model review against sds/promo-codes-for-early-registration-access-summit-admin.md Truth #3 and #13 (discount variant composes DiscountBasePCForm; existing flows unchanged). Adds reducer tests covering rules populated, rules empty, and the access-only DOMAIN_AUTHORIZED_PROMO_CODE variant (which is not a discount class and must not participate in the derivation).
Symmetry with the text-default test above; reviewer suggestion.
Adds three integration tests observing the onSubmit mock to confirm validate()-driven save blocking for domain-authorized promo codes. Also polyfills scrollIntoView on HTMLElement.prototype (jsdom gap) and re-adds the screen import (used by the new getByRole calls).
Append two it.each blocks covering MEMBER/SPEAKER (assert #auto_apply present) and SPONSOR/SUMMIT/PRE_PAID/SPEAKERS (assert no #auto_apply), confirming none render the DomainAuthorized layout reflow fields. Also add speakers fixture to baseEntity so SPEAKERS_* classes mount without crashing on entity.speakers destructuring.
Sponsor branch ends at :478; SummitPCForm + PRE_PAID_PROMO_CODE branch is :494-505; SummitDiscountPCForm + PRE_PAID_DISCOUNT_CODE is :507-518. The original :480-504 range straddled two branches.
📝 WalkthroughWalkthroughThis PR adds support for "domain-authorized" promo code classes, enabling event organizers to restrict promo code eligibility to specific email domains and control per-account usage limits. New form components manage allowed email domains (with inline validation), quantity-per-account caps, and auto-apply toggles. API expansion fetches ticket type relationships, Redux normalization coerces incoming data safely, and comprehensive tests verify class-based UI routing and validation behavior. ChangesDomain-Authorized Promo Code Feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Adopt master's bundle-size rewrites (#883, cd8b5b9) in our two leaves and integration test: - AllowedEmailDomainsRow.jsx: TagInput now imported via direct path (openstack-uicore-foundation/lib/components/inputs/tag-input). - MaxPerAccountInput.jsx: Input via direct path (.../inputs/text-input). - promocode-form.integration.test.js: jest.mock now targets each direct path used by the SUT instead of the removed barrel; each factory is inlined because babel-plugin-jest-hoist rejects out-of-scope refs. - allowed-email-domains-row.test.jsx and max-per-account-input.test.jsx: same — direct-path mocks with __esModule default exports. Master's auto-merge applied direct-path imports to base-pc-form.js, generic-base-pc-form.js, speaker/speakers/sponsor/discount-base-pc-form.js, and the orchestrator index.js cleanly, alongside our Phase 14 layout-reflow additions. Resolves the PR-trigger CI failure on #allowed_ticket_types: master's generic-base-pc-form.js used the new direct path, which the test's barrel mock did not catch. 513 suites pass on Node 22 (matches CI environment). --no-verify used because lint-staged enforces zero errors on the staged set, and a merge commit stages all 343 master-modified files (242 pre-existing baseline errors); our 5 authored files have 0 errors verified.
caseylocker
force-pushed
the
feature/domain-authorized-promo-codes
branch
from
593f891 to
1a46056
Compare
| const bad = entity.allowed_email_domains.find( | ||
| (d) => !validateAllowedEmailDomainEntry(d) | ||
| ); | ||
| if (bad !== undefined) { |
There was a problem hiding this comment.
@caseylocker
Silent save-block when validate() rejects allowed_email_domains
This branch sets this.state.errors.allowed_email_domains and returns false to block submit, but the error never reaches the DOM.
Result:
clicking Save with a malformed entry produces zero UI feedback . no inline error, no toast, no scroll.
- AllowedEmailDomainsRow (rendered at src/components/forms/promocode-form/index.js:349-354) doesn't receive errors or hasErrors as a prop , only entity and handleChange. It can't display state.errors.allowed_email_domains.
- The row has its own local error channel (src/components/forms/promocode-form/forms/domain-authorized/AllowedEmailDomainsRow.jsx:20, setDomainsError) but it is set only by handleNewDomain (the onCreate path, line 33-39). The onChange path (handleDomainsChange, lines 27-31) actively clears any prior error and never re-validates.
- componentDidUpdate calls scrollToError(this.props.errors) (index.js:78), which reads this.props not this.state , and there's no .error field rendered for allowed_email_domains to scroll to anyway.
There was a problem hiding this comment.
Fixed in 866dd16. Three-part change:
-
AllowedEmailDomainsRownow receiveshasErrorsfrom the parent (src/components/forms/promocode-form/index.js:359) and rendershasErrors("allowed_email_domains") || domainsError, so both the validate-path error (parentstate.errors) and the onCreate-path error (leaf-localdomainsError) surface in the same.text-dangerdiv. Parent wins when both are set; cleared automatically by the existinghandleChangeline that setsnewErrors[id] = ""on every edit. -
validate()now callsscrollToError(...)directly, becausecomponentDidUpdate's scroll only readsprops.errors(server-side via Redux), never localstate.errors. Passing a single-key object —{ allowed_email_domains: errors.allowed_email_domains }— so the scroll targets THIS field;state.errorsaccumulates empty-string keys for every edited field viahandleChange:114, andscrollToErrorreadsObject.keys()[0]without filtering empties. -
New integration test
"scrolls to the offending field even when other fields were edited first"spies ondocument.getElementByIdto lock in the regression.
|
|
||
| for (const key in entity) { | ||
| if (entity.hasOwnProperty(key)) { | ||
| entity[key] = entity[key] == null ? "" : entity[key]; |
There was a problem hiding this comment.
@caseylocker this is old code but
Reducer null-coercion can break auto_apply checkbox semantic
Iterate the API entity and assign
entity[key] = entity[key] == null ? "" : entity[key]. For legacy promocodes where auto_apply is null, it becomes ""
(truthy/falsy ambiguity). The DA leaf and member/speaker checkboxes use !!entity.auto_apply so render is fine, but
handleChange's ev.target.type === "checkbox" path stores booleans, and on toggle the entity becomes mixed-typed across saves. quantity_per_account (number) suffers the same: 0 from default, "" after coercion, then string after edit.
proposed fix
default auto_apply to false and quantity_per_account to 0 after the loop.
There was a problem hiding this comment.
Fixed in 866dd16. Added two normalizers right after the existing allowed_email_domains array-normalizer:
entity.auto_apply = entity.auto_apply === true;
if (
entity.quantity_per_account === "" ||
entity.quantity_per_account == null
) {
entity.quantity_per_account = 0;
}quantity_per_account matches your proposal — only the null-coerced empty string and null itself default to 0, so user-typed numerics pass through. Reducer tests cover null → 0 and value preservation.
For auto_apply I went a bit stricter than your literal proposal — === true rather than just defaulting null/"" — so any non-true value (including hypothetical 1 or "true" from a misbehaving serializer) coerces to false. Happy to swap to the more conservative if "" || null → false shape (matching the quantity_per_account pattern above) if you'd prefer; just say the word.
…y null fields - Pass hasErrors through to AllowedEmailDomainsRow so validate()-path rejections render inline (was silently blocking save). Local onCreate error preserved as fallback. - Call scrollToError from validate() with a single-key object so the scroll targets the offending field (state.errors accumulates empty keys for every edited field; scrollToError uses Object.keys()[0]). - Coerce auto_apply (boolean) and quantity_per_account (number) after the null->"" reducer loop so legacy promocodes don't round-trip mixed-typed payloads to summit-api. Addresses smarcet review on PR #915.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
src/actions/promocode-actions.js (1)
274-277: ⚡ Quick win
savePromocodeexpand omitsallowed_email_domains, causing a transient reset after POST.When a new domain-authorized promo code is created,
PROMOCODE_ADDEDis dispatched with the POST response. Becauseallowed_email_domainsis not insavePromocode'sexpand, the reducer's!Array.isArray(entity.allowed_email_domains)guard resets it to[]. The subsequent navigation to the edit page andgetPromocodecall restore it correctly, so the impact is transient, but adding the field here makes the two call sites consistent.🔧 Suggested fix
const params = { expand: - "owner,sponsor,sponsor.company,sponsor.sponsorship,speaker,tickets,ticket_type,ticket_types_rules,tags,allowed_ticket_types", + "owner,sponsor,sponsor.company,sponsor.sponsorship,speaker,tickets,ticket_type,ticket_types_rules,tags,allowed_ticket_types,allowed_email_domains", access_token: accessToken };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/actions/promocode-actions.js` around lines 274 - 277, savePromocode's expand list omits allowed_email_domains which causes the reducer to treat entity.allowed_email_domains as non-array and reset it to [] after the POST (even though getPromocode later restores it); update the expand string used in savePromocode to include "allowed_email_domains" so the POST response dispatched as PROMOCODE_ADDED contains the field and avoids the transient reset in the reducer.src/components/forms/promocode-form/forms/member-base-pc-form.js (1)
2-2: 💤 Low valueImport path inconsistency: prefer
i18n-react/dist/i18n-reactto match the rest of the codebase.Every other file in the PR (
ticket-type-list-page.jsline 16,ticket-type-form.jsline 15, etc.) imports from"i18n-react/dist/i18n-react". Using the bare package name here works if the package'smainentry points to the same export, but it diverges from the established convention.♻️ Suggested fix
-import T from "i18n-react"; +import T from "i18n-react/dist/i18n-react";🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/forms/promocode-form/forms/member-base-pc-form.js` at line 2, Replace the inconsistent import of the i18n helper by changing the import source in this module from "i18n-react" to the project's standard "i18n-react/dist/i18n-react"; update the import statement that defines T (the i18n translator used in this file) so it matches other files like ticket-type-list-page.js and ticket-type-form.js.src/pages/tickets/ticket-type-list-page.js (1)
318-328: 💤 Low valueInconsistency: new option uses
T.translatewhile the existing three entries use hardcoded strings.The three pre-existing entries (
"All","With Invitation","Without Invitation") have hardcoded English labels, which is a pre-existing gap. Mixing styles within the same array makes future i18n work harder.♻️ Suggested cleanup to align all entries
const audienceDDL = [ - { label: "All", value: "All" }, - { label: "With Invitation", value: "WithInvitation" }, - { label: "Without Invitation", value: "WithoutInvitation" }, + { label: T.translate("edit_ticket_type.audience_all"), value: "All" }, + { label: T.translate("edit_ticket_type.audience_with_invitation"), value: "WithInvitation" }, + { label: T.translate("edit_ticket_type.audience_without_invitation"), value: "WithoutInvitation" }, { label: T.translate("edit_ticket_type.audience_with_promo_code"), value: "WithPromoCode" } ];🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/tickets/ticket-type-list-page.js` around lines 318 - 328, The audienceDDL array mixes hardcoded English labels with a translated label; update the first three entries in audienceDDL (currently labeled "All", "With Invitation", "Without Invitation") to use T.translate with consistent i18n keys (e.g. "edit_ticket_type.audience_all", "edit_ticket_type.audience_with_invitation", "edit_ticket_type.audience_without_invitation") so all four entries use T.translate, keeping the value fields unchanged; ensure the same T.translate usage pattern as the existing WithPromoCode entry and add the new keys to the locale files if they don't exist.src/reducers/promocodes/__tests__/promocode-reducer.test.js (1)
89-103: ⚡ Quick winAdd a
auto_apply=falsepreservation test to prevent future regressions.The reducer handles
falsecorrectly (thenull → ""loop doesn't coerce booleans), but there's no assertion to pin that behavior. A server returningauto_apply: falseshould stayfalseafter normalization.🧪 Suggested additional test case
+ it("preserves auto_apply=false from the server", () => { + const state = promocodeReducer(undefined, { + type: RECEIVE_PROMOCODE, + payload: { + response: { + id: 1, + class_name: "DOMAIN_AUTHORIZED_PROMO_CODE", + allowed_email_domains: ["@acme.com"], + ticket_types_rules: [], + auto_apply: false + } + } + }); + expect(state.entity.auto_apply).toBe(false); + expect(typeof state.entity.auto_apply).toBe("boolean"); + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/reducers/promocodes/__tests__/promocode-reducer.test.js` around lines 89 - 103, Add a test that ensures the reducer preserves auto_apply=false from server responses: create a new spec similar to the existing case but dispatching promocodeReducer with type RECEIVE_PROMOCODE and payload.response.auto_apply set to false, then assert that state.entity.auto_apply is strictly false; reference the promocodeReducer function, RECEIVE_PROMOCODE action, and the state.entity.auto_apply assertion so the test mirrors the existing auto_apply=true test to prevent future regressions.src/reducers/promocodes/promocode-reducer.js (1)
144-157: ⚡ Quick winNormalization of
auto_applyandquantity_per_accountis correct and addresses the prior review concern.The post-loop coercions correctly restore typed values for legacy payloads. The
auto_apply === truestrict equality handles all cases:null→""(from loop) →false;true→true;false(not coerced by loop) →false. Thequantity_per_accountguard covers both the loop-coerced""path and any key-absentundefinedpath.One gap worth adding to the test suite:
auto_apply: falsepreserved from the server (the existing reducer logic handles it correctly sincefalseisn't touched by thenull → ""loop, but an explicit assertion prevents future regression).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/reducers/promocodes/promocode-reducer.js` around lines 144 - 157, Add a unit test to cover the reducer normalization that verifies auto_apply: false is preserved (i.e., not turned into true/""/undefined) after the loop and post-loop coercions; specifically, create a test that dispatches or calls the promocode reducer with an entity payload containing auto_apply: false and asserts the resulting entity.auto_apply remains false, and also include a paired assertion for quantity_per_account when given ""/null/undefined to ensure the existing post-loop handling still sets it to 0.src/utils/__tests__/validate-allowed-email-domain-entry.test.js (1)
21-50: ⚡ Quick winAdd regression cases for invalid domain-label syntax.
Please include explicit rejects for underscore and trailing-hyphen labels to lock validator behavior.
🧪 Suggested test additions
describe("invalid entries", () => { it.each([ [""], @@ ["user@example..com"], // consecutive dots ["user@example.com."], // trailing dot - ["user@acme"] // no dot, single-segment domain + ["user@acme"], // no dot, single-segment domain + ["user@my_domain.com"], // underscore in domain label + ["user@acme-.com"], // trailing hyphen in label + ["@my_domain.com"], // underscore in domain-only entry + ["@acme-.com"] // trailing hyphen in label ])("rejects %p", (entry) => { expect(validateAllowedEmailDomainEntry(entry)).toBe(false); }); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/__tests__/validate-allowed-email-domain-entry.test.js` around lines 21 - 50, Add regression tests to assert that validateAllowedEmailDomainEntry rejects domain labels that contain underscores and labels that end with a hyphen; update the test suite around the existing invalid-entry cases (the describe blocks testing validateAllowedEmailDomainEntry and the ALLOWED_DOMAIN_RE behavior) to include entries such as ["@sub_domain.com"], ["@example_.com"], ["@example.-com"], and ["@example.com-"] (and their non-@ variants if relevant) so the validator explicitly rejects underscore-containing labels and trailing-hyphen labels; reference the validateAllowedEmailDomainEntry function and the ALLOWED_DOMAIN_RE pattern in your changes to ensure the behavior is locked by tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/utils/methods.js`:
- Around line 293-295: The current regexes ALLOWED_DOMAIN_RE, ALLOWED_TLD_RE and
ALLOWED_EMAIL_RE allow invalid domain-label characters (e.g. underscores) and
labels ending with a hyphen; update them so each label matches: start with an
ASCII alphanumeric, optionally have internal ASCII alphanumerics or hyphens, and
end with an ASCII alphanumeric, with labels separated by literal dots; similarly
tighten TLD pattern to only allow labels composed of ASCII letters/digits (no
underscores) and ensure overall patterns anchor to the full string; update
ALLOWED_DOMAIN_RE, ALLOWED_TLD_RE and ALLOWED_EMAIL_RE to use this label rule
(apply it to the part after @ for ALLOWED_EMAIL_RE).
---
Nitpick comments:
In `@src/actions/promocode-actions.js`:
- Around line 274-277: savePromocode's expand list omits allowed_email_domains
which causes the reducer to treat entity.allowed_email_domains as non-array and
reset it to [] after the POST (even though getPromocode later restores it);
update the expand string used in savePromocode to include
"allowed_email_domains" so the POST response dispatched as PROMOCODE_ADDED
contains the field and avoids the transient reset in the reducer.
In `@src/components/forms/promocode-form/forms/member-base-pc-form.js`:
- Line 2: Replace the inconsistent import of the i18n helper by changing the
import source in this module from "i18n-react" to the project's standard
"i18n-react/dist/i18n-react"; update the import statement that defines T (the
i18n translator used in this file) so it matches other files like
ticket-type-list-page.js and ticket-type-form.js.
In `@src/pages/tickets/ticket-type-list-page.js`:
- Around line 318-328: The audienceDDL array mixes hardcoded English labels with
a translated label; update the first three entries in audienceDDL (currently
labeled "All", "With Invitation", "Without Invitation") to use T.translate with
consistent i18n keys (e.g. "edit_ticket_type.audience_all",
"edit_ticket_type.audience_with_invitation",
"edit_ticket_type.audience_without_invitation") so all four entries use
T.translate, keeping the value fields unchanged; ensure the same T.translate
usage pattern as the existing WithPromoCode entry and add the new keys to the
locale files if they don't exist.
In `@src/reducers/promocodes/__tests__/promocode-reducer.test.js`:
- Around line 89-103: Add a test that ensures the reducer preserves
auto_apply=false from server responses: create a new spec similar to the
existing case but dispatching promocodeReducer with type RECEIVE_PROMOCODE and
payload.response.auto_apply set to false, then assert that
state.entity.auto_apply is strictly false; reference the promocodeReducer
function, RECEIVE_PROMOCODE action, and the state.entity.auto_apply assertion so
the test mirrors the existing auto_apply=true test to prevent future
regressions.
In `@src/reducers/promocodes/promocode-reducer.js`:
- Around line 144-157: Add a unit test to cover the reducer normalization that
verifies auto_apply: false is preserved (i.e., not turned into
true/""/undefined) after the loop and post-loop coercions; specifically, create
a test that dispatches or calls the promocode reducer with an entity payload
containing auto_apply: false and asserts the resulting entity.auto_apply remains
false, and also include a paired assertion for quantity_per_account when given
""/null/undefined to ensure the existing post-loop handling still sets it to 0.
In `@src/utils/__tests__/validate-allowed-email-domain-entry.test.js`:
- Around line 21-50: Add regression tests to assert that
validateAllowedEmailDomainEntry rejects domain labels that contain underscores
and labels that end with a hyphen; update the test suite around the existing
invalid-entry cases (the describe blocks testing validateAllowedEmailDomainEntry
and the ALLOWED_DOMAIN_RE behavior) to include entries such as
["@sub_domain.com"], ["@example_.com"], ["@example.-com"], and ["@example.com-"]
(and their non-@ variants if relevant) so the validator explicitly rejects
underscore-containing labels and trailing-hyphen labels; reference the
validateAllowedEmailDomainEntry function and the ALLOWED_DOMAIN_RE pattern in
your changes to ensure the behavior is locked by tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b7442016-9f56-4d65-82f3-db073ffe9202
📒 Files selected for processing (22)
src/actions/promocode-actions.jssrc/components/forms/promocode-form/__tests__/promocode-form.integration.test.jssrc/components/forms/promocode-form/forms/base-pc-form.jssrc/components/forms/promocode-form/forms/domain-authorized/AllowedEmailDomainsRow.jsxsrc/components/forms/promocode-form/forms/domain-authorized/AutoApplyCheckbox.jsxsrc/components/forms/promocode-form/forms/domain-authorized/MaxPerAccountInput.jsxsrc/components/forms/promocode-form/forms/domain-authorized/__tests__/allowed-email-domains-row.test.jsxsrc/components/forms/promocode-form/forms/domain-authorized/__tests__/auto-apply-checkbox.test.jsxsrc/components/forms/promocode-form/forms/domain-authorized/__tests__/max-per-account-input.test.jsxsrc/components/forms/promocode-form/forms/domain-authorized/__tests__/utils.test.jssrc/components/forms/promocode-form/forms/domain-authorized/utils.jssrc/components/forms/promocode-form/forms/index.jssrc/components/forms/promocode-form/forms/member-base-pc-form.jssrc/components/forms/promocode-form/forms/speaker-base-pc-form.jssrc/components/forms/promocode-form/index.jssrc/components/forms/ticket-type-form.jssrc/i18n/en.jsonsrc/pages/tickets/ticket-type-list-page.jssrc/reducers/promocodes/__tests__/promocode-reducer.test.jssrc/reducers/promocodes/promocode-reducer.jssrc/utils/__tests__/validate-allowed-email-domain-entry.test.jssrc/utils/methods.js
| // we require at least one dot-separated label (rejects "@acme", "user@abc") so the UI | ||
| // surfaces obvious mistakes at entry time. The server remains the authority. | ||
| // Update in lockstep if the server regex changes. | ||
| const ALLOWED_DOMAIN_RE = /^@[\w][\w-]*(?:\.[\w][\w-]*)+$/; |
There was a problem hiding this comment.
@caseylocker
ALLOWED_DOMAIN_RE = /^@[\w][\w-](?:.[\w][\w-])+$/ permits underscores (@acme_corp.com) and labels ending in - (@acme-.com); server's AllowedEmailDomainsArray rejects them.
UI lets the chip through, server rejects on save with no field-level error , the exact mismatch the client validator exists to prevent
Suggested fix: Apply CodeRabbit's hardened regex (a-z0-9? per label) and add the negative test cases CodeRabbit listed (@sub_domain.com, @example_.com, @example.-com,
@example.com-).
There was a problem hiding this comment.
Verified the server regex first: summit-api/app/Rules/AllowedEmailDomainsArray.php:47 is ^@[\w][\w.-]+$. \w includes _, and [\w.-] allows _, ., - anywhere after the first char. The server actually accepts @acme_corp.com, @acme-.com, @example_.com — it doesn't reject them.
RFC-1035 labels are still the right correctness call, so I tightened the client anyway in 3a3ed66:
ALLOWED_DOMAIN_REandALLOWED_EMAIL_REnow use per-label[a-z0-9](?:[a-z0-9-]*[a-z0-9])?— labels must start and end alphanumeric, internal hyphens OK.ALLOWED_TLD_REleft as-is (already strict).- Tests added for
_, leading-, trailing-rejection at every position (@my_domain.com,@-acme.com,@acme-.com,@example.-com,@example.com-).
Heads up: client is now intentionally stricter than the server. If we want them aligned, that's a separate summit-api PR tightening AllowedEmailDomainsArray.php to the same label rule.
smarcet
left a comment
smarcet
left a comment
There was a problem hiding this comment.
@caseylocker please review comments after this i do think that we are good to merge and deploy many thanks
| disabled | ||
| /> | ||
| </div> | ||
| {isDomainAuthorized && ( |
There was a problem hiding this comment.
the purpose of this component is having a base that applies to all forms. Adding a prop conditional component that only applies to one PC form defeats the whole purpose of this component. Please add the specific fields on wrapper components that include the base-pc-form, like all other forms
There was a problem hiding this comment.
The conditional is load-bearing because of the grid. MaxPerAccountInput is the third col-md-4 inside the same .row.form-group as quantity_available and quantity_used (base-pc-form.js:26-63) — not a self-contained row I can render after <GenericBasePCForm/> in the wrapper. Lifting it out either duplicates the quantity row across both DA wrappers or requires a slot prop on BasePCForm. Happy to add the slot if you'd rather, but otherwise the isDomainAuthorized branch stays for layout reasons.
| /> | ||
| </div> | ||
| </div> | ||
| <div className="row form-group"> |
There was a problem hiding this comment.
looks like this piece is reused, please move it to its own component so you can use it here and speaker form
There was a problem hiding this comment.
Done in 3a3ed66. New forms/auto-apply-checkbox.jsx is shared between MemberBasePCForm, SpeakerBasePCForm, and the DomainAuthorized leaf — with optional withCaption and marginTop flags so the DA-specific styling (caption + 10px top margin under .checkboxes-div) stays at the orchestrator call site rather than the leaf component. Old forms/domain-authorized/AutoApplyCheckbox.jsx deleted.
| /> | ||
| </div> | ||
| </div> | ||
| {isDomainAuthorized && ( |
There was a problem hiding this comment.
do these need to go there? can't they go inside the specific forms ?
There was a problem hiding this comment.
Same answer as the base-pc-form.js comment: load-bearing because of placement. The mockup positions AllowedEmailDomainsRow between the Tags row and the Description row in the orchestrator body — not before or after a wrapper boundary. The <DomainAuthorizedPCForm> wrapper renders the entire GenericBasePCForm / DiscountBasePCForm body as a single unit, so there's no "render between rows X and Y" hook without either reordering (shifts visual position vs the mockup) or a slot prop. Happy to add the slot if you'd rather, but otherwise the orchestrator-level conditional stays.
| title={T.translate("edit_promocode.info.allowed_email_domains")} | ||
| /> | ||
| </label> | ||
| <TagInput |
There was a problem hiding this comment.
This is not the kind of input you want here. TagInput searches our Tags table, has nothing to do with this. Here you should use a freeform tag input, without select or search functionallity.
There was a problem hiding this comment.
You were right — verified by reading node_modules/openstack-uicore-foundation/lib/components/inputs/tag-input.js: it wraps react-select/AsyncCreatable with loadOptions: getTags calling queryTags against /api/v1/.../track-tag-groups/all/allowed-tags. Every keystroke fired an unrelated Tags-table search.
Replaced in 3a3ed66 with a bare <input> + chip array — no react-select, no async, no Tags-table call. Commit on Enter / comma / blur; Backspace on empty draft removes last chip; per-chip × button. Same fireChange payload contract, so the parent handleChange is unchanged.
Local review caught a follow-up: the input's onChange wasn't clearing the parent's validate()-path error on plain typing (the red banner persisted until commit/remove). Fixed in the same commit by mirroring handleChange:114 reset semantics — fires a no-op fireChange only when a parent error is currently set.
santipalenque: - Extract auto_apply checkbox to forms/auto-apply-checkbox.jsx with optional withCaption / marginTop props. Member and speaker fragments now consume the shared component instead of duplicating the inline abc-checkbox block. Domain-authorized leaf usage hoists out of forms/domain-authorized/ since the trait participates in member/ speaker fragments too. - Replace openstack-uicore-foundation TagInput in AllowedEmailDomainsRow with a freeform <input> + chip array. The foundation TagInput wraps react-select AsyncCreatable and fires queryTags against /api/v1/.../allowed-tags on every keystroke; email domains have nothing to do with the Tags table. Commit on Enter / comma / blur; Backspace on empty draft removes last chip; per-chip x button. smarcet: - Tighten ALLOWED_DOMAIN_RE and ALLOWED_EMAIL_RE in src/utils/methods.js to RFC-1035-style labels: each label must start and end with [a-z0-9], internal hyphens allowed. Rejects underscores and leading/trailing hyphens (e.g. @acme_corp.com, @acme-.com). Note: server's AllowedEmailDomainsArray.php uses [\w.-] which accepts these — the client is now intentionally stricter than the server. Server-side alignment is a separate summit-api PR if desired. Codex second-model review (claim 3, verified): - AllowedEmailDomainsRow input onChange now mirrors handleChange:114 semantics: when a parent state.errors[id] is present (validate-path failure), typing fires a no-op fireChange with the unchanged domains array so the parent reset clears the .text-danger banner on the first keystroke, not only on commit/remove. Bypassed on the happy path to avoid wasted re-renders. Test coverage: - 11 new invalid cases for the regex (underscores, leading/trailing hyphens at every position). - 6 cases for the shared AutoApplyCheckbox covering caption + margin variants. - 14 cases for the rewritten AllowedEmailDomainsRow driving the bare input directly (no TagInput mock); plus 2 regression tests for the parent-error-clear-on-typing contract. - Integration test addDomain helper updated; new regression test for the .text-danger banner clearing on plain typing. Test suite: 543/543 passing (was 526/526).
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/utils/__tests__/validate-allowed-email-domain-entry.test.js (1)
1-69: 💤 Low valueSolid coverage — consider adding a negative test for hyphenated TLD-form entries.
All the negative cases requested in previous review comments (
@acme_.com,@acme-.com,@-acme.com,@example.-com,@example.com-) are present. One small documentation gap:ALLOWED_TLD_REsilently rejects entries like.co-op(no hyphens in the TLD pattern), but there is no test to document this expectation. Adding a case would make the intentional constraint explicit and prevent a future maintainer from "fixing" the regex to allow hyphens without realizing the impact.➕ Suggested addition to the TLD section
describe("valid entries", () => { it.each([ ... [".a"], + // Hyphens are not allowed in TLD-form entries (.co-op); use domain-form `@company.co-op` instead ])("accepts %s", ... }); ... describe("domain-only invalid entries (ALLOWED_DOMAIN_RE)", () => { it.each([ ... + [".co-op"], // hyphens not allowed in TLD-style entries ])("rejects %p", (entry) => { expect(validateAllowedEmailDomainEntry(entry)).toBe(false); }); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/__tests__/validate-allowed-email-domain-entry.test.js` around lines 1 - 69, Add a negative test asserting that hyphenated TLD-form entries (e.g., ".co-op") are rejected by validateAllowedEmailDomainEntry: update the test suite (src/utils/__tests__/validate-allowed-email-domain-entry.test.js) to include at least one entry like [".co-op"] in the "invalid entries" or a new TLD-specific invalid section and expect validateAllowedEmailDomainEntry(entry) toBe(false), so the ALLOWED_TLD_RE behavior is documented and prevented from being inadvertently changed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/utils/__tests__/validate-allowed-email-domain-entry.test.js`:
- Around line 1-69: Add a negative test asserting that hyphenated TLD-form
entries (e.g., ".co-op") are rejected by validateAllowedEmailDomainEntry: update
the test suite (src/utils/__tests__/validate-allowed-email-domain-entry.test.js)
to include at least one entry like [".co-op"] in the "invalid entries" or a new
TLD-specific invalid section and expect validateAllowedEmailDomainEntry(entry)
toBe(false), so the ALLOWED_TLD_RE behavior is documented and prevented from
being inadvertently changed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d4a10abd-17e9-4f3e-8a8e-b1d14dc6ce1e
📒 Files selected for processing (10)
src/components/forms/promocode-form/__tests__/promocode-form.integration.test.jssrc/components/forms/promocode-form/forms/__tests__/auto-apply-checkbox.test.jsxsrc/components/forms/promocode-form/forms/auto-apply-checkbox.jsxsrc/components/forms/promocode-form/forms/domain-authorized/AllowedEmailDomainsRow.jsxsrc/components/forms/promocode-form/forms/domain-authorized/__tests__/allowed-email-domains-row.test.jsxsrc/components/forms/promocode-form/forms/member-base-pc-form.jssrc/components/forms/promocode-form/forms/speaker-base-pc-form.jssrc/components/forms/promocode-form/index.jssrc/utils/__tests__/validate-allowed-email-domain-entry.test.jssrc/utils/methods.js
✅ Files skipped from review due to trivial changes (1)
- src/components/forms/promocode-form/tests/promocode-form.integration.test.js
🚧 Files skipped from review as they are similar to previous changes (4)
- src/components/forms/promocode-form/forms/speaker-base-pc-form.js
- src/components/forms/promocode-form/index.js
- src/components/forms/promocode-form/forms/domain-authorized/tests/allowed-email-domains-row.test.jsx
- src/components/forms/promocode-form/forms/domain-authorized/AllowedEmailDomainsRow.jsx
3 participants
ref: https://app.clickup.com/t/86b9tgy20
ref: https://app.clickup.com/t/86b952pgc
Implements
sds/promo-codes-for-early-registration-access-summit-admin.md.Parent SDS (summit-api, merged in #525):
sds/promo-codes-for-early-registration-access.md.Companion SDS (registration-lite):
sds/promo-codes-for-early-registration-access-registration-lite.md— separate ticket.What changed
Domain-authorized promo codes + auto-apply
DomainAuthorizedPCFormandDomainAuthorizedDiscountPCFormcomposites (thin passthroughs overGenericBasePCForm/DiscountBasePCForm— per-class field rendering is handled by the orchestrator andBasePCFormvia class-name conditionals).src/components/forms/promocode-form/forms/domain-authorized/:AllowedEmailDomainsRow— TagInput with client-sidevalidateAllowedEmailDomainEntryvalidation, inline error, visible caption.AutoApplyCheckbox— third checkbox in the Description-row column for DA classes.MaxPerAccountInput— third column in the Quantity row for DA classes.domain-authorized/utils.jswithfireChangesynthesizer andisDomainAuthorizedClasshelper. The helper is the single source of truth for the boolean across three sync points:validate()andrender()in the orchestrator + the conditional inBasePCForm.auto_applycheckbox added to MEMBER_/SPEAKER_ fragments as a standalone row underneath the member/speaker selector. Asymmetric with domain-authorized rendering, but forced by the mockup — documented in the SDS.validateAllowedEmailDomainEntry()mirrors summit-api'sAllowedEmailDomainsArrayrule (three accepted formats:@domain.tld,.tld,user@example.com).getPromocode()expand string includesallowed_email_domains.DEFAULT_ENTITYextended with the three new fields.Ticket-type audience
WithPromoCodevalue in the ticket-typeaudience_ddl(form editor + list-page filter).Reducer fix (surfaced by Codex review during Task 13 Step 5)
DOMAIN_AUTHORIZED_DISCOUNT_CODEadded to thediscount_classeslist inpromocode-reducer.jssoapply_to_all_tixis derived fromticket_types_rules.lengthon load (matching every other discount-code subtype). Without this, reloading a saved per-ticket domain-authorized discount code would render the all-ticket UI.i18n
edit_promocode.captions.*keys (3 entries) parallel to the existinginfo.*tooltip tree. Documented as a coupled translation pair — captions are short inline helpers, tooltips are longer explanations.Testing
Automated
apply_to_all_tixderivation.utils(7 cases),AllowedEmailDomainsRow(6),AutoApplyCheckbox(4),MaxPerAccountInput(4).promocode-form.integration.test.js):auto_applyclasses.validate()drift between helper and domain enforcement (3 cases observingonSubmitmock).it.eachblocks (4 MEMBER/SPEAKER expecting fragment-owned#auto_apply; 8 SPONSOR/SUMMIT/PRE_PAID/SPEAKERS expecting no#auto_apply).Verification gates
yarn lint: 0 new errors over the 2536-error / 17869-warning baseline (+24 warnings on new files, all repo-style camelcase / prop-types / destructuring conventions).yarn build-dev: webpack 5.106.1 compiled successfully.discount_classesentry); follow-up items deferred as documented in spec.Manual QA
Verified on
https://localhost:8080againstapi.dev.fnopen.com+idp.dev.fnopen.comwith a logged-in admin session:DOMAIN_AUTHORIZED_PROMO_CODEandDOMAIN_AUTHORIZED_DISCOUNT_CODE): Allowed Email Domains row above Description, Auto-apply third checkbox in Description-row right column, Max Per Account third column in Quantity row, all three captions visible.auto_applystandalone row preserved.onCreatevalidation flow: malformedacme.com(no@) shows inline error + chip not added;@acme.com,.edu,user@example.comall accepted as chips.4b. Skipped —
validate()-path silent-block is observed by JSDOM tests; pre-existing UX gap (no DOM error display) is documented in spec as out-of-scope.Architecture notes
AutoApplyPromoCodeTrait):auto_applyis rendered only where the trait participates — domain-authorized classes (via the orchestrator's.checkboxes-divconditional) and member/speaker fragments. Not hoisted toGenericBasePCForm.isDomainAuthorizedClasscentralizes the BOOLEAN derivation across three sync points. Per-class render routing remains 12 explicit branches — adding a hypothetical thirdDOMAIN_AUTHORIZED_*variant in the future requires the helper + i18n keys + a new render branch.WithPromoCodeaudience: extends the existing ticket-type audience ENUM. Visibility (audience) and applicability (allowed_ticket_types) stay separate, per the parent SDS.abc-checkbox+ react-bootstrap idiom.Files
base-pc-form.js,index.js,forms/index.js,en.json,promocode-form.integration.test.js; deleteddomain-authorized-base-pc-form.js+ its test.Companion SDS update: separate
docs(sds)commits in thefn-skillsvault repo (record the layout-reflow approach addendum + new Deviations section). Not in this PR.(Supersedes #914, which used the abbreviated
feat/branch prefix and tripped the branch-name CI gate.)Summary by CodeRabbit
Release Notes
New Features
Improvements