Treat quantity 0 as unlimited in ticket and promo caps - #154
Conversation
The widget read a 0 in three quantity fields as "zero allowed" and marked tickets Sold Out, while the API treats 0 as "no limit". getTicketMaxQuantity now treats quantity_2_sell and max_quantity_per_order of 0 as unlimited. A Complimentary Sponsor ticket with max_quantity_per_order 0 (2900 to sell, 68 left) showed as Sold Out. usePromoCode now treats a promo code quantity_available of 0 as unlimited, matching the API's hasQuantityAvailable, so it no longer zeroes the ticket stepper. remaining_quantity_per_account is unchanged: the API drops a code once an account exhausts it, so it is never 0 here.
|
Important Review available on request
Reviews should be triggered manually for repositories with fewer than 10 stars. Select Trigger review above or comment ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe change treats zero ticket and promo quantity limits as unlimited. Ticket calculations also default missing sold quantities to zero. Tests cover stock limits, order limits, per-account caps, sold-out tickets, and prepaid tickets. ChangesQuantity limit handling
Estimated code review effort: 2 (Simple) | ~15 minutes Merge Risk: ⚪ Minimal · up to The change makes zero ticket and promo quantities behave as unlimited, matching the API contract and restoring availability for affected registrations; no actionable merge-blocking risk remains beyond normal checks and review. Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
| if(isPrePaidTicketType(ticket)) return 1; | ||
| let max = Math.min((ticket.quantity_2_sell ?? Number.MAX_SAFE_INTEGER) - ticket.quantity_sold, (ticket.max_quantity_per_order ?? Number.MAX_SAFE_INTEGER)); | ||
| // The API treats 0 as "no limit" for both fields; only a positive value is a real cap. | ||
| const quantityToSell = ticket.quantity_2_sell || Number.MAX_SAFE_INTEGER; |
There was a problem hiding this comment.
@gcutrini The 0-means-unlimited semantics this line establishes are still missing in one sibling consumer of the same fields: the ticketPerOrderLimit memo in src/components/ticket-type/index.js (lines 132-136) computes inventory = (ticket.quantity_2_sell ?? Number.MAX_SAFE_INTEGER) - (ticket.quantity_sold ?? 0), so a ticket with quantity_2_sell: 0 yields inventory <= 0 and cap < inventory is always false — the "limited to N per order" notice is never shown.
Why it matters: quantity_2_sell defaults to 0 in the API model (SummitTicketType.php constructor), so unlimited-stock tickets with a real per-order cap are a common configuration. For those tickets the stepper now correctly stops at the cap (thanks to this PR's fix in getTicketMaxQuantity), but the notice explaining that cap is silently suppressed — the user sees the + button disable with no explanation. The memo's own comment says the cap should surface when it is "the binding constraint on the stepper", which is exactly this case.
Suggested fix (one line, same semantics as this helper):
const inventory = (ticket.quantity_2_sell || Number.MAX_SAFE_INTEGER) - (ticket.quantity_sold ?? 0);Red test that verifies it (fails on this branch with "Unable to find an element with the text: This ticket type is limited to 4 per order.", passes with the one-line fix — verified locally both ways). Suggested location: src/components/ticket-type/__tests__/per-order-notice.test.js:
import React from 'react';
import { render } from '@testing-library/react';
import '@testing-library/jest-dom';
import T from 'i18n-react';
import TicketTypeComponent from '..';
T.setTexts(require('../../../i18n/en.json'));
const unlimitedStockTicket = {
id: 1,
name: 'General Admission',
currency: 'USD',
currency_symbol: '$',
quantity_2_sell: 0, // API semantics: 0 = unlimited stock
quantity_sold: 10,
max_quantity_per_order: 4, // real, binding per-order cap
};
it('shows the per-order limit notice when stock is unlimited (quantity_2_sell 0)', () => {
const { getByText } = render(
<TicketTypeComponent
isActive
allowedTicketTypes={[unlimitedStockTicket]}
originalTicketTypes={[unlimitedStockTicket]}
taxTypes={[]}
changeForm={jest.fn()}
trackViewItem={jest.fn()}
allowPromoCodes={false}
reservation={{ tickets: [{ ticket_type_id: 1 }] }}
/>
);
// The stepper caps at 4 (getTicketMaxQuantity treats quantity_2_sell 0 as
// unlimited stock), so the notice explaining that cap must be shown.
expect(getByText('This ticket type is limited to 4 per order.')).toBeInTheDocument();
});There was a problem hiding this comment.
Good catch, fixed. ticketPerOrderLimit had the same quantity_2_sell ?? MAX, so with 0 stock the inventory went negative and the notice was suppressed. Changed to || MAX and added your per-order-notice test. Verified red before, green after.
| it('treats quantity_2_sell of 0 as unlimited stock', () => { | ||
| // 0 to sell = no cap on stock; per-order limit of 4 is the only bound | ||
| const ticket = { quantity_2_sell: 0, quantity_sold: 10, max_quantity_per_order: 4 }; | ||
| expect(getTicketMaxQuantity(ticket)).toBe(4); |
There was a problem hiding this comment.
@gcutrini The quantity_sold guard the PR description advertises ("guards quantity_sold when absent") has no test pinning it: every case in this file supplies quantity_sold, so reverting the ?? 0 in the helper (NaN propagates through Math.min, making ticketSelectionValid permanently false and blocking the Next button) fails nothing in the suite.
Why it matters: both API serializers currently always emit quantity_sold as an int, so this is contract-pinning rather than a reachable production bug — but an unpinned guard is the first thing a future refactor silently drops.
Suggested test (passes on this branch, fails with Expected: 5, Received: NaN when the ?? 0 guard is removed — verified locally both ways):
it('defaults quantity_sold to 0 when the API omits it', () => {
const ticket = { quantity_2_sell: 100, max_quantity_per_order: 5 };
expect(getTicketMaxQuantity(ticket)).toBe(5);
});There was a problem hiding this comment.
Added your suggested test. Confirmed it fails with NaN when the ?? 0 guard is removed and passes with it.
The ticketPerOrderLimit memo used the same quantity_2_sell ?? MAX that this branch fixed in getTicketMaxQuantity: with quantity_2_sell 0 the inventory went negative and the "limited to N per order" notice was suppressed, so the stepper capped with no explanation. Treat 0 as unlimited stock there too. Add a test pinning the quantity_sold ?? 0 guard in getTicketMaxQuantity, which had no coverage.
ref: https://app.clickup.com/t/86bbebyex
The Registration Lite widget marked a ticket type as Sold Out when its Max Quantity Per Order was 0. The API treats 0 as "no limit", but the widget took it literally as "max 0 per order".
Two layers had the same bug:
The Complimentary Sponsor ticket in the reported case (2900 to sell, 2832 sold, 68 left, Max Quantity Per Order 0) now shows availability.
Immediate workaround with no deploy: set the ticket type's Max Quantity Per Order to 1 in show admin.
Summary by CodeRabbit
Bug Fixes
Tests