fix: use correct Ticket field names in Grayquest fee header calculation - #22
Conversation
- Replace custom_selected_payment_gateway with payment_gateway_type
- Replace custom_selected_payment_gateway_name with payment_gateway_account
- Replace custom_seat_pricing_total with seat_pricing_total
- Replace custom_free_seats_count with free_seats_count
- Replace custom_chargeable_seats_count with chargeable_seats_count
The incorrect custom_ prefix caused _get_ticket_fee_headers and
_calculate_grade_breakdown to read None from the Ticket doc, making
the fee_headers payload always fall back to {total_payable, current_payable}
instead of using the grade-specific fee header from Event Listing grade_details.
sankarsubramaniankvs
left a comment
sankarsubramaniankvs
left a comment
There was a problem hiding this comment.
🔍 Code Review — PR #22
Reviewer: OpenClaw Bot
Verdict: 🔄 Request Changes
Files reviewed: 1 | Issues found: 🔴 0 🟠 1 🟡 0 💡 0
Summary
The fix itself looks directionally right: the old custom_-prefixed field reads were clearly wrong if those fields do not exist on Ticket, and switching to the actual Ticket field names should unblock the grade-specific fee-header path. The scope is also clean and focused.
But this is still a backend behavior fix that changes payload construction logic, and the PR only includes a manual test plan. Under the review rule for backend changes, missing automated coverage is a merge blocker.
🌟 What's Done Well
- The change is narrowly scoped to the broken field lookups instead of mixing in unrelated cleanup.
- The PR description explains the failure mode clearly, including the exact fallback behavior this bug was causing.
🟠 Major
grayquest/utils/utils.py — Backend behavior fix has no automated regression coverage
This PR changes the logic that builds Grayquest fee headers and per-grade breakdown values, but it ships without a unit test or an update to an existing relevant test. That leaves the corrected field mapping and the fallback behavior unprotected against future regressions.
Suggested fix: add backend tests covering at least:
- correct fee-header resolution when the Ticket has matching grade details and valid gateway fields
- fallback to
{total_payable, current_payable}when no matching grade detail exists - multi-grade breakdown using the corrected seat/count fields
File-by-File Summary
| File | Status | Issues |
|---|---|---|
grayquest/utils/utils.py |
🟠 | Missing required backend regression coverage for payload logic change |
🤖 Reviewed by OpenClaw Bot — Unity Edu
- Test grade-specific fee header returned when grade + gateway match - Test fallback to total_payable/current_payable when no match - Test empty seats fallback - Test multi-grade per-grade fee header breakdown - Test multi-grade fallback when no grade_details configured
🔍 Code Review — PR #22Reviewer: OpenClaw Bot SummaryRechecked from the latest head. The previous blocker is addressed: the PR now includes backend unit coverage for the corrected Ticket field mappings, the grade-specific fee-header path, and the fallback behavior when no matching grade detail exists. The fix remains narrowly scoped, and the new tests cover the behavior that was actually broken instead of only checking happy-path output. Clear now. 🌟 What's Done Well
File-by-File Summary
|
2 participants
Summary
_get_ticket_fee_headersand_calculate_grade_breakdownwere reading Ticket fields using incorrectcustom_-prefixed field namesNone{total_payable, current_payable}values instead of the grade-specific fee header sourced from Event Listinggrade_detailsFix
custom_-prefixed field names with the actual field names present on the Ticket DocTypegrade_detailsTest Plan
grade_detailspopulated