Feat/#11#20
Merged
Merged
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.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds authenticated ChangesMerchant Profile API
Auth Token Test Update
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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.
Contributor
|
@EmmanuelAR Please fix the CI. |
…nt variable for secret - Imported environment configuration to access the JWT secret. - Modified the JWT verification in tests to utilize the environment variable instead of a hardcoded value.
codebestia
approved these changes
Jun 29, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #11
Evidence: https://www.loom.com/share/92e6a0e28cf34c4fa02c902f0f2a5a66
Postman collection:
Shade - Merchant Profile API (#11).postman_collection.json
Summary by CodeRabbit
New Features
GET /meandPATCH /me.webhookandaccount).Bug Fixes
logo/webhookset tonull).webhookenforcement.Tests