Create Seller Role on Registration (with Store Profile)#141
Create Seller Role on Registration (with Store Profile)#141Villarley merged 5 commits intoStarShopCr:Developfrom
Conversation
WalkthroughAdds role-aware user profile extensions (location, country, buyerData, sellerData), enforces cross-field validation, and updates responses to omit user id. Introduces Stores module with entity, DTOs, service, controller, and migrations, including default store creation on seller registration. Disables TypeORM synchronize globally. Updates types, middlewares, tests, and documentation. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant AuthController
participant AuthService
participant UserRepo as Users Repository
participant RoleService
participant StoreService
participant StoreRepo as Stores Repository
Client->>AuthController: POST /auth/register (walletAddress, role, ... buyer/seller data)
AuthController->>AuthService: registerWithWallet(data)
AuthService->>UserRepo: Find existing by wallet
alt User exists
AuthService->>UserRepo: Update fields (location/country/buyerData/sellerData)
else Create new user
AuthService->>UserRepo: Save new user with fields
AuthService->>RoleService: Assign role to user
end
opt role == seller
AuthService->>StoreService: createDefaultStore(user.id, sellerData)
StoreService->>UserRepo: Validate seller exists/role
StoreService->>StoreRepo: Create Store (PENDING_APPROVAL)
StoreService-->>AuthService: Store or error (logged)
end
AuthService-->>AuthController: User (no id), token meta
AuthController-->>Client: 200 { success, data.user, expiresIn }
sequenceDiagram
autonumber
actor Seller
participant StoreController
participant StoreService
participant StoreRepo as Stores Repository
Seller->>StoreController: POST /stores (JWT, CreateStoreDto)
StoreController->>StoreService: createStore(sellerId, dto)
StoreService->>StoreRepo: Save store (PENDING_APPROVAL)
StoreService-->>StoreController: Store
StoreController-->>Seller: 201 { success, data }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related issues
Possibly related PRs
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ 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 (
|
- Removed user ID from the AuthController and related DTOs to streamline response data. - Updated example values in DTOs for consistency and clarity.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 29
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
src/modules/shared/middleware/auth.middleware.ts (1)
80-90: Don't instantiate middleware with null deps; throw ForbiddenException (403) for RBAC failure
- Creating a new AuthMiddleware(null, null) to reuse mapRoleToEnum is a DI smell and can break later if the constructor is extended.
- Throwing ReferenceError yields a 500; use ForbiddenException for authorization failures.
Apply this diff to make requireRole pure and return proper HTTP semantics:
export const requireRole = ( roleName: 'buyer' | 'seller' | 'admin' ): ((req: AuthenticatedRequest, res: Response, next: NextFunction) => void) => { - return (req: AuthenticatedRequest, res: Response, next: NextFunction) => { - const requiredRole = new AuthMiddleware(null, null).mapRoleToEnum(roleName); - if (!req.user || !req.user.role.some(role => role === requiredRole)) { - throw new ReferenceError('Insufficient permissions'); - } - next(); - }; + const ROLE_NAME_TO_ENUM: Record<'buyer' | 'seller' | 'admin', Role> = { + admin: Role.ADMIN, + seller: Role.SELLER, + buyer: Role.BUYER, + }; + return (req: AuthenticatedRequest, res: Response, next: NextFunction) => { + const requiredRole = ROLE_NAME_TO_ENUM[roleName]; + if (!req.user || !req.user.role?.includes(requiredRole)) { + throw new ForbiddenException('Insufficient permissions'); + } + next(); + }; };And update imports:
// Add ForbiddenException to the import list import { Injectable, NestMiddleware, UnauthorizedException, ForbiddenException } from '@nestjs/common';src/types/express.d.ts (1)
20-21: Inconsistent id typing between Request.user and Express.UserRequest.user.id is string | number while Express.User.id is string. This inconsistency propagates to controllers and causes brittle comparisons.
Apply this diff to align on string | number:
interface User { - id: string; + id: string | number;Also applies to: 40-41
src/modules/users/controllers/user.controller.ts (2)
121-127: Authorization bug: enum-vs-string role check and id type mismatch can block valid updates
- req.user.role is an array of Role enums (from middleware), but you're comparing to 'admin' (string). This will never match.
- userId is a number (ParseIntPipe), while req.user.id may be a string — strict inequality can fail even for the same user.
Apply this diff to normalize id and perform a robust admin check:
- const currentUserId = req.user?.id; - const currentUserRole = req.user?.role?.[0]; + const currentUserId = + typeof req.user?.id === 'string' + ? parseInt(req.user.id as string, 10) + : (req.user?.id as number | undefined); + const roles = (req.user?.role ?? []) as Array<Role | string>; + const isAdmin = roles.includes(Role.ADMIN) || roles.includes('admin'); // Check if user is updating their own profile or is admin - if (userId !== currentUserId && currentUserRole !== 'admin') { + if (userId !== currentUserId && !isAdmin) { throw new UnauthorizedError('You can only update your own profile'); }Optional: Replace UnauthorizedError with a 403 (Forbidden) to better reflect RBAC failure.
159-165: Same authorization bug here: enum-vs-string role check and id type mismatchMirror the fix from updateUser for getUserById.
Apply this diff:
- const currentUserId = req.user?.id; - const currentUserRole = req.user?.role?.[0]; + const currentUserId = + typeof req.user?.id === 'string' + ? parseInt(req.user.id as string, 10) + : (req.user?.id as number | undefined); + const roles = (req.user?.role ?? []) as Array<Role | string>; + const isAdmin = roles.includes(Role.ADMIN) || roles.includes('admin'); // Check if user is accessing their own profile or is admin - if (userId !== currentUserId && currentUserRole !== 'admin') { + if (userId !== currentUserId && !isAdmin) { throw new UnauthorizedError('Access denied'); }src/modules/users/services/user.service.ts (2)
54-66: Avoid creating users without roles; make save + role assignment atomicIf the role is missing, the code silently skips assignment, leaving a user without roles. Wrap the save and role assignment in a single transaction and fail if the role doesn’t exist.
Apply this diff to make the operation atomic and fail if role is not found:
- const saved = await this.userRepository.save(user); - - // assign role to user_roles table - const roleRepo = AppDataSource.getRepository(Role); - const userRoleRepo = AppDataSource.getRepository(UserRole); - const role = await roleRepo.findOne({ where: { name: data.role } }); - if (role) { - const userRole = userRoleRepo.create({ - userId: saved.id, - roleId: role.id, - user: saved, - role, - }); - await userRoleRepo.save(userRole); - } - return saved; + return await AppDataSource.transaction(async (trx) => { + const saved = await trx.getRepository(User).save(user); + const role = await trx.getRepository(Role).findOne({ where: { name: data.role } }); + if (!role) { + throw new BadRequestError('Role not found'); + } + const userRole = trx.getRepository(UserRole).create({ + userId: saved.id, + roleId: role.id, + user: saved, + role, + }); + await trx.getRepository(UserRole).save(userRole); + return saved; + });
95-105: Don’t use truthy checks for updates; use explicit undefined checksUsing if (data.email) / if (data.name) blocks clearing values (e.g., empty string) and treats falsy values as “do not update”.
Apply this diff:
- if (data.email) { + if (data.email !== undefined) { const existingUser = await this.userRepository.findOne({ where: { email: data.email } }); if (existingUser && existingUser.id !== user.id) { throw new BadRequestError('Email already in use'); } user.email = data.email; } - if (data.name) { + if (data.name !== undefined) { user.name = data.name; }src/modules/auth/services/auth.service.ts (1)
133-144: Ensure role exists; consider transaction to avoid user without rolesIf the role isn’t found, you currently proceed without assigning, leaving the user without roles. At minimum, throw; ideally, wrap user save + role assignment in a transaction.
Apply this minimal diff:
const userRole = await this.roleRepository.findOne({ where: { name: data.role } }); - if (userRole) { + if (userRole) { const userRoleEntity = this.userRoleRepository.create({ userId: savedUser.id, roleId: userRole.id, user: savedUser, role: userRole, }); await this.userRoleRepository.save(userRoleEntity); - } + } else { + throw new BadRequestError('Role not found'); + }If you prefer, I can provide a transaction-based refactor similar to the UserService suggestion.
🧹 Nitpick comments (45)
src/modules/files/tests/test-utils.ts (1)
7-10: Mock additions look consistent; verify null handling in validatorsAdding location, country, buyerData, sellerData aligns with the new entity. If DTO validation is involved in these tests, note that sellerData: null will only be skipped if @IsOptional runs before @isObject on that property. See my note in UserDTO to place @IsOptional first.
Optional verification: if any test exercises DTO validation, prefer undefined over null here:
- sellerData: null, + sellerData: undefined,src/dtos/UserDTO.ts (3)
85-93: Place @IsOptional before other validators to ensure null/undefined are skippedGuard optionality first to avoid @isObject failing when value is null. Also consider narrowing type from any to a safe object.
@ApiPropertyOptional({ description: 'Buyer-specific data (only allowed if role is buyer)', example: { preferences: ['electronics', 'books'] }, }) - @IsRoleSpecificData({ message: 'buyerData is only allowed for buyers' }) - @IsObject({ message: 'Buyer data must be an object' }) - @IsOptional() - buyerData?: any; + @IsOptional() + @IsRoleSpecificData({ message: 'buyerData is only allowed for buyers' }) + @IsObject({ message: 'Buyer data must be an object' }) + buyerData?: Record<string, unknown>;
94-101: Mirror the optional-first order for sellerDataSame rationale as buyerData. This keeps behavior consistent and avoids null pitfalls.
@ApiPropertyOptional({ description: 'Seller-specific data (only allowed if role is seller)', example: { businessName: 'Tech Store', categories: ['electronics'] }, }) - @IsRoleSpecificData({ message: 'sellerData is only allowed for sellers' }) - @IsObject({ message: 'Seller data must be an object' }) - @IsOptional() - sellerData?: any; + @IsOptional() + @IsRoleSpecificData({ message: 'sellerData is only allowed for sellers' }) + @IsObject({ message: 'Seller data must be an object' }) + sellerData?: Record<string, unknown>;
16-57: Custom decorator works; minor cleanup on typingThe decorator is fine. Prefer using the lower-case TypeScript primitive for the “object” parameter type.
-function IsRoleSpecificData(validationOptions?: ValidationOptions) { - return function (object: Object, propertyName: string) { +function IsRoleSpecificData(validationOptions?: ValidationOptions) { + return function (object: object, propertyName: string) {src/modules/auth/middleware/authorize-roles.middleware.ts (1)
12-14: Avoid unsafe cast; normalize roles to array before checkingCasting req.user.role as Role[] will mask runtime issues if it’s actually a single role. Normalize then some(...) to keep it robust.
Apply this diff:
- const userRoles = req.user.role as Role[]; - const hasAllowedRole = userRoles.some((role) => allowedRoles.includes(role)); + const userRoles: Role[] = Array.isArray(req.user.role) + ? (req.user.role as Role[]) + : [req.user.role as Role]; + const hasAllowedRole = userRoles.some((role) => allowedRoles.includes(role));Optional follow-up: consider throwing ForbiddenException for insufficient permissions and mapping 401 for unauthenticated vs 403 for forbidden to align status codes with semantics.
src/modules/shared/middleware/session.middleware.ts (1)
38-43: Avoidas any; include the new user fields on req.user and cast to Express.User.This cast likely compensates for the newly-augmented user shape (location, country, buyerData, sellerData). Prefer populating those fields and using a precise type to preserve type-safety.
Apply:
- req.user = { - id: user.id, - walletAddress: user.walletAddress, - role: user.userRoles.map((ur) => ur.role.name as Role), - } as any; + req.user = { + id: user.id, + walletAddress: user.walletAddress, + role: user.userRoles.map((ur) => ur.role.name as Role), + location: user.location ?? null, + country: user.country ?? null, + buyerData: user.buyerData ?? null, + sellerData: user.sellerData ?? null, + } as Express.User;If Express.User isn’t augmented accordingly, either:
- adjust the global augmentation in src/types/express.d.ts to include these fields (prefer optional), or
- introduce a dedicated type (e.g., AuthenticatedUser) and use that instead of
any.src/modules/files/tests/file.controller.spec.ts (1)
93-97: Extended User fields in test mocks align with the updated model.Nice addition of location, country, buyerData, sellerData to keep mocks in sync.
Two small follow-ups:
- If Role.USER was replaced (as elsewhere in this PR), update the request mock to use BUYER for consistency (current: Line 36 uses Role.USER).
- Consider typing
mockUseras Partial to reduce the need for double casting.For the role enum consistency, update the request mock:
// Line 36 (in this spec) role: [Role.BUYER],Optionally, prefer Partial for mocks:
const mockUser: Partial<User> = { id: 1, walletAddress: '0x123', name: 'Test User', email: 'test@example.com', password: 'hashed_password', location: 'Test City', country: 'Test Country', buyerData: {}, sellerData: null, orders: [], userRoles: [], notifications: [], }; // Use as needed where a full User is required in return valuessrc/modules/files/tests/file.service.spec.ts (2)
47-67: Reduce duplication by centralizing mockUser creation.The mockUser object is repeated across tests with identical fields. Consider introducing a small factory helper to keep tests DRY and easier to evolve when the user shape changes again.
Example helper you can place near the top of this file:
function buildMockUser(overrides: Partial<any> = {}) { return { id: 1, walletAddress: '0x123', name: 'Test User', email: 'test@example.com', password: 'hashed_password', location: 'Test City', country: 'Test Country', buyerData: {}, sellerData: null, orders: [], userRoles: [], notifications: [], wishlist: [], stores: [], createdAt: new Date(), updatedAt: new Date(), ...overrides, }; }Then use buildMockUser() in each test, overriding only what differs.
298-341: Add an explicit unauthorized-ownership test case for deleteFile.You cover “not found” and “owned” scenarios. Add a test where a file exists but uploadedById does not match the caller, asserting:
- no provider delete is called (neither cloudinary nor s3)
- service returns false
I can draft the additional test if you want.
Also applies to: 343-386
src/modules/auth/middleware/jwt-auth.middleware.ts (1)
45-52: Avoidas any; include the new user fields and rely on augmented Express typings.You’ve likely added the new fields to Express.User elsewhere. Populate them here and drop the
as anycast.req.user = { id: user.id, email: user.email, walletAddress: user.walletAddress, name: user.name, - role: user.userRoles?.map((ur) => ur.role.name as Role) || [decoded.role as Role], - } as any; + role: user.userRoles?.map((ur) => ur.role.name as Role) || [decoded.role as Role], + location: user.location ?? undefined, + country: user.country ?? undefined, + buyerData: user.buyerData ?? undefined, + sellerData: user.sellerData ?? undefined, + };Ensure your Express type augmentation file (e.g., src/types/express.d.ts) is included via typeRoots or a global import so this compiles without
any.src/modules/wishlist/common/types/auth-request.type.ts (1)
4-17: De-duplicate AuthRequest by reusing the shared type.There’s a Shared AuthenticatedRequest type elsewhere in the codebase. To avoid drift, re-export or alias it here instead of maintaining a separate duplicate.
Apply:
-import { Request } from 'express'; -import { Role } from '../../../../types/role'; - -export interface AuthRequest extends Request { - user: { - id: string | number; - walletAddress: string; - name?: string; - email?: string; - role: Role[]; - location?: string; - country?: string; - buyerData?: any; - sellerData?: any; - createdAt?: Date; - updatedAt?: Date; - }; -} +export { AuthenticatedRequest as AuthRequest } from '../../../shared/types/auth-request.type';src/modules/shared/middleware/auth.middleware.ts (2)
40-43: Defaulting unknown roles to BUYER — confirm intended least-privilege fallbackMapping both 'user' and any unknown role names to BUYER changes prior semantics. This is fine from a least-privilege standpoint, but please confirm there are no other role strings (e.g., 'moderator', 'support') that should be recognized explicitly. Consider telemetry/logging for unknown role names.
11-23: Duplicate AuthenticatedRequest type — consolidate to a single source of truthThis interface duplicates definitions in src/modules/shared/types/auth-request.type.ts and src/types/express.d.ts. Keep just the module augmentation in src/types/express.d.ts (preferred), and import/use that everywhere to avoid divergence (e.g., id type, role type).
src/modules/shared/types/auth-request.type.ts (1)
4-18: Type duplication and potential drift across interfacesThis AuthenticatedRequest duplicates the Request augmentation in src/types/express.d.ts and the local interface in auth.middleware.ts. There’s already divergence on id types across files. Recommend keeping only the express.d.ts augmentation and referencing it project-wide.
src/types/express.d.ts (1)
55-57: Avoid re-declaring AuthenticatedRequest here if you already augment RequestYou can rely on Express.Request augmentation alone (user?: …) and remove this extra interface, or ensure all code imports this one consistently. Multiple parallel types are easy to desync.
src/modules/users/entities/user.entity.ts (2)
29-33: Enforce documented length constraints for location/countryDocs state max 100 chars. Add
length: 100to the columns to enforce at DB level.- @Column({ nullable: true }) + @Column({ nullable: true, length: 100 }) location?: string; - @Column({ nullable: true }) + @Column({ nullable: true, length: 100 }) country?: string;
53-55: Relation looks correct; consider optionality and cascade semanticsMapping to
Store.selleris correct. Minor: mark as optional (stores?: Store[]) and evaluate if you needonDelete/cascadebehaviors on theManyToOneside for cleanup semantics.docs/user-registration.md (1)
7-7: Align endpoint path with API base (/api/v1)Elsewhere (README, examples) use
/api/v1/users. Update for consistency.-`POST /users` +`POST /api/v1/users`src/modules/auth/tests/auth.service.spec.ts (2)
176-194: Add a seller registration test that asserts default store creationGiven sellers should trigger default store creation (non-fatal on failure), add coverage to assert
createDefaultStoreis called and errors are swallowed.Example test to add:
it('should create a default store for seller registration (non-fatal on failure)', async () => { const mockUserRepository = { findOne: jest.fn().mockResolvedValue(null), create: jest.fn().mockReturnValue({ id: 2, walletAddress: mockWalletAddress, userRoles: [{ role: { name: 'seller' } }] }), save: jest.fn().mockResolvedValue({ id: 2, walletAddress: mockWalletAddress, userRoles: [{ role: { name: 'seller' } }] }), }; (authService as any).userRepository = mockUserRepository; const mockStoreService = (authService as any).storeService as { createDefaultStore: jest.Mock }; mockStoreService.createDefaultStore.mockResolvedValue(undefined); const result = await authService.registerWithWallet({ walletAddress: mockWalletAddress, role: UserRoleEnum.SELLER, name: 'Seller', email: 'seller@example.com', sellerData: { businessName: 'My Store' }, }); expect(result.user?.id).toBe(2); expect(mockStoreService.createDefaultStore).toHaveBeenCalledWith(2, { businessName: 'My Store' }); // simulate non-fatal failure mockStoreService.createDefaultStore.mockRejectedValueOnce(new Error('boom')); await expect( authService.registerWithWallet({ walletAddress: 'GD6LXK4RB6D522ECACFVUEOKPCYBGQ6SKYONMVNIUOWUAIRNLSYAOB4R', role: UserRoleEnum.SELLER, sellerData: {}, }) ).resolves.toBeDefined(); });
116-146: Optional: Assert token cookie behavior or token issuanceIf the controller sets an HttpOnly cookie, consider asserting
jwtService.signinputs and thatexpiresInmatches config (even if the API body omits token). This tightens contract coverage.docs/README.md (3)
29-42: Add language to fenced code block (markdownlint MD040)Specify
textfor the ASCII architecture diagram.-``` +```text ┌─────────────────┐ ┌─────────────────┐ ┌─────────────────┐ │ Controller │ │ Service │ │ Repository │ │ (Validation) │───▶│ (Business │───▶│ (Database) │ │ │ │ Logic) │ │ │ └─────────────────┘ └─────────────────┘ └─────────────────┘ │ │ │ ▼ ▼ ▼ ┌─────────────────┐ ┌─────────────────┐ ┌─────────────────┐ │ DTOs │ │ Entities │ │ Migrations │ │ (Input/ │ │ (Data Model) │ │ (Schema │ │ Output) │ │ │ │ Changes) │ └─────────────────┘ └─────────────────┘ └─────────────────┘ -``` +```
166-181: Add language to code structure fenced block (markdownlint MD040)Use
textfor the tree.-``` +```text src/ ├── modules/ │ ├── users/ # User management │ │ ├── entities/ # Database entities │ │ ├── controllers/ # API endpoints │ │ ├── services/ # Business logic │ │ └── tests/ # Test files │ └── auth/ # Authentication │ ├── dto/ # Data transfer objects │ ├── services/ # Auth services │ └── tests/ # Auth tests ├── types/ # TypeScript types ├── migrations/ # Database migrations └── docs/ # Documentation -``` +```
362-362: Avoid using emphasis as a heading (markdownlint MD036)Convert the emphasized line to a proper heading.
-**Built with ❤️ for the StarShop community** +### Built with ❤️ for the StarShop communitydocs/IMPLEMENTATION_GUIDE.md (2)
22-35: Add language to fenced code block (markdownlint MD040)Use
textfor the ASCII architecture diagram.-``` +```text ┌─────────────────┐ ┌─────────────────┐ ┌─────────────────┐ │ Controller │ │ Service │ │ Repository │ │ (Validation) │───▶│ (Business │───▶│ (Database) │ │ │ │ Logic) │ │ │ └─────────────────┘ └─────────────────┘ └─────────────────┘ │ │ │ ▼ ▼ ▼ ┌─────────────────┐ ┌─────────────────┐ ┌─────────────────┐ │ DTOs │ │ Entities │ │ Migrations │ │ (Input/ │ │ (Data Model) │ │ (Schema │ │ Output) │ │ │ │ Changes) │ └─────────────────┘ └─────────────────┘ └─────────────────┘ -``` +```
297-305: Migration snippet: prefer consistent casing and JSONBIf your DB uses camelCase columns via naming strategy, the snippet is fine. Otherwise, prefer snake_case and ensure
jsonbmatches actual migration.-ADD COLUMN "buyerData" jsonb, -ADD COLUMN "sellerData" jsonb; +ADD COLUMN "buyer_data" jsonb, +ADD COLUMN "seller_data" jsonb;If the codebase uses camelCase columns, consider adding a note clarifying the naming strategy.
src/modules/users/services/user.service.ts (1)
27-41: Role-specific data validation is good; clarify admin policy and align with AuthService/DTOs
- Current logic requires buyerData/sellerData for buyer/seller. AuthService’s registerWithWallet does not enforce “required”. DTOs mark them optional.
- Define whether admin is allowed to carry buyer/seller data; if not, add a check.
Would you like me to update the DTOs and AuthService to enforce the same “required/forbidden” rules, and add an admin restriction?
docs/TECHNICAL_SPECIFICATION.md (2)
201-215: UserResponse shape likely mismatches runtime model for rolesThe codebase models roles via userRoles relation; a single role: string field may be inconsistent. Consider primaryRole plus roles array, or omit role here.
Apply this diff to reflect a safer, extensible response:
-interface UserResponse { +interface UserResponse { id: number; walletAddress: string; name: string; email: string; - role: string; + primaryRole?: string; + roles?: string[]; location?: string; country?: string; buyerData?: any; sellerData?: any; createdAt?: Date; updatedAt?: Date; }
314-328: Align error class examples with actual implementation (extends AppError)In code, BadRequestError extends AppError (with HTTP status). The doc’s example extends Error and may confuse consumers.
Apply this diff:
-export class BadRequestError extends Error { - constructor(message: string) { - super(message); - this.name = 'BadRequestError'; - } -} +export class BadRequestError extends AppError { + constructor(message: string) { + super(400, message); + } +} -export class UnauthorizedError extends Error { - constructor(message: string) { - super(message); - this.name = 'UnauthorizedError'; - } -} +export class UnauthorizedError extends AppError { + constructor(message: string) { + super(401, message); + } +}src/migrations/1751199238000-CreateStoresTable.ts (1)
11-16: Minor: prefer int + generated increment over serial for portabilityUsing type: 'int' with isGenerated: true and generationStrategy: 'increment' avoids serial-specific assumptions and aligns with TypeORM conventions.
Would you like a patch to switch to int + generationStrategy?
src/modules/auth/services/auth.service.ts (2)
145-153: Log with a structured logger instead of console.errorUse your logger abstraction to capture context (userId, role) and error data. Avoid console in production paths.
I can wire this to your existing logger if you share its location.
211-218: updateUser duplicates logic; reuse UserService and enforce role/email rulesThis path bypasses email uniqueness checks and role-specific update constraints present in UserService.
Consider delegating to UserService:
- async updateUser(userId: number, updateData: { + async updateUser(userId: number, updateData: { name?: string; email?: string; location?: string; country?: string; buyerData?: any; sellerData?: any; }): Promise<User> { - const user = await this.userRepository.findOne({ where: { id: userId } }); - if (!user) { - throw new BadRequestError('User not found'); - } - // Update user data - Object.assign(user, updateData); - await this.userRepository.save(user); - return this.getUserById(String(userId)); + return this.userService.updateUser(String(userId), updateData); }Also applies to: 225-230
src/modules/auth/dto/auth.dto.ts (1)
5-46: Avoid duplicating custom validators across modules; centralize IsRoleSpecificDataThis validator appears in multiple places per PR summary. Centralize it (e.g., src/common/validators) to prevent drift and duplicated logic.
I can extract and update imports across DTOs if you want a patch.
docs/store-system.md (3)
99-139: Consider documenting idempotent default store creationThe pseudo-implementation checks for an existing default by name. If the seller changes their name, the check will miss and a second “default” store is created. Recommend documenting an
isDefaultboolean (unique per seller) or another stable discriminator to ensure idempotency across name changes.
188-198: Swagger table: minor formatting/grammar nitsThe Markdown table renders but LanguageTool flags minor issues. Optional polish:
- Ensure consistent header alignment and spacing.
- “Role Required” column values: consider code formatting (e.g.,
Seller,Admin) for consistency.No functional impact.
9-16: Minor grammar/style improvements (optional)Examples:
- “when seller registers” → “when a seller registers”
- Use consistent punctuation in bullet points; some lines lack terminal punctuation while others include it.
Purely editorial—no functional impact.
I can sweep the doc for grammar/style in one pass if you’d like.
Also applies to: 101-109, 403-418, 499-509
src/modules/stores/dto/store.dto.ts (2)
59-66: Optionally validate coordinates as numbersIf you intend to accept numeric coordinates, consider a nested DTO with
IsNumber()andValidateNestedrather than a raw object. Minimal improvement (optional):
- Create a
CoordinatesDtowith proper number validation.- Replace
@IsObject()with@ValidateNested()and@Type(() => CoordinatesDto).This prevents string values slipping through.
I can draft
CoordinatesDtoand wire it in.
245-294: Response DTO shape vs returned entity
StoreResponseDtomatches the intended output, but the controller returns full entities directly. Without explicit mapping or class-transformer, extra relations (e.g.,seller) may leak. Consider mapping entities toStoreResponseDtoor using@Exclude/@Exposeon the entity with a global ClassSerializerInterceptor.I can add a lightweight mapper function or wire
ClassSerializerInterceptorwhere appropriate.src/modules/stores/controllers/store.controller.ts (3)
59-64: Fix array responses in Swagger
type: [StoreResponseDto]is not the preferred way. UseApiOkResponsewithisArrayor an explicit schema inside your{ success, data }wrapper:
data: { type: 'array', items: { $ref: getSchemaPath(StoreResponseDto) } }Also applies to: 141-148, 157-164
118-139: DELETE response Swagger mismatchThe handler returns
{ success, message }but no schema is documented. Consider:- @ApiResponse({ - status: 200, - description: 'Store deleted successfully', - }) + @ApiOkResponse({ + description: 'Store deleted successfully', + schema: { + type: 'object', + properties: { + success: { type: 'boolean', example: true }, + message: { type: 'string', example: 'Store deleted successfully' } + } + } + })
141-175: Consider caching for public read endpoints
GET /storesandGET /stores/searchare good candidates for caching (e.g., Nest CacheInterceptor or a service-layer cache with TTL) to reduce DB load under traffic spikes.I can wire
CacheInterceptorfor these endpoints or add a service-level cache abstraction if you prefer finer control.Also applies to: 177-197
src/modules/stores/services/store.service.ts (4)
30-37: Default store idempotency is fragile (name-based uniqueness)You derive “existing store” by matching a generated name. If seller changes their name, a second default may be created. Consider persisting an
isDefaultflag and querying by{ sellerId, isDefault: true }, with a partial unique index on(sellerId) WHERE isDefault = true. If you do this, set the field during creation and adjust the existence check.Happy to draft the entity field, migration, and the query change.
Also applies to: 39-58
192-208: GuardverifiedAtsemantics and enforce valid status valuesWith the controller change to
ParseEnumPipe, invalid statuses will be blocked earlier. Optionally, add a simple guard here to be defensive if this service is called from elsewhere.Also consider resetting
verifiedAtwhen moving to non-ACTIVE statuses if that matches your business logic.
148-154: Add ordering tie-breakers for stable paginationWhen ordering by
ratingand thenreviewCount, consider addingcreatedAt DESCas a final tiebreaker to ensure stable sorting across equal metrics (helps cursor-based pagination later).Also applies to: 183-187
222-233: Type a StoreStatsDto and avoidanyDefine a
StoreStatsDtoto document the response. This improves Swagger and makes refactors safer.I can add
StoreStatsDto, export it from DTOs, and wire it into the controller/service.src/modules/stores/entities/store.entity.ts (1)
80-82: Numeric transformer for decimal columns (optional)Postgres decimal types are returned as strings by default. If you need
ratingas a number in JSON, add a transformer.Example:
export class ColumnNumericTransformer { to(data?: number): number | null { return typeof data === 'number' ? data : data == null ? null : Number(data); } from(data?: string | null): number | null { return data == null ? null : parseFloat(data); } }Then:
- @Column({ type: 'decimal', precision: 3, scale: 2, nullable: true }) + @Column({ type: 'decimal', precision: 3, scale: 2, nullable: true, transformer: new ColumnNumericTransformer() }) rating?: number;
| ```sql | ||
| CREATE TABLE users ( | ||
| id SERIAL PRIMARY KEY, | ||
| email VARCHAR UNIQUE, | ||
| name VARCHAR, | ||
| wallet_address VARCHAR UNIQUE NOT NULL, | ||
| location VARCHAR, -- NEW FIELD | ||
| country VARCHAR, -- NEW FIELD | ||
| buyer_data JSONB, -- NEW FIELD | ||
| seller_data JSONB, -- NEW FIELD | ||
| created_at TIMESTAMP DEFAULT NOW(), | ||
| updated_at TIMESTAMP DEFAULT NOW() | ||
| ); | ||
| ``` |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Column naming inconsistency (snake_case vs camelCase)
"Users Table" DDL shows wallet_address, buyer_data, etc., while the later migration snippet (Lines 298-304) uses "buyerData", "sellerData". Align docs to the actual migration/entity naming (and naming strategy) to prevent confusion.
Run to confirm actual migration columns:
🏁 Script executed:
#!/bin/bash
fd -t f -g '*AddUserFields*.ts' | while read -r f; do
echo "== $f ==";
rg -n -C1 'location|country|buyer|seller' "$f";
doneLength of output: 654
Align documentation column names with actual migrations
The migration adds buyerData and sellerData (camelCase), not buyer_data/seller_data. Update the docs snippet accordingly to prevent confusion:
• File: docs/IMPLEMENTATION_GUIDE.md (Lines 49–62)
- Change
buyer_data JSONB,→buyerData JSONB, - Change
seller_data JSONB,→sellerData JSONB,
Revised snippet:
CREATE TABLE users (
id SERIAL PRIMARY KEY,
email VARCHAR UNIQUE,
name VARCHAR,
wallet_address VARCHAR UNIQUE NOT NULL,
location VARCHAR, -- NEW FIELD
country VARCHAR, -- NEW FIELD
buyerData JSONB, -- NEW FIELD
sellerData JSONB, -- NEW FIELD
created_at TIMESTAMP DEFAULT NOW(),
updated_at TIMESTAMP DEFAULT NOW()
);🤖 Prompt for AI Agents
In docs/IMPLEMENTATION_GUIDE.md around lines 49 to 62, the SQL example uses
snake_case column names buyer_data and seller_data but the migrations use
camelCase buyerData and sellerData; update the SQL snippet to rename those two
columns to buyerData JSONB and sellerData JSONB so the documentation matches the
actual migration column names.
| ```typescript | ||
| @Entity('stores') | ||
| export class Store { | ||
| @PrimaryGeneratedColumn() | ||
| id: number; | ||
|
|
||
| @Column() | ||
| name: string; | ||
|
|
||
| @Column({ type: 'text', nullable: true }) | ||
| description?: string; | ||
|
|
||
| @Column({ nullable: true }) | ||
| logo?: string; | ||
|
|
||
| @Column({ nullable: true }) | ||
| banner?: string; | ||
|
|
||
| @Column({ type: 'jsonb', nullable: true }) | ||
| contactInfo?: ContactInfo; | ||
|
|
||
| @Column({ type: 'jsonb', nullable: true }) | ||
| address?: Address; | ||
|
|
||
| @Column({ type: 'jsonb', nullable: true }) | ||
| businessHours?: BusinessHours; | ||
|
|
||
| @Column({ type: 'jsonb', nullable: true }) | ||
| categories?: string[]; | ||
|
|
||
| @Column({ type: 'jsonb', nullable: true }) | ||
| tags?: string[]; | ||
|
|
||
| @Column({ type: 'decimal', precision: 3, scale: 2, nullable: true }) | ||
| rating?: number; | ||
|
|
||
| @Column({ type: 'int', default: 0 }) | ||
| reviewCount: number; | ||
|
|
||
| @Column({ type: 'jsonb', nullable: true }) | ||
| policies?: Policies; | ||
|
|
||
| @Column({ type: 'jsonb', nullable: true }) | ||
| settings?: StoreSettings; | ||
|
|
||
| @Column({ | ||
| type: 'enum', | ||
| enum: StoreStatus, | ||
| default: StoreStatus.PENDING_APPROVAL, | ||
| }) | ||
| status: StoreStatus; | ||
|
|
||
| @Column({ type: 'boolean', default: false }) | ||
| isVerified: boolean; | ||
|
|
||
| @Column({ type: 'boolean', default: false }) | ||
| isFeatured: boolean; | ||
|
|
||
| @ManyToOne(() => User, (user) => user.stores) | ||
| @JoinColumn({ name: 'seller_id' }) | ||
| seller: User; | ||
|
|
||
| @Column() | ||
| sellerId: number; | ||
| } | ||
| ``` |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Clarify entity definition to avoid accidental duplication
The example includes both a relation (with @JoinColumn) and an explicit sellerId column. In TypeORM, this easily results in two physical columns unless carefully aligned. Either:
- Let the relation create the
sellerIdcolumn via@JoinColumn({ name: 'sellerId' }), and avoid a separate@Columndeclaration; or - Use
@RelationIdfor thesellerIdproperty derived from the relation.
I’ve provided a safe pattern in the entity review.
🤖 Prompt for AI Agents
In docs/store-system.md around lines 21 to 86, the Store entity declares a
relation with @ManyToOne/@JoinColumn and also defines a separate sellerId
@Column which can create duplicate physical columns; remove the explicit @Column
declaration for sellerId and instead let the relation create the FK by setting
@JoinColumn({ name: 'sellerId' }) on the seller property, or alternatively
replace the separate @Column with a @RelationId(() => Store.seller) sellerId
property so the ID is derived from the relation rather than defined twice.
| @ManyToOne(() => User, (user) => user.stores) | ||
| @JoinColumn({ name: 'seller_id' }) | ||
| seller: User; | ||
|
|
||
| @Column() | ||
| sellerId: number; | ||
| } |
There was a problem hiding this comment.
Fix FK column naming mismatch (seller_id vs sellerId) across docs and code
The Store entity snippet shows @JoinColumn({ name: 'seller_id' }), while the SQL schema defines the FK as sellerId INTEGER ... and the service code queries by sellerId. This inconsistency will lead to duplicate columns or broken FKs if followed literally.
Unify on a single column name, preferably sellerId (camelCase), for:
- Entity relation JoinColumn
- Actual DB column and FK
- Service queries and DTOs
I’ve suggested a concrete entity fix under src/modules/stores/entities/store.entity.ts. Please mirror the same in the docs’ code blocks and DDL.
Also applies to: 363-386
🤖 Prompt for AI Agents
In docs/store-system.md around lines 79-85 (and also apply same change at
363-386), the entity snippet uses @JoinColumn({ name: 'seller_id' }) while the
rest of the code and SQL use sellerId; this mismatch will create duplicate
columns or broken FKs—change the JoinColumn name to 'sellerId' and ensure the
entity column/annotation, the DDL FK name, and all service/DTO references
consistently use sellerId (camelCase) across the docs and example SQL so the ORM
mapping, database schema, and service queries align.
| "walletAddress": "GABCDEFGHIJKLMNOPQRSTUVWXYZ1234567890123456789012345678901234567890", | ||
| "role": "buyer", |
There was a problem hiding this comment.
Fix invalid Stellar wallet address example (length/charset)
Examples must be 56 chars (^G[A-Z2-7]{55}$). The current sample exceeds 56. Replace with a valid-format example to prevent confusion.
- "walletAddress": "GABCDEFGHIJKLMNOPQRSTUVWXYZ1234567890123456789012345678901234567890",
+ "walletAddress": "GD6LXK4RB6D522ECACFVUEOKPCYBGQ6SKYONMVNIUOWUAIRNLSYAOB4Q",📝 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.
| "walletAddress": "GABCDEFGHIJKLMNOPQRSTUVWXYZ1234567890123456789012345678901234567890", | |
| "role": "buyer", | |
| "walletAddress": "GD6LXK4RB6D522ECACFVUEOKPCYBGQ6SKYONMVNIUOWUAIRNLSYAOB4Q", | |
| "role": "buyer", |
🤖 Prompt for AI Agents
In docs/user-registration.md around lines 35 to 36, the example Stellar
walletAddress is invalid (too long and contains unsupported characters); replace
the current sample with a 56-character valid-format example matching
^G[A-Z2-7]{55}$ (i.e., starts with 'G' followed by 55 chars from A-Z and 2-7) so
the documentation shows a correct Stellar public key format.
| "walletAddress": "GXYZABCDEFGHIJKLMNOPQRSTUVWXYZ1234567890123456789012345678901234567890", | ||
| "role": "seller", |
There was a problem hiding this comment.
Fix seller wallet example to a valid 56-char Stellar address
Current example exceeds 56 characters.
- "walletAddress": "GXYZABCDEFGHIJKLMNOPQRSTUVWXYZ1234567890123456789012345678901234567890",
+ "walletAddress": "GAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAWHF",📝 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.
| "walletAddress": "GXYZABCDEFGHIJKLMNOPQRSTUVWXYZ1234567890123456789012345678901234567890", | |
| "role": "seller", | |
| "walletAddress": "GAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAWHF", | |
| "role": "seller", |
🤖 Prompt for AI Agents
In docs/user-registration.md around lines 48 to 49, the example seller
walletAddress is too long (exceeds the 56-character length required for a valid
Stellar public key); replace the current string with a valid 56-character
Stellar G... public key (ensure it starts with 'G' and is exactly 56 chars) so
the example demonstrates a properly formatted Stellar address.
| const seller = await this.userRepository.findOne({ | ||
| where: { id: sellerId }, | ||
| relations: ['userRoles'], | ||
| }); | ||
|
|
||
| if (!seller) { | ||
| throw new NotFoundException('Seller not found'); | ||
| } | ||
|
|
||
| // Check if seller has seller role | ||
| const hasSellerRole = seller.userRoles.some(ur => ur.role.name === 'seller'); | ||
| if (!hasSellerRole) { | ||
| throw new BadRequestException('Only sellers can create stores'); |
There was a problem hiding this comment.
Role check may throw due to missing role relation; load userRoles.role
relations: ['userRoles'] does not load role inside userRoles, so ur.role.name can be undefined and cause a crash.
Apply:
- const seller = await this.userRepository.findOne({
- where: { id: sellerId },
- relations: ['userRoles'],
- });
+ const seller = await this.userRepository.findOne({
+ where: { id: sellerId },
+ relations: ['userRoles', 'userRoles.role'],
+ });
...
- const hasSellerRole = seller.userRoles.some(ur => ur.role.name === 'seller');
+ const hasSellerRole = seller.userRoles?.some((ur) => ur.role?.name === 'seller') ?? false;📝 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.
| const seller = await this.userRepository.findOne({ | |
| where: { id: sellerId }, | |
| relations: ['userRoles'], | |
| }); | |
| if (!seller) { | |
| throw new NotFoundException('Seller not found'); | |
| } | |
| // Check if seller has seller role | |
| const hasSellerRole = seller.userRoles.some(ur => ur.role.name === 'seller'); | |
| if (!hasSellerRole) { | |
| throw new BadRequestException('Only sellers can create stores'); | |
| const seller = await this.userRepository.findOne({ | |
| where: { id: sellerId }, | |
| relations: ['userRoles', 'userRoles.role'], | |
| }); | |
| if (!seller) { | |
| throw new NotFoundException('Seller not found'); | |
| } | |
| // Check if seller has seller role | |
| const hasSellerRole = seller.userRoles?.some((ur) => ur.role?.name === 'seller') ?? false; | |
| if (!hasSellerRole) { | |
| throw new BadRequestException('Only sellers can create stores'); | |
| } |
🤖 Prompt for AI Agents
In src/modules/stores/services/store.service.ts around lines 64 to 76, the role
lookup can throw because relations only load userRoles but not the nested role,
causing ur.role to be undefined; update the query to load the nested relation
(e.g., include userRoles.role) and make the runtime check defensive (use a
null-safe check for ur.role?.name === 'seller') so missing role objects won't
crash and only users with the seller role pass.
| async updateStore(storeId: number, sellerId: number, updateStoreDto: UpdateStoreDto): Promise<Store> { | ||
| const store = await this.storeRepository.findOne({ | ||
| where: { id: storeId, sellerId }, | ||
| }); | ||
|
|
||
| if (!store) { | ||
| throw new NotFoundException('Store not found or access denied'); | ||
| } | ||
|
|
||
| Object.assign(store, updateStoreDto); | ||
| return await this.storeRepository.save(store); | ||
| } |
There was a problem hiding this comment.
Block admin-only fields from seller updates
Sellers should not be able to set status, isVerified, or isFeatured. Strip these before persisting.
async updateStore(storeId: number, sellerId: number, updateStoreDto: UpdateStoreDto): Promise<Store> {
const store = await this.storeRepository.findOne({
where: { id: storeId, sellerId },
});
@@
- Object.assign(store, updateStoreDto);
+ const { status, isVerified, isFeatured, ...allowed } = (updateStoreDto ?? {}) as any;
+ Object.assign(store, allowed);
return await this.storeRepository.save(store);
}📝 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.
| async updateStore(storeId: number, sellerId: number, updateStoreDto: UpdateStoreDto): Promise<Store> { | |
| const store = await this.storeRepository.findOne({ | |
| where: { id: storeId, sellerId }, | |
| }); | |
| if (!store) { | |
| throw new NotFoundException('Store not found or access denied'); | |
| } | |
| Object.assign(store, updateStoreDto); | |
| return await this.storeRepository.save(store); | |
| } | |
| async updateStore(storeId: number, sellerId: number, updateStoreDto: UpdateStoreDto): Promise<Store> { | |
| const store = await this.storeRepository.findOne({ | |
| where: { id: storeId, sellerId }, | |
| }); | |
| if (!store) { | |
| throw new NotFoundException('Store not found or access denied'); | |
| } | |
| const { status, isVerified, isFeatured, ...allowed } = (updateStoreDto ?? {}) as any; | |
| Object.assign(store, allowed); | |
| return await this.storeRepository.save(store); | |
| } |
🤖 Prompt for AI Agents
In src/modules/stores/services/store.service.ts around lines 117 to 128, sellers
must be prevented from changing admin-only fields; before assigning and saving,
remove or ignore status, isVerified, and isFeatured from the incoming
updateStoreDto. Implement a whitelist or explicitly delete those three keys from
the DTO (e.g., create a new object with only allowed fields or copy
updateStoreDto and delete status, isVerified, isFeatured) then Object.assign the
sanitized object to store and save; keep the NotFoundException logic intact.
| @Column({ type: 'json', nullable: true }) | ||
| buyerData?: any; | ||
|
|
||
| @Column({ type: 'json', nullable: true }) | ||
| sellerData?: any; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Use Postgres JSONB and stronger typing for buyer/seller data
Entities declare type: 'json' with any, while docs/migrations reference JSONB. Aligning to jsonb avoids operator inefficiencies and matches migrations. Also, prefer a safer type.
Proposed change:
- @Column({ type: 'json', nullable: true })
- buyerData?: any;
+ @Column({ type: 'jsonb', nullable: true })
+ buyerData?: Record<string, unknown>;
- @Column({ type: 'json', nullable: true })
- sellerData?: any;
+ @Column({ type: 'jsonb', nullable: true })
+ sellerData?: Record<string, unknown>;To verify migration alignment, run:
🏁 Script executed:
#!/bin/bash
# Check migration column names/types for user fields
fd -t f -g '*AddUserFields*.ts' | while read -r f; do
echo "== $f ==";
sed -n '1,200p' "$f";
doneLength of output: 1019
Align User Entity JSON Columns with Migrations and Add Stronger Typing
The migration src/migrations/1751199237000-AddUserFields.ts adds buyerData and sellerData as jsonb. The entity should match:
- In
src/modules/users/entities/user.entity.ts:- Change
type: 'json'→type: 'jsonb' - Replace
any→Record<string, unknown>
- Change
Proposed diff:
--- a/src/modules/users/entities/user.entity.ts
+++ b/src/modules/users/entities/user.entity.ts
@@ lines 35-39
- @Column({ type: 'json', nullable: true })
- buyerData?: any;
+ @Column({ type: 'jsonb', nullable: true })
+ buyerData?: Record<string, unknown>;
- @Column({ type: 'json', nullable: true })
- sellerData?: any;
+ @Column({ type: 'jsonb', nullable: true })
+ sellerData?: Record<string, unknown>;This ensures alignment with migrations and safer typings.
📝 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.
| @Column({ type: 'json', nullable: true }) | |
| buyerData?: any; | |
| @Column({ type: 'json', nullable: true }) | |
| sellerData?: any; | |
| @Column({ type: 'jsonb', nullable: true }) | |
| buyerData?: Record<string, unknown>; | |
| @Column({ type: 'jsonb', nullable: true }) | |
| sellerData?: Record<string, unknown>; |
🤖 Prompt for AI Agents
In src/modules/users/entities/user.entity.ts around lines 35 to 39, the entity
defines buyerData and sellerData as @Column({ type: 'json', nullable: true })
buyerData?: any; and sellerData?: any;, which does not match the migration that
created jsonb columns and uses overly permissive typing; change the column type
to 'jsonb' and replace the any types with Record<string, unknown> for both
properties so the entity matches migrations and has stronger typing.
| if (data.location !== undefined) { | ||
| user.location = data.location; | ||
| } | ||
|
|
||
| if (data.country !== undefined) { | ||
| user.country = data.country; | ||
| } | ||
|
|
||
| if (data.buyerData !== undefined) { | ||
| user.buyerData = data.buyerData; | ||
| } | ||
|
|
||
| if (data.sellerData !== undefined) { | ||
| user.sellerData = data.sellerData; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Block cross-role updates based on the persisted user role
A buyer can currently set sellerData and vice versa. Derive role from the user’s roles and enforce symmetric constraints on update.
Apply this diff:
if (data.location !== undefined) {
user.location = data.location;
}
if (data.country !== undefined) {
user.country = data.country;
}
+ // Enforce role-specific update constraints
+ const roleName = user.userRoles?.[0]?.role?.name;
+ if (roleName === 'buyer' && data.sellerData !== undefined) {
+ throw new BadRequestError('Buyers cannot have seller data');
+ }
+ if (roleName === 'seller' && data.buyerData !== undefined) {
+ throw new BadRequestError('Sellers cannot have buyer data');
+ }
+
if (data.buyerData !== undefined) {
user.buyerData = data.buyerData;
}
if (data.sellerData !== undefined) {
user.sellerData = data.sellerData;
}| role: [Role.BUYER], | ||
| }, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Sanity check: ensure no lingering Role.USER usages.
If the enum dropped USER in favor of BUYER/SELLER, update any stale tests or mocks to avoid breakage.
Run this to find any remaining references:
Also applies to: 102-103, 122-123, 140-141
🏁 Script executed:
#!/bin/bash
# Find Role.USER references in TS files (with surrounding context)
rg -nP --type=ts -C2 '\bRole\.USER\b'
# Also check common test mock patterns, if any
rg -nP --type=ts -C2 'role:\s*\[\s*Role\.USER\s*\]'Length of output: 857
Remove stale Role.USER references in tests
Found a lingering Role.USER usage that will break now that USER has been removed:
- src/modules/files/tests/file.controller.spec.ts:36
role: [Role.USER],
Please update this to use the appropriate new enum value (e.g. Role.BUYER or Role.SELLER) so tests continue to pass.
🤖 Prompt for AI Agents
In src/modules/wishlist/tests/wishlist.controller.spec.ts around lines 85-86,
there is a stale reference to Role.USER (or other tests may still reference it);
replace any occurrences of Role.USER with the appropriate new enum value (e.g.,
Role.BUYER or Role.SELLER) that matches the test scenario, update any imports if
needed, and run the test suite to confirm the change passes.
2 participants
🚀 StarShop Pull Request
Mark with an
xall the checkboxes that apply (like[x])📌 Type of Change
📝 Changes description
Added the back end code to register either as a buyer or seller and the code to ensure the distinction of either role and data.
📸 Evidence (A photo is required as evidence)
⏰ Time spent breakdown
1 Hour analyzing
2 Hours working in the implementation
🌌 Comments
I ensured that the implementation wont interfere with future updates and features. Thanks a lot for letting me help with the development of this amazing project!
Thank you for contributing to StarShop, we are glad that you have chosen us as your project of choice and we hope that you continue to contribute to this great project, so that together we can make our mark at the top!
Summary by CodeRabbit
New Features
API Changes
Documentation
Chores