fix: move feedback stats updates to backend function#520
fix: move feedback stats updates to backend function#520riddhima25bet10005-a11y wants to merge 13 commits into
Conversation
📝 WalkthroughWalkthroughClient submitFeedback now writes only the feedback document via a Firestore transaction; a new Cloud Function onFeedbackSubmit (Firestore onCreate) validates feedback and performs protected batched updates (event stats, club reputation, user points, feedbackRequests completion). Tests, exports, logging, a live-preview HTML, PR bodies, and automation scripts were added/updated. ChangesFeedback Submission Refactoring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
app/src/lib/__tests__/feedbackService.test.jsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. app/src/lib/feedbackService.jsESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox. cloud-functions/lib/dailyDigest.jsOops! Something went wrong! :( ESLint: 10.4.1 TypeError [ERR_IMPORT_ATTRIBUTE_MISSING]: Module "file:///cloud-functions/.eslintrc.json?mtime=1780413665657" needs an import attribute of "type: json"
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/lib/feedbackService.js (1)
19-36:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDon't overwrite a document that's processed by a create-only trigger.
This path is always
events/{eventId}/feedback/{userId}, so a second submit becomes an update.onFeedbackSubmitonly runs on creates, which means retries or edits can change the feedback document without reapplying stats, reputation, points, or request completion. Either enforce create-only semantics here or make the backend handler idempotent for updates too.🤖 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/lib/feedbackService.js` around lines 19 - 36, The code currently calls setDoc(feedbackRef, payload) which will overwrite an existing events/{eventId}/feedback/{userId} doc and bypass the onFeedbackSubmit create-only trigger; change this to a create-only write by first checking for existence (getDoc/transaction) and only writing if the doc does not exist—e.g., use a Firestore transaction or getDoc(feedbackRef) and if the doc.exists() then return/error, otherwise setDoc(feedbackRef, payload); reference feedbackRef, payload, feedbackRequestId and the onFeedbackSubmit create-only behavior when making the change.
🤖 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 `@cloud-functions/src/onFeedbackSubmit.ts`:
- Around line 18-65: The batch currently trusts client-supplied fields
(attended, eventRating, clubRating, clubId, feedbackRequestId) before writing
via eventRef, clubRef, userRef, requestRef and statsUpdate; change the code to
first read canonical documents (event doc for eventId, feedbackRequest doc for
feedbackRequestId, and event's club owner) and validate ownership and ranges
server-side: confirm the feedbackRequest belongs to userId and references
eventId, ensure clubId matches the event's club, clamp/validate eventRating and
clubRating to allowed ranges, and verify attended is consistent with
event/registration state; only then mutate statsUpdate, reputation increment,
user points and request status in the batch. Ensure all checks occur prior to
calling batch.set and abort/throw if validation fails.
- Around line 68-73: The current try/catch around batch.commit() in
onFeedbackSubmit.ts swallows failures—after logging the error the function
returns normally—so change the catch to rethrow the caught error (or throw a new
Error that includes the original) after logging so the invocation fails and
retries/alerts can occur; update the catch block that wraps await batch.commit()
(referencing batch.commit(), eventId and userId in the log) to log the error and
then rethrow it.
---
Outside diff comments:
In `@app/src/lib/feedbackService.js`:
- Around line 19-36: The code currently calls setDoc(feedbackRef, payload) which
will overwrite an existing events/{eventId}/feedback/{userId} doc and bypass the
onFeedbackSubmit create-only trigger; change this to a create-only write by
first checking for existence (getDoc/transaction) and only writing if the doc
does not exist—e.g., use a Firestore transaction or getDoc(feedbackRef) and if
the doc.exists() then return/error, otherwise setDoc(feedbackRef, payload);
reference feedbackRef, payload, feedbackRequestId and the onFeedbackSubmit
create-only behavior when making the change.
🪄 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: 069e2f9b-dcca-49cc-9b09-a2a82d7e7146
📒 Files selected for processing (3)
app/src/lib/feedbackService.jscloud-functions/src/index.tscloud-functions/src/onFeedbackSubmit.ts
|
Hey @roshankumar0036singh |
…ove unused import
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/src/lib/__tests__/feedbackService.test.js (1)
24-45: ⚡ Quick winAssert the transaction write contract, not just that it ran.
These tests currently pass as long as
submitFeedback()callsrunTransaction()and resolves. They do not verify the PR’s key guarantee: the client performs exactly one feedback-doc write and does not regress into other client-side writes. Please assertmockTransaction.setcall count and payload for both attended and no-show flows.Suggested test tightening
test('submits attended feedback successfully', async () => { + let mockTransaction; runTransaction.mockImplementationOnce(async (_, callback) => { - const mockTransaction = { - get: jest.fn().mockResolvedValue({ exists: () => false }), - set: jest.fn(), - }; - return await callback(mockTransaction); + mockTransaction = { + get: jest.fn().mockResolvedValue({ exists: () => false }), + set: jest.fn(), + }; + return await callback(mockTransaction); }); const result = await submitFeedback({ feedbackRequestId: 'req1', eventId: 'event1', @@ expect(runTransaction).toHaveBeenCalled(); + expect(mockTransaction.set).toHaveBeenCalledTimes(1); + expect(mockTransaction.set).toHaveBeenCalledWith( + expect.anything(), + expect.objectContaining({ + feedbackRequestId: 'req1', + eventId: 'event1', + clubId: 'club1', + userId: 'user1', + attended: true, + eventRating: 5, + clubRating: 4, + feedback: 'Great event', + }), + ); expect(result).toEqual({ success: true }); });Also applies to: 48-67
🤖 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/lib/__tests__/feedbackService.test.js` around lines 24 - 45, The tests currently only verify runTransaction was invoked; update the submitFeedback tests to assert the transaction write contract by checking that the mockTransaction.set method on the object passed into the runTransaction callback is called exactly once and with the expected document path and payload for both attended and no-show flows: in the attended test confirm mockTransaction.set called once with the feedback document containing attended: true, eventRating, clubRating, feedback and correct ids (feedbackRequestId, eventId, clubId, userId); in the no-show test assert mockTransaction.set called once with attended: false and the no-show-specific payload (no ratings), and keep existing runTransaction and result assertions. Ensure you reference the mockTransaction used in runTransaction.mockImplementationOnce and the submitFeedback function when adding these assertions.
🤖 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/lib/__tests__/feedbackService.test.js`:
- Around line 24-45: The tests currently only verify runTransaction was invoked;
update the submitFeedback tests to assert the transaction write contract by
checking that the mockTransaction.set method on the object passed into the
runTransaction callback is called exactly once and with the expected document
path and payload for both attended and no-show flows: in the attended test
confirm mockTransaction.set called once with the feedback document containing
attended: true, eventRating, clubRating, feedback and correct ids
(feedbackRequestId, eventId, clubId, userId); in the no-show test assert
mockTransaction.set called once with attended: false and the no-show-specific
payload (no ratings), and keep existing runTransaction and result assertions.
Ensure you reference the mockTransaction used in
runTransaction.mockImplementationOnce and the submitFeedback function when
adding these assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 30756cf6-7216-426a-9866-96c5fb25a530
📒 Files selected for processing (23)
app/src/lib/__tests__/feedbackService.test.jsapp/src/lib/feedbackService.jscloud-functions/lib/dailyDigest.jscloud-functions/lib/index.jscloud-functions/src/index.tscloud-functions/src/onFeedbackSubmit.test.tscloud-functions/src/onFeedbackSubmit.tsfix_tests_and_push.ps1live-preview-map.htmlpr_body.mdpr_body_215.mdpr_body_219.mdpr_script.ps1pr_script_359.ps1pr_script_364.ps1pr_script_364_fix.ps1push_cloud_fixes.ps1push_cloud_fixes2.ps1push_cloud_fixes3.ps1push_cloud_fixes4.ps1push_cloud_fixes5.ps1rebuild_and_pull.ps1resolve_and_push.ps1
💤 Files with no reviewable changes (16)
- push_cloud_fixes.ps1
- push_cloud_fixes3.ps1
- resolve_and_push.ps1
- push_cloud_fixes5.ps1
- fix_tests_and_push.ps1
- pr_body.md
- push_cloud_fixes2.ps1
- pr_script_364_fix.ps1
- pr_script_364.ps1
- push_cloud_fixes4.ps1
- rebuild_and_pull.ps1
- pr_body_219.md
- live-preview-map.html
- pr_body_215.md
- pr_script_359.ps1
- pr_script.ps1
🚧 Files skipped from review as they are similar to previous changes (6)
- cloud-functions/src/index.ts
- cloud-functions/lib/dailyDigest.js
- app/src/lib/feedbackService.js
- cloud-functions/lib/index.js
- cloud-functions/src/onFeedbackSubmit.test.ts
- cloud-functions/src/onFeedbackSubmit.ts
|
Hi @roshankumar0036singh |
|
@riddhima25bet10005-a11y clean up the pr you have pusehd too many ai related files |
|
|
Hi @roshankumar0036singh |
2 participants
Closes #519
Moved the privileged feedback stats and reputation updates to a Cloud Function to avoid client-side permission issues.
Summary by CodeRabbit