Feat/#11#20
Conversation
- Added `getMyProfileController` and `updateMyProfileController` to handle fetching and updating the authenticated merchant's profile. - Introduced validation for updating merchant profiles with `validateUpdateMerchant`. - Updated routes to include new endpoints for profile management. - Created integration and unit tests for the new profile functionalities, ensuring proper authentication and validation handling.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds authenticated ChangesMerchant Profile API
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ 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
tests/integration/merchant.profile.test.tsOops! Something went wrong! :( ESLint: 9.24.0 ReferenceError: require is not defined in ES module scope, you can use import instead tests/unit/merchant.profile.services.test.tsOops! Something went wrong! :( ESLint: 9.24.0 ReferenceError: require is not defined in ES module scope, you can use import instead tests/unit/merchant.update.validation.test.tsOops! Something went wrong! :( ESLint: 9.24.0 ReferenceError: require is not defined in ES module scope, you can use import instead Comment |
- Deleted the implementation plan and design spec for the Merchant Profile API as they are no longer relevant to the current project structure.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (5)
tests/integration/merchant.profile.test.ts (2)
56-59: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAssert the sensitive fields named in the API contract.
These checks cover relation leakage, but the linked issue specifically forbids exposing
emailOtpand API-key hash fields. Adding assertions for those fields will keep the sanitizer contract from regressing silently.🤖 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/integration/merchant.profile.test.ts` around lines 56 - 59, The integration test for the merchant profile response currently checks for some hidden fields but misses the contract-specific sensitive fields. Update the assertions in merchant.profile.test.ts around the response checks to also verify that response.body does not expose emailOtp or API-key hash fields, alongside the existing refreshTokens/apiKeys checks, so the sanitizer contract is covered by the test.
85-99: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winCover all explicitly non-editable fields in the ignore-path test.
This only exercises
addressandmerchantIdandaccountare also part of the immutable contract. Including them here would better protect against accidental writes to identity/account fields.Suggested test expansion
const response = await request(app) .patch(ME_URL) .set('Authorization', 'Bearer valid-token') - .send({ firstName: 'Grace', address: '0xHACK', email: 'evil@example.com' }); + .send({ + firstName: 'Grace', + address: '0xHACK', + email: 'evil@example.com', + merchantId: 999, + account: '0xHACKED', + }); expect(response.status).toBe(200); const updateArg = prismaMock.merchant.update.mock.calls[0][0]; expect(updateArg.data).toEqual({ firstName: 'Grace' }); expect(response.body.address).toBe('0x123'); expect(response.body.email).toBe('ada@example.com'); + expect(response.body.merchantId).toBe(1); + expect(response.body.account).toBe('CCONTRACT');🤖 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/integration/merchant.profile.test.ts` around lines 85 - 99, Expand the ignore-path test in merchant.profile.test.ts so it also covers the other immutable fields enforced by the merchant profile update flow. In the existing silently ignores non-editable fields test around the PATCH to ME_URL, include merchantId and account in the sent payload alongside address and email, and keep asserting that prismaMock.merchant.update only receives editable data (via the updateArg.data check) while the response still preserves the original merchantId/account values. Use the existing merchant.profile test setup and prismaMock.merchant.update mock to verify these fields are stripped before persistence.tests/unit/merchant.update.validation.test.ts (1)
23-29: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd
logo: ''coverage for clear semantics.The contract gives
logothe same clear-to-null behavior aswebhook, but this suite only locks in the webhook branch. A regression invalidateUpdateMerchant({ logo: '' })would currently slip through.Suggested test
test('accepts logo and webhook cleared with null', () => { expect(validateUpdateMerchant({ logo: null, webhook: null })).toEqual({}); }); + test('accepts an empty string logo as a clear', () => { + expect(validateUpdateMerchant({ logo: '' })).toEqual({}); + }); + test('accepts an empty string webhook as a clear', () => { expect(validateUpdateMerchant({ webhook: '' })).toEqual({}); });🤖 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/unit/merchant.update.validation.test.ts` around lines 23 - 29, Add coverage for the clear-to-null behavior of validateUpdateMerchant by extending the existing validation tests in merchant.update.validation.test.ts to assert that an empty string for logo is treated the same as webhook. Update or add a test near the current validateUpdateMerchant cases so that validateUpdateMerchant({ logo: '' }) returns an empty object, matching the clear semantics already covered for webhook.tests/unit/merchant.profile.services.test.ts (1)
7-26: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSanitization assertions are vacuous because
baseMerchantomits the sensitive fields.
baseMerchanthas norefreshTokens/apiKeys, soexpect(result).not.toHaveProperty('refreshTokens'/'apiKeys')(Lines 38-39, 83) passes trivially and does not actually verify the allowlist drops internal fields. Add the sensitive fields to the fixture so the assertions exercise the sanitizer.🧪 Strengthen the fixture
createdAt: new Date(), updatedAt: new Date(), + // internal fields that must be stripped by sanitizeMerchant + refreshTokens: [{ token: 'secret' }], + apiKeys: [{ hash: 'hashed' }], + emailOtp: '123456', };🤖 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/unit/merchant.profile.services.test.ts` around lines 7 - 26, The sanitizer tests are currently vacuous because the shared `baseMerchant` fixture in `merchant.profile.services.test.ts` does not include sensitive fields, so the `sanitizeMerchantProfile` assertions never prove they are removed. Update `baseMerchant` to include internal properties like `refreshTokens` and `apiKeys`, then keep the existing `expect(result).not.toHaveProperty(...)` checks so they verify the allowlist behavior of `sanitizeMerchantProfile` and the related service output.src/controllers/merchant.controllers.ts (1)
66-110: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBoth controllers correctly mirror the established auth/validation/error pattern. 401 guard,
validateUpdateMerchant400 path, andAppError→status / generic→500 mapping are consistent withregisterMerchantController.Optional: the try/catch +
AppError/500 block is now duplicated across three controllers; extracting anasyncHandler/error-mapping wrapper would remove the repetition. Deferable.🤖 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 `@src/controllers/merchant.controllers.ts` around lines 66 - 110, The controllers already follow the expected auth, validation, and AppError/500 mapping pattern, so no functional change is needed in getMyProfileController or updateMyProfileController. If you want to address the duplication noted in the review, extract the repeated try/catch error mapping into a reusable async handler or shared helper and have getMyProfileController, updateMyProfileController, and registerMerchantController use it consistently.
🤖 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 `@src/controllers/merchant.controllers.ts`:
- Around line 66-110: The controllers already follow the expected auth,
validation, and AppError/500 mapping pattern, so no functional change is needed
in getMyProfileController or updateMyProfileController. If you want to address
the duplication noted in the review, extract the repeated try/catch error
mapping into a reusable async handler or shared helper and have
getMyProfileController, updateMyProfileController, and
registerMerchantController use it consistently.
In `@tests/integration/merchant.profile.test.ts`:
- Around line 56-59: The integration test for the merchant profile response
currently checks for some hidden fields but misses the contract-specific
sensitive fields. Update the assertions in merchant.profile.test.ts around the
response checks to also verify that response.body does not expose emailOtp or
API-key hash fields, alongside the existing refreshTokens/apiKeys checks, so the
sanitizer contract is covered by the test.
- Around line 85-99: Expand the ignore-path test in merchant.profile.test.ts so
it also covers the other immutable fields enforced by the merchant profile
update flow. In the existing silently ignores non-editable fields test around
the PATCH to ME_URL, include merchantId and account in the sent payload
alongside address and email, and keep asserting that prismaMock.merchant.update
only receives editable data (via the updateArg.data check) while the response
still preserves the original merchantId/account values. Use the existing
merchant.profile test setup and prismaMock.merchant.update mock to verify these
fields are stripped before persistence.
In `@tests/unit/merchant.profile.services.test.ts`:
- Around line 7-26: The sanitizer tests are currently vacuous because the shared
`baseMerchant` fixture in `merchant.profile.services.test.ts` does not include
sensitive fields, so the `sanitizeMerchantProfile` assertions never prove they
are removed. Update `baseMerchant` to include internal properties like
`refreshTokens` and `apiKeys`, then keep the existing
`expect(result).not.toHaveProperty(...)` checks so they verify the allowlist
behavior of `sanitizeMerchantProfile` and the related service output.
In `@tests/unit/merchant.update.validation.test.ts`:
- Around line 23-29: Add coverage for the clear-to-null behavior of
validateUpdateMerchant by extending the existing validation tests in
merchant.update.validation.test.ts to assert that an empty string for logo is
treated the same as webhook. Update or add a test near the current
validateUpdateMerchant cases so that validateUpdateMerchant({ logo: '' })
returns an empty object, matching the clear semantics already covered for
webhook.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 986e1cb3-78bf-447b-b05c-a19f26e03f37
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (9)
docs/superpowers/plans/2026-06-26-merchant-profile-api.mddocs/superpowers/specs/2026-06-26-merchant-profile-api-design.mdsrc/controllers/merchant.controllers.tssrc/routes/merchant.routes.tssrc/services/merchant.services.tssrc/utils/validation.tstests/integration/merchant.profile.test.tstests/unit/merchant.profile.services.test.tstests/unit/merchant.update.validation.test.ts
…ations - Updated integration and unit tests for merchant profile to include internal relations like refreshTokens and apiKeys. - Modified the test for non-editable fields to account for additional fields: merchantId and account. - Added validation for accepting empty string values for logo and webhook in update validation tests.
|
@EmmanuelAR Please fix the CI. |
2 participants
Closes #11
Evidence: https://www.loom.com/share/92e6a0e28cf34c4fa02c902f0f2a5a66
Postman collection:
Shade - Merchant Profile API (#11).postman_collection.json
Summary by CodeRabbit
New Features
Bug Fixes
Tests