Skip to content

[Card grants] Add status filter to card grants table - #14908

Open
ArushYadlapati wants to merge 2 commits into
hackclub:mainfrom
ArushYadlapati:arush/card-grant-status-filter
Open

[Card grants] Add status filter to card grants table#14908
ArushYadlapati wants to merge 2 commits into
hackclub:mainfrom
ArushYadlapati:arush/card-grant-status-filter

Conversation

@ArushYadlapati

Copy link
Copy Markdown

Summary of the problem

Fixes #14043.

The card grants table on an event's card grants page only has a text search box. There's no way to narrow the list down to, say, just the ones that have been activated vs. not activated, which is not great for events with a lot of grants.

Describe your changes

This PR creates a filter for the different status types that matches the existing filter UI (like the one on the ledger page). There are new CardGrant scopes for each state, and a filter_by_state scope that maps the status param to one of them, falling back to all on unknown values. Canceled grants are split into "Returned" and "Converted to reimbursement report" depending or not on whether a reimbursements report was created from them.

Filter:
image

Filter in action:
image

@ArushYadlapati
ArushYadlapati requested a review from a team September 6, 2026 10:21
@ArushYadlapati

ArushYadlapati commented Sep 6, 2026

Copy link
Copy Markdown
Author

Ready for review. Also, can I request a review from Claude?

@mattsoh

mattsoh commented Sep 6, 2026

Copy link
Copy Markdown
Member

@claude do review thing

@claude

claude Bot commented Sep 6, 2026

Copy link
Copy Markdown

Claude finished @mattsoh's task in 3m 26s —— View job


Code Review

  • Gather context (PR diff, changed files)
  • Understand request (code review requested by @mattsoh)
  • Review controller/model changes
  • Review view/helper changes
  • Review spec changes
  • Post final review

Reviewed the diff against origin/main. Overall this is a clean, well-scoped implementation that follows the existing filter pattern used elsewhere (reimbursements, transfers, etc.), and the new scopes/tests line up nicely with CardGrant#state/#state_text. One correctness gap and a couple of minor nits below.

Findings

1. "Frozen" filter misses cards that are inactive (never activated) — app/models/card_grant.rb:114

scope :frozen, -> { active.joins(:stripe_card).merge(StripeCard.frozen) }

StripeCard#frozen? is initially_activated? && stripe_status == "inactive", while StripeCard#inactive? is !initially_activated? && stripe_status == "inactive" (app/models/stripe_card.rb:231-240). But CardGrant#state/#state_text (app/models/card_grant.rb:150 and :168) group both into the "Frozen" badge:

elsif stripe_card.frozen? || stripe_card.inactive?
  "Frozen"

So a grant whose card was created but hasn't yet completed activation (inactive?, e.g. a brief window right after create_stripe_card) will display "Frozen" in the table but won't show up when filtering by "Frozen" — it also won't show up under any other filter option, since it doesn't match accepted (requires StripeCard.active) either. Worth aligning the scope with the display logic, e.g.:

scope :frozen, -> { active.joins(:stripe_card).where(stripe_cards: { stripe_status: "inactive" }) }

Fix this →

2. No filter bucket for fraud-flagged grants (minor/possibly intentional)
CardGrant#state_text has a distinct "Fraudulent" label (suspected_fraud?), but FILTERABLE_STATES (app/models/card_grant.rb:119-126) has no corresponding option — such grants will simply fall under whichever other scope they otherwise match (e.g. "Accepted"). This may be intentional since fraud status is computed from pre_authorization/card_grant_setting rather than a simple column, but worth confirming it's not an oversight.

3. Nit: naming scope :frozen shadows Object#frozen?app/models/card_grant.rb:114
Not a bug (it's a class-level method, no actual collision with the #frozen? instance method Rails relies on for readonly-record checks), but it reads oddly at a glance and could confuse future readers/tempt a .frozen? typo. Consider a less overloaded name if you want extra clarity, though it's fine to leave as-is given it mirrors the existing state_text wording.

4. Nit: typo in spec description — spec/controllers/card_grants_controller_spec.rb:68
"shows all grants when the status paramater is invalid"parameter.

Other observations (no action needed)

  • app/models/card_grant.rb:128-134 (filter_by_state) safely guards public_send(state) behind a whitelist check (FILTERABLE_STATES.key?(state)), so there's no arbitrary-method-call risk from user input.
  • app/views/card_grants/index.html.erb passing request.query_parameters into the turbo frame src is a nice, necessary fix so a direct link with ?status=... actually loads pre-filtered.
  • filter_chip_label in app/helpers/events_helper.rb:434 is a good generalization — it also fixes chip rendering for other existing [label, value]-style select filters (e.g. transfer_type in events_controller.rb:1684), which previously would have shown the raw value.
  • Test coverage (controller + model specs) is solid and matches the new scopes well.

Note: I wasn't able to run the test suite in this environment (bundle exec rspec requires interactive approval that isn't available here), so the above is based on static review only — worth confirming the specs pass, especially if scope #1 above is changed.

@ArushYadlapati

Copy link
Copy Markdown
Author

I'll start addressing this feedback right away!

@ArushYadlapati

Copy link
Copy Markdown
Author

@claude could you do the review thing again perchance 🙏🏽

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Grant Overview] Add status filter for grants

2 participants