fix(campaign): return 409 on double claim instead of 200 - #667
Closed
scarface-dev1 wants to merge 1 commit into
Closed
fix(campaign): return 409 on double claim instead of 200#667scarface-dev1 wants to merge 1 commit into
scarface-dev1 wants to merge 1 commit into
Conversation
Closes ritik4ever#583 The POST /api/campaigns/:id/claim endpoint now checks if the campaign has already been claimed before processing. If claimedAt is already set, the endpoint returns a 409 Conflict with CAMPAIGN_ALREADY_CLAIMED error code. Changes: - campaignStore.ts: reconcileOnChainClaim throws toServiceError with 409 / CAMPAIGN_ALREADY_CLAIMED instead of silently returning campaign - api.test.ts: duplicate claim test expects 409 with error code/message verification, plus history check (1 claim event before/after) - mutation.test.ts: "second claim is idempotent" updated to assert 409 throw with "Campaign already claimed" message
|
Someone is attempting to deploy a commit to the ritik4ever's projects Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThe claim flow now rejects duplicate claims with ChangesCampaign claim lifecycle
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
![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
🤖 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 `@backend/src/services/campaignStore.ts`:
- Around line 1002-1004: Move claim detection into the existing db.transaction
by conditionally updating campaigns through the claim write using campaign id
and claimed_at IS NULL. Check the update’s affected-row count before recordEvent
and throw toServiceError with CAMPAIGN_ALREADY_CLAIMED when fewer than one row
was updated; remove reliance on the pre-transaction campaign.claimedAt check.
🪄 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 Plus
Run ID: 17bc7fb7-ed5d-4ba1-a516-75f216de1a95
📒 Files selected for processing (3)
backend/src/api.test.tsbackend/src/services/__tests__/mutation.test.tsbackend/src/services/campaignStore.ts
Comment on lines
1002
to
1004
| if (campaign.claimedAt) { | ||
| return campaign; | ||
| throw toServiceError('Campaign already claimed.', 409, 'CAMPAIGN_ALREADY_CLAIMED'); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Files matching campaignStore:\n'
fd -a 'campaignStore\.ts$' . || true
file="$(fd 'campaignStore\.ts$' . | head -n1 || true)"
if [ -n "$file" ]; then
printf '\nLine count:\n'
wc -l "$file"
printf '\nRelevant section around claim event:\n'
sed -n '960,1045p' "$file" | nl -ba -v960
printf '\nSchema/migrations mentioning claimed_at/campaigns:\n'
rg -n --glob '*.sql' --glob '*.ts' --glob '*.js' 'claimed_at|CREATE TABLE .*campaign|campaigns' .
fiRepository: ritik4ever/stellar-goal-vault
Length of output: 391
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="backend/src/services/campaignStore.ts"
printf 'Relevant section around claim event:\n'
sed -n '960,1045p' "$file"
printf '\nSchema/migrations mentioning claimed_at/campaigns:\n'
rg -n --glob '*.sql' --glob '*.ts' --glob '*.js' 'claimed_at|CREATE TABLE .*campaign|campaigns' .
printf '\nCheck for claimed_at uniqueness/index definitions:\n'
rg -n --glob '*.sql' --glob '*.ts' --glob '*.js' 'UNIQUE|INDEX| claimed_at|campaigns' .Repository: ritik4ever/stellar-goal-vault
Length of output: 50385
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Campaign schema and migrations:\n'
sed -n '78,112p' backend/src/services/db.ts
sed -n '158,260p' backend/src/services/db.ts
printf '\nrecordEvent implementation/usages:\n'
rg -n "function recordEvent|const recordEvent|recordEvent\\(" backend/src/services/campaignStore.ts backend/src/services/db.ts
printf '\nConcurrent duplicate claim behavior probe (SQLite, no repository code executed):\n'
if command -v sqlite3 >/dev/null 2>&1; then
tmp="$(mktemp)"
cat > "$tmp" <<'SQL'
PRAGMA foreign_keys=off;
BEGIN;
CREATE TABLE campaign_events (
campaign_id INTEGER PRIMARY KEY,
event_type TEXT,
event_data JSON NOT NULL
);
CREATE TABLE campaigns (id TEXT PRIMARY KEY, claimed_at INTEGER);
INSERT INTO campaigns(id, claimed_at) VALUES ('c1', NULL);
COMMIT;
PRAGMA foreign_keys=off;
BEGIN IMMEDIATE;
INSERT INTO campaign_events(campaign_id, event_type, event_data) VALUES ('c1', 'claimed', '{}');
UPDATE campaigns SET claimed_at = 2 WHERE id = 'c1' AND claimed_at IS NULL;
COMMIT;
SELECT sqlite3_changes();
SELECT event_data, campaign_id FROM campaign_events ORDER BY rowid;
SELECT claimed_at FROM campaigns WHERE id='c1';
SQL
sqlite3 ":memory:" < "$tmp"
rm -f "$tmp"
else
echo "sqlite3 not available"
fiRepository: ritik4ever/stellar-goal-vault
Length of output: 5228
Make claim detection atomic with the claim write.
campaign.claimedAt is read before db.transaction, so concurrent claimers can both observe an unclaimed campaign and append duplicate claimed events. Keep the current transaction shape but make the update conditional and fail before recordEvent:
UPDATE campaigns SET claimed_at = ? WHERE id = ? AND claimed_at IS NULLThen throw CAMPAIGN_ALREADY_CLAIMED when the affected row count is less than 1.
🤖 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 `@backend/src/services/campaignStore.ts` around lines 1002 - 1004, Move claim
detection into the existing db.transaction by conditionally updating campaigns
through the claim write using campaign id and claimed_at IS NULL. Check the
update’s affected-row count before recordEvent and throw toServiceError with
CAMPAIGN_ALREADY_CLAIMED when fewer than one row was updated; remove reliance on
the pre-transaction campaign.claimedAt check.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #583
Summary
Fixes a bug where calling
POST /api/campaigns/:id/claimon an already-claimed campaign returned200 OKwith a success response instead of rejecting the request.Root Cause
reconcileOnChainClaim()incampaignStore.tshad a silent-return guard whenclaimedAtwas already set:Changes
backend/src/services/campaignStore.ts
claimedAtguard fromreturn campaigntothrow toServiceError('Campaign already claimed.', 409, 'CAMPAIGN_ALREADY_CLAIMED')softDeleteCampaign(409 /ALREADY_DELETED)backend/src/api.test.ts
backend/src/services/tests/mutation.test.ts
Acceptance Criteria
Security Notes
No security impact. The change enforces a correctness invariant. The error is properly caught by the existing Express error middleware and returned with a structured JSON error response.
Summary by CodeRabbit