Fixed Expectation to be both id & uuid - #155
Conversation
WalkthroughA new helper function for validating numeric IDs and an exported function to retrieve club data by either UUID or numeric ID were added. The Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant clubService
Caller->>clubService: getAllClubMemberships(clubId, params)
clubService->>clubService: findClubById(clubId, params)
alt clubId is UUID
clubService->>clubService: findByUUID(clubId)
else clubId is numeric
clubService->>DB: find club by numeric ID
else invalid
clubService->>Caller: throw 400 error
end
clubService->>DB: fetch memberships (with pagination/filtering/sorting)
clubService-->>Caller: return memberships
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Nitpick comments (1)
services/clubService.js (1)
305-328: Fix JSDoc parameter documentation.The parameter name was correctly updated from
clubUuidtoclubId, but the JSDoc comment still references the old name.Apply this diff to fix the documentation:
/** * Get all memberships for a club - * @param {String|Number} clubId Club ID or UUID + * @param {String|Number} clubId Club ID or UUID * @param {Object} params Query parametersThe implementation correctly uses the new
findClubByIdfunction. Good work maintaining the existing pagination and filtering logic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
services/clubService.js(5 hunks)
🔇 Additional comments (2)
services/clubService.js (2)
16-19: LGTM! Clean validation helper function.The
isNumericIdhelper correctly validates positive integers with comprehensive checks for NaN, integer type, and positive value.
869-869: LGTM! Properly exported the new function.The new
findClubByIdfunction is correctly added to the module exports.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
API-REST/Club/Club-get-info.bru (2)
34-34: Update documentation to reflect dual identifier support.The documentation still states "UUID of the club to retrieve" but the API now accepts both UUIDs and numeric IDs. Consider updating to something like "UUID or numeric ID of the club to retrieve".
- - `clubUuid`: UUID of the club to retrieve + - `clubUuid`: UUID or numeric ID of the club to retrieve
44-44: Update example to show both identifier types.The example URL still uses a UUID. Consider adding an example showing numeric ID usage as well.
## ExampleGET /clubs/550e8400-e29b-41d4-a716-446655440000?fields=name,description,status&include=supervisor
- GET /clubs/1?fields=name,description,status&include=supervisor
</blockquote></details> <details> <summary>controllers/clubController.js (1)</summary><blockquote> `24-24`: **Consider updating function name for clarity.** The function name `getClubByUuid` is now misleading since it accepts both UUIDs and numeric IDs. Consider renaming to something like `getClubById` or `getClubByIdentifier` for better clarity. Note: This may be a breaking change if the function is exported and used elsewhere, so evaluate the impact before making this change. </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used: CodeRabbit UI** **Review profile: CHILL** **Plan: Pro** <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between da464a3b603e7559dd4dcc67e76b77e30f54170f and 47116167e2ba670298d2bc3028566f772f53cc09. </details> <details> <summary>📒 Files selected for processing (3)</summary> * `API-REST/Club/Club-get-info.bru` (1 hunks) * `controllers/clubController.js` (1 hunks) * `services/clubService.js` (5 hunks) </details> <details> <summary>🚧 Files skipped from review as they are similar to previous changes (1)</summary> * services/clubService.js </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🧬 Code Graph Analysis (1)</summary> <details> <summary>controllers/clubController.js (1)</summary><blockquote> <details> <summary>services/clubService.js (8)</summary> * `clubData` (134-137) * `clubData` (164-167) * `clubData` (192-198) * `clubData` (313-313) * `clubData` (476-476) * `clubData` (614-614) * `clubData` (684-684) * `clubData` (805-805) </details> </blockquote></details> </details> </details> <details> <summary>🔇 Additional comments (3)</summary><blockquote> <details> <summary>API-REST/Club/Club-get-info.bru (1)</summary> `19-19`: **LGTM! Test parameter correctly validates numeric ID support.** The change from UUID to numeric ID "1" properly tests the new dual identifier functionality. </details> <details> <summary>controllers/clubController.js (2)</summary> `25-28`: **LGTM! Correctly implements dual identifier support.** The change from `findByUUID` to `findClubById` properly implements the new functionality to accept both UUIDs and numeric IDs. --- `52-53`: <details> <summary>❓ Verification inconclusive</summary> **Verify consistency of identifier handling across controller functions.** Several other functions in this controller still use `req.params.clubUuid` parameter. Ensure this is intentional if these functions should only accept UUIDs, or consider updating them to use the new dual identifier support if applicable. Also applies to: 68-68, 79-80, 111-112, 148-149, 163-164, 188-189 --- 🏁 Script executed: ```shell #!/bin/bash # Description: Check which service methods are called by other controller functions # and verify if they support dual identifiers echo "Checking service method calls in clubController.js:" rg -A 2 -B 1 "clubService\." controllers/clubController.js echo -e "\nChecking if other service methods support dual identifiers:" rg -A 5 -B 2 "(updateClub|deleteClub|getAllClubMemberships|createMembership|joinClub|leaveClub|resetClubTerm)" services/clubService.jsLength of output: 4597
🏁 Script executed:
#!/bin/bash # Extract implementation of findClubById to verify identifier handling rg -n -A 20 "const findClubById" services/clubService.jsLength of output: 761
Identifier Support Consistency
The service layer currently only supports dual identifiers (UUID and numeric ID) in
findClubById. All mutating operations (update, delete, join/leave, reset term, create membership, etc.) strictly validate and accept UUIDs only—any non-UUID will be rejected. The controller’s use ofreq.params.clubUuid(andmembershipUuid) is consistent with this design.Please confirm whether you intend to:
- Restrict all write operations to UUIDs (no change required),
- Or extend numeric-ID support across mutating methods.
If you choose the latter, you’ll need to update these service methods to handle numeric IDs similarly to
findClubById:• services/clubService.js –
updateClub,deleteClub,getAllClubMemberships,createMembership,joinClub,leaveClub,resetClubTerm
• controllers/clubController.js – corresponding endpoints usingreq.params.clubUuid
1 participant
Summary by CodeRabbit