feat: implement multi-transaction support for event creation and update in Firestore (#333)#426
Conversation
|
Warning Review limit reached
More reviews will be available in 55 minutes and 46 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughCreateEvent now uses a Firestore transaction to atomically write an event, an attendance/bootstrap document, and update the creator's organizer stats; an app Jest test verifies the transaction; Firestore rules gained post-write ownership checks; rules tests were refactored to use batched, timestamped setups. ChangesAtomic Event Creation Transaction
Sequence DiagramsequenceDiagram
participant App as CreateEvent App
participant Txn as Firestore Transaction
participant Events as events collection
participant Attendance as events/{eventId}/attendance
participant Users as users collection
App->>Txn: runTransaction(callback)
Txn->>Events: set(eventRef, eventData)
Txn->>Attendance: set(bootstrapRef, placeholderData)
Txn->>Users: update(userRef, {organizerStats.eventsCreated: increment(1), lastEventCreatedAt: serverTimestamp()})
Txn-->>App: commit success (all or nothing)
App->>App: Alert.alert('Success', 'Event Created!')
App->>App: navigation.goBack()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.
🧹 Nitpick comments (3)
app/src/screens/__tests__/CreateEvent.test.js (2)
141-152: 💤 Low valueMinor style improvements per static analysis.
The logic is correct, but optional chaining and
.at()would be cleaner:♻️ Optional style improvements
const mockDoc = jest.fn((firstArg, ...segments) => { - if (firstArg && firstArg.__isCollectionRef && segments.length === 0) { + if (firstArg?.__isCollectionRef && segments.length === 0) { return { id: 'event_tx_1', path: `${firstArg.path}/event_tx_1` }; } const parts = [...(typeof firstArg === 'string' ? [firstArg] : []), ...segments]; return { - id: parts[parts.length - 1], + id: parts.at(-1), path: parts.join('/'), }; });🤖 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 `@app/src/screens/__tests__/CreateEvent.test.js` around lines 141 - 152, The mockDoc implementation can be simplified for clarity: in the jest mock function named mockDoc, replace the explicit firstArg and property check with optional chaining (firstArg?.__isCollectionRef) and when returning the last segment use .at(-1) instead of parts[parts.length - 1]; keep the existing logic that constructs parts from a string firstArg and ...segments and preserve the special-case return for collection refs (id 'event_tx_1' and path `${firstArg.path}/event_tx_1`), but rewrite those checks using optional chaining and .at(-1) for cleaner style.
126-126: ⚖️ Poor tradeoffConsider adding test coverage for new organizer documents.
The mock always returns
{ exists: () => true }, so the fallback path inCreateEvent.js(lines 358-366) wheretransaction.setwithmerge: trueis used for new organizers is not exercised. Consider adding a second test case wheremockTransactionGetreturns{ exists: () => false }to verify the initialization logic.🤖 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 `@app/src/screens/__tests__/CreateEvent.test.js` at line 126, The test currently only covers the existing-organizer path because mockTransactionGet always returns exists: true; add a second test in CreateEvent.test.js that sets mockTransactionGet to return { exists: () => false } (e.g. via mockImplementationOnce) so the fallback branch in CreateEvent.js is exercised, then run the transaction and assert that transaction.set was called for the organizer document with merge: true and the expected initial organizer fields; reference mockTransactionGet, transaction.set and the new-organizer initialization branch in CreateEvent.js.tests/firestore.rules.test.ts (1)
84-111: ⚡ Quick winThis happy-path test does not exercise the new
isEventOwnerAfter()branch.Because this request is authenticated with
{ club: true }, the attendance write is already allowed by the existingisClub()check infirestore.rules. This test will still pass even ifisEventOwnerAfter()is broken or removed, so it does not actually cover the new authorization path introduced by this PR. Please add a case that would fail without the post-write helper.🤖 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 `@tests/firestore.rules.test.ts` around lines 84 - 111, The current happy-path test 'Club admin atomically creates event + attendance placeholder + organizer stats -> allowed' never hits the new isEventOwnerAfter() branch because it authenticates with { club: true }; add a new test case that authenticates as a normal user (no club role) using getFirestoreContext without the club flag, then perform an atomic batch that creates the event document (ownerId set to the test user) plus the attendance placeholder and author updates so the authorization must rely on isEventOwnerAfter() to allow the attendance write; ensure the test asserts success with assertSucceeds(batch.commit()) and name it to indicate it covers the isEventOwnerAfter() path so failures will catch regressions in isEventOwnerAfter().
🤖 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 `@app/src/screens/__tests__/CreateEvent.test.js`:
- Around line 141-152: The mockDoc implementation can be simplified for clarity:
in the jest mock function named mockDoc, replace the explicit firstArg and
property check with optional chaining (firstArg?.__isCollectionRef) and when
returning the last segment use .at(-1) instead of parts[parts.length - 1]; keep
the existing logic that constructs parts from a string firstArg and ...segments
and preserve the special-case return for collection refs (id 'event_tx_1' and
path `${firstArg.path}/event_tx_1`), but rewrite those checks using optional
chaining and .at(-1) for cleaner style.
- Line 126: The test currently only covers the existing-organizer path because
mockTransactionGet always returns exists: true; add a second test in
CreateEvent.test.js that sets mockTransactionGet to return { exists: () => false
} (e.g. via mockImplementationOnce) so the fallback branch in CreateEvent.js is
exercised, then run the transaction and assert that transaction.set was called
for the organizer document with merge: true and the expected initial organizer
fields; reference mockTransactionGet, transaction.set and the new-organizer
initialization branch in CreateEvent.js.
In `@tests/firestore.rules.test.ts`:
- Around line 84-111: The current happy-path test 'Club admin atomically creates
event + attendance placeholder + organizer stats -> allowed' never hits the new
isEventOwnerAfter() branch because it authenticates with { club: true }; add a
new test case that authenticates as a normal user (no club role) using
getFirestoreContext without the club flag, then perform an atomic batch that
creates the event document (ownerId set to the test user) plus the attendance
placeholder and author updates so the authorization must rely on
isEventOwnerAfter() to allow the attendance write; ensure the test asserts
success with assertSucceeds(batch.commit()) and name it to indicate it covers
the isEventOwnerAfter() path so failures will catch regressions in
isEventOwnerAfter().
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1ef59c1f-5fd5-4d9d-90ee-2a142596fc3e
📒 Files selected for processing (4)
app/src/screens/CreateEvent.jsapp/src/screens/__tests__/CreateEvent.test.jsfirestore.rulestests/firestore.rules.test.ts
|
@roshankumar0036singh please review |
|
@vansh-09 createdAt uses ISO string, instead use serverTimestamp() |
|
@vansh-09 Missing newline at end of firestore.rules |
|
isEventOwnerAfter calls getAfter() twice in the same function, which counts against Firestore rules read limits |
|
why is the mockTransactionGet is hardcoded to exists: () => true |
|
If the transaction retries due to , nowIso will carry the timestamp from the first attempt fix this |
|
@vansh-09 fix the issue flagged by osnar mean there are duplicate code blocks in pr remoev them |
|
|
@roshankumar0036singh please check |
roshankumar0036singh
merged commit e5761d1
into
roshankumar0036singh:main
2 participants
Description
CreateEvent.jsnow performs:firestore.rulesto allow attendance writes that occur in the same atomic commit as event creation (using post-commit ownership checks).firestore.rules.test.tsCreateEvent.test.jsFixes #333
Type of change
How Has This Been Tested?
Ran:
npx jest src/screens/tests/CreateEvent.test.js --runInBand --coverage=false
Result:
Checklist:
Summary by CodeRabbit
Bug Fixes
Security Updates
Tests