Minor fixes #38
Minor fixes #38
Conversation
WalkthroughcreateAMember now requires and forwards a provider field; updateAMember delegates password updates to a new updatePassword service and otherwise updates member fields; updateMember removes an explicit existence check and includes name in update payload; UpdateMemberPayload gains optional password. Tests and docs adjusted accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Controller
participant MemberService
participant AccountModel
participant MemberModel
Client->>Controller: POST /members (body includes provider)
Controller->>Controller: validate email,name,password,passoutYear,provider
alt missing required field
Controller-->>Client: 400 Required fields absent
else
Controller->>MemberService: createMember(..., provider)
MemberService->>MemberModel: create(...)
MemberService-->>Controller: created
Controller-->>Client: 201 Created
end
Client->>Controller: PATCH /members/:id (payload may include password)
Controller->>Controller: handle image upload -> parsedBody
alt password present
Controller->>MemberService: updatePassword(id, password)
MemberService->>AccountModel: findFirst(memberId)
alt no account
MemberService-->>Controller: 404 Associated account not found
else
MemberService->>AccountModel: update(password)
MemberService-->>Controller: account updated
end
else
Controller->>MemberService: updateMember(id, parsedBody)
MemberService->>MemberModel: update(member fields)
MemberService-->>Controller: member updated
end
Controller-->>Client: 200 OK (with updated member)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
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. CodeRabbit Commands (Invoked using PR/Issue comments)Type 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: 2
🔭 Outside diff range comments (1)
src/services/member.service.ts (1)
73-96: Consider using database transactions for data consistency.Since the function can update both
accountandmembertables, consider wrapping the operations in a transaction to ensure data consistency in case of failures.export const updateMember = async ( id: string, payload: UpdateMemberPayload ) => { + return await prisma.$transaction(async (tx) => { const { name, password, ...rest } = payload; if (password) { - const account = await prisma.account.findFirst({ + const account = await tx.account.findFirst({ where: { memberId: id }, }); if (!account) throw new ApiError("Associated account not found", 404); - await prisma.account.update({ + await tx.account.update({ where: { id: account.id }, data: { password }, }); } const dataToUpdate = Object.fromEntries( Object.entries({ name, ...rest }).filter(([_, v]) => v !== undefined) ); if (JSON.stringify(dataToUpdate) === "{}" && !password) throw new ApiError("No fields passed", 400); if (JSON.stringify(dataToUpdate) !== "{}") { - return await prisma.member.update({ + return await tx.member.update({ where: { id }, data: dataToUpdate, }); } + }); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/controllers/member.controller.ts(1 hunks)src/services/member.service.ts(1 hunks)src/types/members.d.ts(1 hunks)tests/Member.test.ts(8 hunks)
🔇 Additional comments (5)
src/types/members.d.ts (1)
18-18: LGTM! Clean interface extension.The optional
passwordfield addition is well-structured and aligns with the update functionality in the service layer.tests/Member.test.ts (4)
16-19: Good addition of account model mocks.The account model mocks with
findFirstandupdatemethods properly support the new password update functionality in the service layer.
46-46: Correct test data update.Adding the
providerfield to the test request body aligns with the new validation requirement in the controller.
109-110: Improved test assertions.The direct method calls in assertions are cleaner than using intermediate spy variables and improve test readability.
Also applies to: 115-116
161-161: Clean formatting improvements.The single-line mock call formatting improves code readability while maintaining the same functionality.
Also applies to: 164-165, 167-167
| const {email, name, password, passoutYear, provider} = req.body; | ||
|
|
||
| if (!email || !name || !password || !passoutYear) { | ||
| if (!email || !name || !password || !passoutYear || !provider) { |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Enhanced validation looks good.
The addition of provider validation is consistent with the existing pattern and strengthens the member creation flow.
Verify that all API clients and tests include the provider field when calling this endpoint:
🏁 Script executed:
#!/bin/bash
# Description: Search for createAMember API calls to ensure they include provider field
# Expected: All calls should include provider field
echo "Searching for createAMember API calls..."
rg -A 10 -B 5 "createAMember|/members.*POST" --type js --type ts --type json
echo -e "\nSearching for member creation requests..."
rg -A 15 -B 5 "email.*name.*password.*passoutYear" --type js --type tsLength of output: 6644
Tests and API docs must be updated for the new provider validation
The added provider—and previously existing passoutYear—fields now break the createAMember tests and out-of-date JSDoc. Please:
• tests/Member.test.ts
– Include both passoutYear and provider in the req.body fixtures for your createAMember tests.
– Example diff:
it('should respond with 201 and created member', async () => {
- const req = {
- body: {
- email: 'shruti@example.com',
- name: 'Shruti',
- password: 'password123',
- }
- } as Request;
+ const req = {
+ body: {
+ email: 'shruti@example.com',
+ name: 'Shruti',
+ password: 'password123',
+ passoutYear: 2025,
+ provider: 'local',
+ }
+ } as Request;• src/routes/members.ts
– Update your route JSDoc to surface the new parameters:
* @apiBody {String} [name] Full name of the member.
+ * @apiBody {Number} passoutYear Graduation year of the member.
+ * @apiBody {String} provider Authentication provider for the member.
* @apiBody {File} [file] Profile photo file (field name: "file").Once tests and docs are aligned with the controller’s required fields, the build will pass and the validation enhancement remains effective.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!email || !name || !password || !passoutYear || !provider) { | |
| // tests/Member.test.ts | |
| it('should respond with 201 and created member', async () => { | |
| const req = { | |
| body: { | |
| email: 'shruti@example.com', | |
| name: 'Shruti', | |
| password: 'password123', | |
| passoutYear: 2025, | |
| provider: 'local', | |
| } | |
| } as Request; | |
| const res = await createAMember(req, resStub); | |
| expect(res.status).toHaveBeenCalledWith(201); | |
| // …other assertions… | |
| }); |
| if (!email || !name || !password || !passoutYear || !provider) { | |
| // src/routes/members.ts | |
| /** | |
| * @apiBody {String} [name] Full name of the member. | |
| * @apiBody {Number} passoutYear Graduation year of the member. | |
| * @apiBody {String} provider Authentication provider for the member. | |
| * @apiBody {File} [file] Profile photo file (field name: "file"). | |
| */ |
🤖 Prompt for AI Agents
In src/controllers/member.controller.ts at line 49, the validation now requires
'provider' and 'passoutYear' fields, which breaks existing tests and outdated
JSDoc. To fix this, update tests/Member.test.ts to include 'provider' and
'passoutYear' in the req.body fixtures for createAMember tests, and modify
src/routes/members.ts to add these new parameters in the route JSDoc. This will
align tests and documentation with the controller's validation requirements and
ensure the build passes.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
src/services/member.service.ts
Outdated
| if (password) { | ||
| const account = await prisma.account.findFirst({ | ||
| where: { memberId: id }, | ||
| }); | ||
|
|
||
| if (!account) throw new ApiError("Associated account not found", 404); | ||
|
|
||
| if (!member) { | ||
| throw new ApiError("Member not found", 404); | ||
| return await prisma.account.update({ | ||
| where: { id: account.id }, | ||
| data: { password }, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Critical: Early return prevents updating member fields when password is provided.
The current logic returns early after updating the password, which means if a client sends both password and other member fields (like name, bio, etc.), only the password will be updated and other fields will be ignored.
Consider this fix to handle both password and member field updates:
export const updateMember = async (
id: string,
payload: UpdateMemberPayload
) => {
const { name, password, ...rest } = payload;
+ let passwordUpdated = false;
if (password) {
const account = await prisma.account.findFirst({
where: { memberId: id },
});
if (!account) throw new ApiError("Associated account not found", 404);
- return await prisma.account.update({
+ await prisma.account.update({
where: { id: account.id },
data: { password },
});
+ passwordUpdated = true;
}
const dataToUpdate = Object.fromEntries(
Object.entries({ name, ...rest }).filter(([_, v]) => v !== undefined)
);
- if (JSON.stringify(dataToUpdate) === "{}" && !password) throw new ApiError("No fields passed", 400);
+ if (JSON.stringify(dataToUpdate) === "{}" && !passwordUpdated) throw new ApiError("No fields passed", 400);
+ if (JSON.stringify(dataToUpdate) === "{}") {
+ return { passwordUpdated };
+ }
return await prisma.member.update({
where: { id },
data: dataToUpdate,
});
};🤖 Prompt for AI Agents
In src/services/member.service.ts around lines 73 to 84, the code returns
immediately after updating the password, preventing simultaneous updates to
other member fields. Refactor the logic to first update the password if
provided, then proceed to update other member fields without returning early.
Combine both updates in a way that ensures all requested changes are applied
before returning the final result.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/services/member.service.ts (1)
73-77: Including name in dataToUpdate is correct; simplify empty-payload check.The current empty check via JSON.stringify is brittle.
Apply this diff:
- if (JSON.stringify(dataToUpdate) === "{}") throw new ApiError("No fields passed", 400); + if (Object.keys(dataToUpdate).length === 0) { + throw new ApiError("No fields passed", 400); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/controllers/member.controller.ts(2 hunks)src/routes/members.ts(6 hunks)src/services/member.service.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/routes/members.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/services/member.service.ts (2)
src/utils/apiError.ts (1)
ApiError(6-17)src/types/members.d.ts (1)
UpdateMemberPayload(6-18)
| if (!email || !name || !password || !passoutYear || !provider) { | ||
| throw new ApiError("Required fields absent", 400); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Don't require password for OAuth signups; refine validation to depend on provider.
Requiring password unconditionally breaks Google/GitHub signups. Require it only when provider === "credentials".
Apply this diff:
- if (!email || !name || !password || !passoutYear || !provider) {
+ if (!email || !name || !passoutYear || !provider || (provider === "credentials" && !password)) {
throw new ApiError("Required fields absent", 400);
}Run to verify no callers use unsupported provider values (e.g., "local") and that tests align with status code 400:
🏁 Script executed:
#!/bin/bash
# Check for unsupported provider literals like "local"
rg -n "provider\\s*:\\s*['\"]local['\"]" -S --glob '!node_modules' .
# Check for tests expecting status 402 for 'Required fields absent'
rg -n "Required fields absent" --glob '!node_modules' tests | sed 's/:/ /' | while read -r f l _; do sed -n "${l}p" "$f"; doneLength of output: 276
🏁 Script executed:
#!/bin/bash
# Search for any code comparing provider to "local"
rg -nE 'provider\s*===?\s*["'"'"']local["'"'"']' -S --glob '!node_modules' .
# Show tests that reference the ApiError message "Required fields absent" with context
rg -n -A2 "Required fields absent" -S --glob '!node_modules' tests/Length of output: 395
Refine password validation and align test expectations
Update the signup validation so that password is only required when provider === "credentials", and adjust your tests to expect a 400 error (not 402) for missing fields.
• In src/controllers/member.controller.ts (around lines 49–51):
- if (!email || !name || !password || !passoutYear || !provider) {
+ if (
+ !email ||
+ !name ||
+ !passoutYear ||
+ !provider ||
+ (provider === "credentials" && !password)
+ ) {
throw new ApiError("Required fields absent", 400);
}• In tests/Member.test.ts (line 68):
- await expect(handler(req, res)).rejects.toThrow(new ApiError('Required fields absent', 402));
+ await expect(handler(req, res)).rejects.toThrow(new ApiError('Required fields absent', 400));No references to a "local" provider were found in the codebase.
🤖 Prompt for AI Agents
In src/controllers/member.controller.ts around lines 49 to 51, the signup
validation currently requires password unconditionally and throws ApiError(...,
400) should be used for missing fields; change the validation so password is
only required when provider === "credentials" (i.e., require email, name,
passoutYear, provider always; require password only if provider ===
"credentials"), and ensure the thrown ApiError uses status code 400 for missing
fields; also update tests/Member.test.ts (line 68) to expect new
ApiError('Required fields absent', 400) instead of 402.
| if(parsedBody.password) await memberService.updatePassword(memberId, parsedBody.password); | ||
| else await memberService.updateMember(memberId, parsedBody); |
There was a problem hiding this comment.
Password-only branch discards other field updates; update both.
If payload includes password plus other fields, the non-password fields are ignored. Update password and then update remaining fields.
Apply this diff:
- if(parsedBody.password) await memberService.updatePassword(memberId, parsedBody.password);
- else await memberService.updateMember(memberId, parsedBody);
+ const { password, ...rest } = parsedBody;
+ if (password) {
+ await memberService.updatePassword(memberId, password);
+ }
+ if (Object.keys(rest).length) {
+ await memberService.updateMember(memberId, rest);
+ }For stronger consistency, consider wrapping both updates in a single service method using a Prisma transaction to make the operation atomic.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if(parsedBody.password) await memberService.updatePassword(memberId, parsedBody.password); | |
| else await memberService.updateMember(memberId, parsedBody); | |
| // ... around line 92 in src/controllers/member.controller.ts | |
| - if(parsedBody.password) await memberService.updatePassword(memberId, parsedBody.password); | |
| - else await memberService.updateMember(memberId, parsedBody); | |
| + const { password, ...rest } = parsedBody; | |
| + if (password) { | |
| + await memberService.updatePassword(memberId, password); | |
| + } | |
| + if (Object.keys(rest).length) { | |
| + await memberService.updateMember(memberId, rest); | |
| + } | |
| // ... following code |
🤖 Prompt for AI Agents
In src/controllers/member.controller.ts around lines 92-93, the current branch
returns early when parsedBody.password exists and thus ignores other fields;
change the flow to always apply non-password updates plus the password: if
parsedBody.password is present, call memberService.updatePassword(memberId,
parsedBody.password) and then call memberService.updateMember(memberId,
parsedBodyWithoutPassword) (ensure you strip password from the second call),
otherwise just call updateMember as before. Alternatively (preferred), add a new
service method (e.g., updateMemberWithPassword) that accepts memberId and the
full payload and performs both updates inside a single Prisma transaction to
make the operation atomic and avoid partial updates.
| payload: UpdateMemberPayload | ||
| ) => { |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Confirm UpdateMemberPayload shape across modules.
Ensure UpdateMemberPayload excludes password now that password changes go through updatePassword, or if it includes password?, ensure callers strip it before passing to updateMember (as per controller change).
🏁 Script executed:
#!/bin/bash
# Inspect UpdateMemberPayload for a 'password' property
rg -n "interface\\s+UpdateMemberPayload" -A 20 src --glob '!node_modules'Length of output: 771
🏁 Script executed:
#!/bin/bash
# Show the updateMember method and how it handles the payload
rg -n "updateMember" -C 5 src/services/member.service.tsLength of output: 246
Remove password from UpdateMemberPayload and strip it out in updateMember
The UpdateMemberPayload type in src/types/members.d.ts still defines an optional password? field (line 18). Since password changes now go through the dedicated updatePassword flow, please:
- In
src/types/members.d.ts, remove thepassword?: stringline from theUpdateMemberPayloadinterface. - In
src/services/member.service.ts’supdateMemberfunction, explicitly destructurepasswordso it’s not passed through (e.g.const { name, password, ...rest } = payload;).
This guarantees that updateMember cannot be used to change passwords.
🤖 Prompt for AI Agents
In src/services/member.service.ts around lines 69-70 and src/types/members.d.ts
(line ~18), remove the optional password field from the UpdateMemberPayload type
in members.d.ts and update updateMember to strip password from the incoming
payload; specifically delete the password?: string line in the interface, and in
updateMember destructure the payload to extract password (e.g. const { password,
...rest } = payload) so the password is not forwarded or used when updating a
member.
| export const updatePassword = async(id: string, password: string) => { | ||
| const account = await prisma.account.findFirst({ | ||
| where: { memberId: id }, | ||
| }); | ||
|
|
||
| if (!account) throw new ApiError("Associated account not found", 404); | ||
|
|
||
| return await prisma.account.update({ | ||
| where: { id: account.id }, | ||
| data: { password }, | ||
| }); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Hash passwords and block password updates for non-credentials accounts.
updatePassword stores plaintext and allows updates for OAuth accounts. Both are security issues.
Apply this diff to enforce provider and hash the password:
export const updatePassword = async(id: string, password: string) => {
const account = await prisma.account.findFirst({
where: { memberId: id },
});
if (!account) throw new ApiError("Associated account not found", 404);
- return await prisma.account.update({
+ if (account.provider !== "credentials") {
+ throw new ApiError("Password updates not allowed for OAuth accounts", 400);
+ }
+
+ const hashed = await bcrypt.hash(password, 12);
+ return await prisma.account.update({
where: { id: account.id },
- data: { password },
+ data: { password: hashed },
});
}Add the import (outside this hunk):
import bcrypt from "bcryptjs";To ensure no other code paths write plaintext passwords, search for raw password assignments:
🏁 Script executed:
#!/bin/bash
rg -n "password\\s*:\\s*password\\b" --glob '!node_modules' .
rg -n "argon2|bcrypt" --glob '!node_modules' .Length of output: 109
🏁 Script executed:
#!/bin/bash
# Find all prisma.account.create/update calls to see where password fields may be set
rg -n 'prisma\.account\.(create|update)' --glob '!node_modules' -A3 .Length of output: 563
🏁 Script executed:
#!/bin/bash
# Show full prisma.account.create call to check for password assignments
rg -n 'prisma\.account\.create' --glob '!node_modules' -A10 src/services/member.service.tsLength of output: 346
Enforce password hashing at creation and restrict updates to credentials-only accounts
The current implementation stores plaintext passwords on signup and allows password changes for OAuth users. To address both issues:
• Add bcrypt import at the top of src/services/member.service.ts
• On account creation (around line 55), hash the password when provider === "credentials"
• In updatePassword (around line 85), block non-credentials providers and hash the new password
Diff for src/services/member.service.ts:
--- a/src/services/member.service.ts
+++ b/src/services/member.service.ts
@@
+ import bcrypt from "bcryptjs";
// ...signup or createMember function...
- await prisma.account.create({
- data: {
- provider,
- providerAccountId: email,
- password: provider === "credentials" ? password : null,
- memberId: newMember.id,
- },
- });
+ const hashedPassword = provider === "credentials"
+ ? await bcrypt.hash(password, 12)
+ : null;
+ await prisma.account.create({
+ data: {
+ provider,
+ providerAccountId: email,
+ password: hashedPassword,
+ memberId: newMember.id,
+ },
+ });
// ...later in the file...
export const updatePassword = async (id: string, password: string) => {
const account = await prisma.account.findFirst({
where: { memberId: id },
});
if (!account) throw new ApiError("Associated account not found", 404);
-
- return await prisma.account.update({
- where: { id: account.id },
- data: { password },
- });
+ if (account.provider !== "credentials") {
+ throw new ApiError("Password updates not allowed for OAuth accounts", 400);
+ }
+
+ const newHashed = await bcrypt.hash(password, 12);
+ return await prisma.account.update({
+ where: { id: account.id },
+ data: { password: newHashed },
+ });
};This ensures no plaintext passwords are stored and prevents OAuth accounts from updating passwords.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const updatePassword = async(id: string, password: string) => { | |
| const account = await prisma.account.findFirst({ | |
| where: { memberId: id }, | |
| }); | |
| if (!account) throw new ApiError("Associated account not found", 404); | |
| return await prisma.account.update({ | |
| where: { id: account.id }, | |
| data: { password }, | |
| }); | |
| } | |
| // At the top of src/services/member.service.ts | |
| import bcrypt from "bcryptjs"; | |
| // … other imports … | |
| // …signup or createMember function… | |
| export const createMember = async ( | |
| /* params */ | |
| ) => { | |
| // … prior logic … | |
| - await prisma.account.create({ | |
| - data: { | |
| - provider, | |
| - providerAccountId: email, | |
| - password: provider === "credentials" ? password : null, | |
| - memberId: newMember.id, | |
| - }, | |
| - }); | |
| + const hashedPassword = provider === "credentials" | |
| + ? await bcrypt.hash(password, 12) | |
| + : null; | |
| + await prisma.account.create({ | |
| + data: { | |
| + provider, | |
| + providerAccountId: email, | |
| + password: hashedPassword, | |
| + memberId: newMember.id, | |
| + }, | |
| + }); | |
| // … remainder of createMember … | |
| }; | |
| // …later in the file… | |
| export const updatePassword = async (id: string, password: string) => { | |
| const account = await prisma.account.findFirst({ | |
| where: { memberId: id }, | |
| }); | |
| if (!account) { | |
| throw new ApiError("Associated account not found", 404); | |
| } | |
| - return await prisma.account.update({ | |
| - where: { id: account.id }, | |
| - data: { password }, | |
| - }); | |
| + if (account.provider !== "credentials") { | |
| + throw new ApiError("Password updates not allowed for OAuth accounts", 400); | |
| + } | |
| + | |
| + const newHashed = await bcrypt.hash(password, 12); | |
| + return await prisma.account.update({ | |
| + where: { id: account.id }, | |
| + data: { password: newHashed }, | |
| + }); | |
| }; |
🤖 Prompt for AI Agents
In src/services/member.service.ts around lines 55 and 85-96, the code stores
plaintext passwords on signup and allows password updates for non-credentials
(OAuth) accounts; import bcrypt at the top, and when creating an account (around
line 55) only hash the password if provider === "credentials" (replace the
stored plaintext with the bcrypt hash), and in updatePassword (lines 85-96)
first fetch the account/provider, throw an ApiError if the provider is not
"credentials", then bcrypt.hash the new password before calling
prisma.account.update to persist the hashed password.
i-am-that-guy
left a comment
i-am-that-guy
left a comment
There was a problem hiding this comment.
please add a test for update password feature
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/Member.test.ts (1)
223-223: Consider using a more type-safe mock return value.Using
{} as anyfor theupdatePasswordmock return value bypasses TypeScript's type checking. Consider using a proper return type or at leastundefinedif the method doesn't return anything meaningful.- jest.spyOn(memberService, 'updatePassword').mockResolvedValue({} as any); + jest.spyOn(memberService, 'updatePassword').mockResolvedValue(undefined as any);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/Member.test.ts(8 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/Member.test.ts (2)
src/controllers/member.controller.ts (1)
updateAMember(73-99)src/utils/imageUtils.ts (1)
uploadImage(23-70)
🔇 Additional comments (4)
tests/Member.test.ts (4)
16-19: LGTM! Account mocks align with service layer changes.The addition of
account.findFirstandaccount.updatemocks is appropriate for supporting the new password update functionality that works with the account entity.
39-61: Test correctly validates the new provider field requirement.The test properly includes the
provider: 'credentials'field and validates thatcreateMemberis called with all required parameters including the new provider field.
77-122: Test correctly validates member update without password.The test properly verifies that when no password is provided, the
updateMemberservice method is called with the correct payload andgetDetailsis invoked to retrieve the updated member data.
190-236: Password update test properly validates the new flow.The test correctly verifies that:
- When a password is provided in memberData,
updatePasswordis called instead ofupdateMember- The password is correctly extracted and passed to the service
getDetailsis called to retrieve the updated member dataThis aligns with the controller implementation where password updates are routed to a dedicated service method.
| expect(memberService.updateMember).toHaveBeenCalledWith('abc-123', { | ||
| }); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify that an empty object is the intended payload for profile photo updates.
The test shows updateMember being called with an empty object {} when only a profile photo is uploaded. This seems correct since the photo URL is handled separately through the uploadImage utility, but please confirm this is the intended behavior.
🏁 Script executed:
#!/bin/bash
# Description: Verify the updateMember service implementation to confirm empty payload handling
# Check how updateMember handles empty payloads
ast-grep --pattern 'updateMember($_id, $_payload) {
$$$
}'
# Check the controller implementation for context
rg -A 10 "if\s*\(imageUrl\)" src/controllers/member.controller.tsLength of output: 494
🏁 Script executed:
#!/bin/bash
# Show context around the test expectation in tests/Member.test.ts
sed -n '160,200p' tests/Member.test.tsLength of output: 1152
🏁 Script executed:
#!/bin/bash
# Locate the updateMember implementation to see how it processes the payload
rg -n "updateMember" -A15 src/servicesLength of output: 1009
🏁 Script executed:
#!/bin/bash
# Display the full updateAMember handler implementation for context
rg -n "export const updateAMember" -A50 src/controllers/member.controller.tsLength of output: 1949
🏁 Script executed:
#!/bin/bash
# Locate the uploadImage utility implementation
rg -n "export function uploadImage" -A30 src/utilsLength of output: 52
Prevent calling updateMember with an empty payload
The updateMember service will throw an ApiError("No fields passed", 400) if its payload is {}. In the photo–only update case for an existing image, the handler uploads to Supabase but never assigns imageUrl (since it calls uploadImage(supabase, file, "members", oldImage) without capturing its return), so parsedBody remains empty and updateMember is invoked with {}—which will break in production.
To fix:
- In src/controllers/member.controller.ts (around lines 83–91), either:
- Assign
parsedBody.profilePhoto = oldImagewhen re-uploading over an existing photo, or - Skip calling
memberService.updateMemberifObject.keys(parsedBody).length === 0.
- Assign
- In tests/Member.test.ts (around lines 180–181), update the assertion to match the chosen behavior:
- If you opt to include the existing URL, expect
expect(memberService.updateMember).toHaveBeenCalledWith('abc-123', { profilePhoto: 'https://old.url/image.png' });
- If you opt to skip the DB update entirely, assert that
updateMemberis not called.
- If you opt to include the existing URL, expect
🤖 Prompt for AI Agents
In tests/Member.test.ts around lines 180-181 and in
src/controllers/member.controller.ts around lines 83-91, fix the photo-only
update flow so updateMember is not called with an empty payload: either (A) when
re-uploading over an existing image, assign parsedBody.profilePhoto = oldImage
after upload so parsedBody is non-empty and then update the test to expect
updateMember called with that profilePhoto URL, or (B) skip calling
memberService.updateMember when Object.keys(parsedBody).length === 0 and update
the test to assert updateMember was not called; implement one of these two
behaviors consistently in both controller and test.
i-am-that-guy
left a comment
i-am-that-guy
left a comment
There was a problem hiding this comment.
"tested code ahe"
-shruti jadhav
Summary by CodeRabbit
Bug Fixes
New Features
Tests
Documentation