Pull Request for Auto-expire BuyerRequests Based on expiresAt#125
Pull Request for Auto-expire BuyerRequests Based on expiresAt#125Villarley merged 38 commits intoStarShopCr:mainfrom
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis change implements automatic and manual closing of expired buyer requests. It introduces a scheduled service using Changes
Sequence Diagram(s)sequenceDiagram
participant Scheduler as BuyerRequestSchedulerService
participant DB as Database
participant Logger as Logger
Note over Scheduler: Scheduled every 10 min or on app start
Scheduler->>DB: Find buyer_requests where status=OPEN and expiresAt < now
alt Expired requests found
Scheduler->>DB: Update status to CLOSED
Scheduler->>Logger: Log count and details of closed requests
else No expired requests
Scheduler->>Logger: Log "No expired requests found"
end
sequenceDiagram
participant Buyer as Buyer (API Client)
participant Controller as BuyerRequestsController
participant Service as BuyerRequestsService
participant DB as Database
Buyer->>Controller: PATCH /buyer-requests/:id/close
Controller->>Service: closeRequest(id, userId)
Service->>DB: Find buyer request by id
alt Owner and request is open
Service->>DB: Update status to CLOSED
Service->>Controller: Return updated request DTO
else Not owner or already closed
Service->>Controller: Throw Forbidden or BadRequest
end
Controller->>Buyer: Response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (12)
✨ 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 comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🔭 Outside diff range comments (1)
src/modules/offers/controllers/offers.controller.ts (1)
75-77: Inconsistent ID type conversion in accept method.The method converts
req.user.idtoString()while other methods in the same controller convert it toNumber(), creating inconsistency.accept(@Param('id') id: string, @Request() req: AuthRequest) { - return this.offersService.accept(id, String(req.user.id)); + return this.offersService.accept(id, Number(req.user.id)); }
🧹 Nitpick comments (12)
src/main.ts (1)
32-39: Consider startup performance impact of synchronous cleanup.The startup cleanup logic is well-implemented with proper error handling. However, consider the potential performance impact if there are many expired requests to process during application startup.
Consider making this operation non-blocking if startup time becomes an issue:
// Close expired buyer requests on startup - try { - const schedulerService = app.get(BuyerRequestSchedulerService); - const closedCount = await schedulerService.closeExpiredRequests(); - console.log(`Startup: Closed ${closedCount} expired buyer requests`); - } catch (error) { - console.error('Failed to close expired requests on startup:', error); - } + // Non-blocking startup cleanup + setImmediate(async () => { + try { + const schedulerService = app.get(BuyerRequestSchedulerService); + const closedCount = await schedulerService.closeExpiredRequests(); + console.log(`Startup: Closed ${closedCount} expired buyer requests`); + } catch (error) { + console.error('Failed to close expired requests on startup:', error); + } + });commit_files.sh (1)
59-62: Consider workflow implications of automated commits.Creating multiple individual commits may not align with all development workflows and could clutter the git history unnecessarily.
Consider adding a flag to optionally squash commits or allow users to choose between individual commits and a single comprehensive commit for the entire feature.
src/modules/buyer-requests/tests/buyer-request-scheduler.service.spec.ts (2)
11-23: Consider using more realistic test data.The mock expired requests use hardcoded dates from 2023, which may become confusing over time as the dates become increasingly outdated.
- expiresAt: new Date('2023-01-01'), + expiresAt: new Date(Date.now() - 24 * 60 * 60 * 1000), // Yesterday- expiresAt: new Date('2023-01-02'), + expiresAt: new Date(Date.now() - 48 * 60 * 60 * 1000), // Two days ago
55-68: Improve test specificity for query conditions.The test uses
expect.any(Object)for theLessThanmatcher, which doesn't verify that the correct current timestamp is being used.expect(repository.find).toHaveBeenCalledWith({ where: { status: BuyerRequestStatus.OPEN, - expiresAt: expect.any(Object), // LessThan matcher + expiresAt: expect.objectContaining({ + _type: 'lessThan', + _value: expect.any(Date) + }), }, });src/modules/offers/tests/offer.entity.spec.ts (1)
112-119: Add meaningful assertion for constraint testing.The test claims to enforce a price constraint but doesn't actually test the constraint - it just verifies the value was set.
it('should enforce price >= 0 constraint', () => { const offer = new Offer(); offer.price = -10; // Invalid negative price - // In a real database, this would throw an error - // Here we just verify the value is set - expect(offer.price).toBe(-10); + // Note: Entity-level validation would be handled by class-validator decorators + // or database constraints. This test verifies the property accepts the value + // but actual validation should be tested separately. + expect(offer.price).toBeLessThan(0); });src/modules/buyer-requests/tests/buyer-requests.integration.spec.ts (1)
51-51: Long timeout might indicate slow test setup.The 30-second timeout suggests the test setup might be slow. Consider optimizing the setup or investigating why it needs such a long timeout.
src/modules/buyer-requests/services/buyer-request-scheduler.service.ts (1)
25-35: Consider potential performance impact of large result sets.The
find()operation loads all expired requests into memory before the update. For large numbers of expired requests, this could cause memory issues.Consider using a more memory-efficient approach:
- // Find all open requests that have expired - const expiredRequests = await this.buyerRequestRepository.find({ - where: { - status: BuyerRequestStatus.OPEN, - expiresAt: LessThan(new Date()), - }, - }); - - if (expiredRequests.length === 0) { - this.logger.log('No expired buyer requests found'); - return; - } + // Get count of expired requests for logging + const expiredCount = await this.buyerRequestRepository.count({ + where: { + status: BuyerRequestStatus.OPEN, + expiresAt: LessThan(new Date()), + }, + }); + + if (expiredCount === 0) { + this.logger.log('No expired buyer requests found'); + return; + }Then update the logging to use
expiredCountinstead ofexpiredRequests.length.src/modules/offers/tests/offer.service.spec.ts (2)
19-23: Mixed ID types may cause confusion.The code has mixed ID types -
mockSellerIdandmockBuyerRequestIdare numbers, butmockOfferIdis still a string. This inconsistency could lead to confusion.Consider standardizing all mock IDs to the same type for consistency:
- const mockSellerId = 1; - const mockBuyerId = 1; - const mockBuyerRequestId = 123; - const mockOfferId = 'offer-uuid-1'; + const mockSellerId = 1; + const mockBuyerId = 1; + const mockBuyerRequestId = 123; + const mockOfferId = 1; // or keep as string if offers use UUID
100-127: Test assertion could be more specific.The test only checks
expect(result).toEqual(mockPendingOffer)but could be more specific about what properties are important to verify after creation.Consider adding more specific assertions:
const result = await service.create(createOfferDto, mockSellerId); - expect(result).toEqual(mockPendingOffer); + expect(result).toEqual(mockPendingOffer); + expect(result.status).toBe(OfferStatus.PENDING); + expect(result.sellerId).toBe(mockSellerId); + expect(result.buyerRequestId).toBe(mockBuyerRequestId);src/modules/offers/services/offers.service.ts (1)
58-125: Consider consistent typing for ID parameters.While
sellerIdandbuyerRequestIdhave been changed tonumber, theofferIdandbuyerIdparameters inacceptandrejectmethods remain asstring. Additionally, the comparisons on lines 68 and 114 requiretoString()conversion, suggestinguserIdis a number in the entity.For consistency across the codebase, consider updating these methods to use numeric IDs as well.
src/modules/buyer-requests/services/buyer-requests.service.ts (1)
77-80: Consider database portability for full-text search.The query uses PostgreSQL-specific functions (
to_tsvector,plainto_tsquery). While the ILIKE fallback provides basic functionality, consider abstracting the search logic to support different databases in the future.src/modules/buyer-requests/controllers/buyer-requests.controller.ts (1)
33-38: Consider adding explicit @Body() decorators for consistency.For better clarity and consistency with NestJS best practices, consider adding explicit
@Body()decorators to DTO parameters:create( - createBuyerRequestDto: CreateBuyerRequestDto, + @Body() createBuyerRequestDto: CreateBuyerRequestDto, @Request() req ): Promise<BuyerRequestResponseDto> {update( @Param('id', ParseIntPipe) id: number, - updateBuyerRequestDto: UpdateBuyerRequestDto, + @Body() updateBuyerRequestDto: UpdateBuyerRequestDto, @Request() req ): Promise<BuyerRequestResponseDto> {Also applies to: 65-71, 82-84
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (32)
commit_files.sh(1 hunks)package.json(1 hunks)src/app.module.ts(3 hunks)src/main.ts(2 hunks)src/modules/buyer-request/buyer-request.entity.spec.ts(0 hunks)src/modules/buyer-request/buyer-request.entity.ts(0 hunks)src/modules/buyer-request/enums/buyer-request-status.enum.ts(0 hunks)src/modules/buyer-request/validators/budget-range.validator.ts(0 hunks)src/modules/buyer-requests/buyer-requests.module.ts(1 hunks)src/modules/buyer-requests/controllers/buyer-requests.controller.ts(1 hunks)src/modules/buyer-requests/entities/buyer-request.entity.ts(1 hunks)src/modules/buyer-requests/services/buyer-request-scheduler.service.ts(1 hunks)src/modules/buyer-requests/services/buyer-requests.service.ts(1 hunks)src/modules/buyer-requests/tests/buyer-request-scheduler.service.spec.ts(1 hunks)src/modules/buyer-requests/tests/buyer-requests.controller.spec.ts(3 hunks)src/modules/buyer-requests/tests/buyer-requests.integration.spec.ts(4 hunks)src/modules/buyer-requests/tests/buyer-requests.service.spec.ts(4 hunks)src/modules/buyerRequests/buyerRequests.module.ts(0 hunks)src/modules/buyerRequests/controllers/buyerRequestsAdmin.controller.ts(0 hunks)src/modules/buyerRequests/dto/block-buyer-request.dto.ts(0 hunks)src/modules/buyerRequests/entities/buyerRequest.entity.ts(0 hunks)src/modules/buyerRequests/services/buyerRequestsAdmin.service.ts(0 hunks)src/modules/offers/controllers/offers.controller.ts(5 hunks)src/modules/offers/controllers/offersAdmin.controller.ts(1 hunks)src/modules/offers/dto/create-offer.dto.ts(1 hunks)src/modules/offers/entities/offer.entity.ts(2 hunks)src/modules/offers/services/offer-attachment.service.ts(5 hunks)src/modules/offers/services/offers.service.ts(6 hunks)src/modules/offers/services/offersAdmin.service.ts(1 hunks)src/modules/offers/tests/offer-attachment.service.spec.ts(3 hunks)src/modules/offers/tests/offer.entity.spec.ts(5 hunks)src/modules/offers/tests/offer.service.spec.ts(4 hunks)
💤 Files with no reviewable changes (9)
- src/modules/buyerRequests/buyerRequests.module.ts
- src/modules/buyer-request/enums/buyer-request-status.enum.ts
- src/modules/buyerRequests/dto/block-buyer-request.dto.ts
- src/modules/buyer-request/buyer-request.entity.spec.ts
- src/modules/buyer-request/validators/budget-range.validator.ts
- src/modules/buyerRequests/services/buyerRequestsAdmin.service.ts
- src/modules/buyerRequests/entities/buyerRequest.entity.ts
- src/modules/buyerRequests/controllers/buyerRequestsAdmin.controller.ts
- src/modules/buyer-request/buyer-request.entity.ts
🧰 Additional context used
🧬 Code Graph Analysis (9)
src/modules/buyer-requests/buyer-requests.module.ts (1)
src/app.module.ts (1)
Module(38-86)
src/modules/buyer-requests/entities/buyer-request.entity.ts (1)
src/modules/offers/entities/offer.entity.ts (1)
Entity(18-83)
src/modules/offers/tests/offer-attachment.service.spec.ts (2)
src/modules/files/tests/test-utils.ts (1)
mockFile(10-23)src/modules/files/index.ts (1)
FileType(2-2)
src/app.module.ts (5)
src/modules/buyer-requests/buyer-requests.module.ts (1)
Module(9-15)src/modules/files/files.module.ts (1)
Module(4-8)src/modules/offers/offers.module.ts (1)
Module(11-17)src/modules/coupons/coupon.module.ts (1)
Module(10-23)src/modules/shared/shared.module.ts (1)
Module(7-17)
src/modules/offers/controllers/offersAdmin.controller.ts (1)
src/modules/offers/dto/block-offer.dto.ts (1)
BlockOfferDto(3-7)
src/modules/offers/controllers/offers.controller.ts (3)
src/modules/wishlist/common/types/auth-request.type.ts (1)
AuthRequest(4-14)src/modules/offers/dto/update-offer.dto.ts (1)
UpdateOfferDto(4-25)src/modules/offers/dto/upload-attachment.dto.ts (1)
UploadAttachmentDto(3-7)
src/modules/offers/services/offer-attachment.service.ts (1)
src/modules/offers/dto/offer-attachment-response.dto.ts (1)
OfferAttachmentResponseDto(3-11)
src/modules/offers/tests/offer.service.spec.ts (1)
src/modules/offers/dto/create-offer.dto.ts (1)
CreateOfferDto(14-55)
src/modules/buyer-requests/tests/buyer-requests.integration.spec.ts (1)
src/modules/buyer-requests/entities/buyer-request.entity.ts (1)
Entity(21-75)
🔇 Additional comments (37)
package.json (1)
45-45: Approve @nestjs/schedule AdditionThe dependency
@nestjs/schedule@^6.0.0is the latest stable release and has no known security vulnerabilities. It’s correctly listed underdependenciesfor runtime use—no further action needed.• package.json (line 45):
"@nestjs/schedule": "^6.0.0"src/modules/offers/controllers/offersAdmin.controller.ts (1)
19-19: String parameter type is consistent across controller and service – approved.The
blockmethod inOffersAdminServicenow declaresasync block(id: string, isBlocked: boolean), which aligns perfectly with the controller’s@Param('id') id: string. No further changes are needed here.src/modules/buyer-requests/buyer-requests.module.ts (1)
1-16: LGTM - Module updates support scheduler functionality.The additions are well-structured:
- Adding
Userentity enables user-related operations in the schedulerBuyerRequestSchedulerServiceproperly added to both providers and exports for dependency injection and external access- Changes align with the PR objectives for auto-expire functionality
src/modules/offers/tests/offer-attachment.service.spec.ts (1)
1-231: LGTM - Formatting and type consistency improvements.The changes are purely stylistic and type consistency improvements:
- Consistent use of single quotes
sellerIdtype changed from string to number aligning with codebase standardization- Test logic and assertions remain functionally identical
- Proper semicolon usage throughout
src/main.ts (2)
5-5: LGTM: Service import is correctly added.The import for
BuyerRequestSchedulerServiceis properly added to support the startup cleanup functionality.
7-7: Good addition: Explicit return type improves type safety.Adding the explicit
Promise<void>return type to the bootstrap function enhances code clarity and type safety.src/modules/offers/services/offersAdmin.service.ts (2)
10-10: LGTM: Property formatting is consistent.The readonly modifier formatting is appropriate.
17-17: ID type consistency confirmed
Theidparameter is defined as a string (UUID) on theOfferentity and is used asstringin all related service methods (block,findOne,update,remove), so no changes are required.src/modules/buyer-requests/entities/buyer-request.entity.ts (4)
11-13: Good: Consistent import formatting and relative paths.The formatting changes to use single quotes and relative import paths improve code consistency.
15-19: LGTM: Enum formatting is consistent.The single quote formatting for enum values is consistent with the codebase style.
21-27: Excellent: Well-designed database indexes for performance.The indexes are strategically chosen to support the expected query patterns:
- Full-text search index for search functionality
- Category, budget range, expiration, status, and creation date indexes for filtering and sorting
- Particularly good to see the expiration index supporting the new auto-expire feature
33-75: LGTM: Entity structure and relationships are correctly defined.The entity fields, relationships, and TypeORM decorators are properly configured:
- Appropriate column types and constraints
- Correct foreign key relationships with User and Offer entities
- Proper indexing on foreign keys
- Search vector field for full-text search capabilities
src/modules/offers/entities/offer.entity.ts (3)
25-25: LGTM: Foreign key type aligns with BuyerRequest entity.The change from
stringtonumberforbuyerRequestIdcorrectly aligns with the BuyerRequest entity's new numeric primary key type.
35-35: LGTM: Foreign key type aligns with User entity.The change from
stringtonumberforsellerIdcorrectly aligns with the User entity's numeric primary key type.
24-35: Numeric foreign key handling verifiedThe
CreateOfferDtocorrectly uses@Transform(({ value }) => Number.parseInt(value))alongside@IsNumber(), and all service methods accept and propagatebuyerRequestId: numberandsellerId: numberwhen querying or saving offers. No further changes are needed.src/app.module.ts (4)
4-4: LGTM: ScheduleModule import added for cron job support.The import is necessary to support the new
BuyerRequestSchedulerServicecron functionality.
1-36: Good: Consistent single quote formatting throughout imports.The formatting changes to use single quotes consistently across all imports improve code style uniformity.
41-41: LGTM: ScheduleModule properly configured for the application.The
ScheduleModule.forRoot()configuration enables the cron job scheduler that theBuyerRequestSchedulerServicedepends on for automatic expiration of buyer requests.
43-69: Good: TypeORM configuration formatting improved.The formatting changes to use single quotes and explicit string types in the TypeORM configuration improve code consistency without affecting functionality.
src/modules/buyer-requests/tests/buyer-requests.controller.spec.ts (2)
21-24: LGTM!The mock service setup correctly includes all the new methods (
getSearchSuggestions,getPopularCategories,closeRequest) that were added to the service interface.
159-185: closeRequest correctly returns expiry fieldsThe
closeRequestservice method invokesmapToResponseDto, which computes and includes bothisExpiringSoonanddaysUntilExpirybefore returning the DTO. No changes are required.src/modules/buyer-requests/tests/buyer-request-scheduler.service.spec.ts (1)
80-89: LGTMN!Excellent error handling test that verifies the service doesn't throw errors when database operations fail, which is crucial for a scheduled background task.
src/modules/buyer-requests/tests/buyer-requests.integration.spec.ts (2)
10-24: LGTM! Good test isolation with simplified TestUser entity.The introduction of a simplified
TestUserentity for testing is a good practice that removes dependencies on the full user module and complex relationships. This makes the tests more focused and faster to execute.
32-46: Verify the dynamic imports are working correctly.The test setup uses dynamic imports for the controller and service. While this can work, it might make the tests less readable and could potentially cause issues if the imports fail.
Consider using static imports for better reliability:
- controllers: [ - (await import('../controllers/buyer-requests.controller')).BuyerRequestsController, - ], - providers: [(await import('../services/buyer-requests.service')).BuyerRequestsService], + controllers: [BuyerRequestsController], + providers: [BuyerRequestsService],And add the static imports at the top:
+import { BuyerRequestsController } from '../controllers/buyer-requests.controller'; +import { BuyerRequestsService } from '../services/buyer-requests.service';src/modules/offers/services/offer-attachment.service.ts (1)
1-190: LGTM! Clean formatting improvements.The formatting changes (single quotes, consistent semicolons) improve code consistency. The business logic remains unchanged and the error handling is comprehensive.
src/modules/buyer-requests/services/buyer-request-scheduler.service.ts (2)
19-19: LGTM! Correct cron expression for 10-minute intervals.The cron expression
'0 */10 * * * *'correctly runs every 10 minutes as required.
62-82: LGTM! Clean manual closure method.The manual method is well-implemented with proper error handling and logging. It's good that it rethrows errors for proper error propagation to the caller.
src/modules/buyer-requests/tests/buyer-requests.service.spec.ts (3)
36-56: LGTM! Improved mock data factory.The enhanced
createMockBuyerRequestfactory withuserIdandoffersarray provides more complete mock data for testing.
305-368: Excellent test coverage for closeRequest method.The test suite comprehensively covers all scenarios:
- Successful closure
- Request not found
- Forbidden access (wrong user)
- Already closed request
The test data setup and assertions are well-structured.
332-347: closeRequest repository calls align with test expectations
buyerRequestRepository.findOneis called with{ where: { id }, relations: ['user'] }buyerRequestRepository.saveis called with the request object updated tostatus: BuyerRequestStatus.CLOSED(deep-equal to{ ...mockOpenRequest, status: CLOSED })No changes required.
src/modules/offers/tests/offer.service.spec.ts (1)
50-64: LGTM! Good use of factory function for mock data.The
createMockPendingOfferfactory function provides consistent and complete mock data. This is a good practice for test maintainability.src/modules/offers/services/offers.service.ts (3)
1-13: LGTM! Import reorganization improves readability.The import statements have been properly formatted following NestJS conventions with framework imports first, followed by external dependencies and internal modules.
24-56: Good type consistency improvement.The change from
stringtonumberforsellerIdand removal of explicitNumber()casting forbuyerRequestIdimproves type safety and code clarity.
150-201: LGTM! Consistent numeric ID usage.The
update,remove,findByBuyerRequest, andfindBySellermethods have been properly updated to use numeric IDs, maintaining consistency with the entity changes.src/modules/buyer-requests/services/buyer-requests.service.ts (2)
25-57: Well-implemented create method with proper validations.The method correctly validates budget ranges and expiration dates, sets sensible defaults, and properly initializes the buyer request status.
286-314: Excellent implementation of manual close functionality.The method properly validates ownership, prevents redundant operations, and correctly updates the request status. This aligns perfectly with the PR objectives for buyer-only access control.
src/modules/buyer-requests/controllers/buyer-requests.controller.ts (1)
79-84: Well-implemented close endpoint with proper authorization.The endpoint correctly uses JWT authentication and role-based access control to ensure only buyers can close their own requests. This perfectly implements the requirements from issue #103.
| # Add and commit package.json | ||
| git add package.json | ||
| git commit -m "feat: add schedule package dependency for cron jobs" | ||
|
|
||
| # Add and commit package-lock.json | ||
| git add package-lock.json | ||
| git commit -m "feat: update package-lock with nestjs schedule deps" | ||
|
|
||
| # Add and commit app.module.ts | ||
| git add src/app.module.ts | ||
| git commit -m "feat: configure ScheduleModule for cron tasks" | ||
|
|
||
| # Add and commit main.ts | ||
| git add src/main.ts | ||
| git commit -m "feat: add startup hook for expired requests cleanup" | ||
|
|
||
| # Add and commit buyer-requests module | ||
| git add src/modules/buyer-requests/buyer-requests.module.ts | ||
| git commit -m "feat: integrate scheduler service in buyer requests module" | ||
|
|
||
| # Add and commit buyer-requests controller | ||
| git add src/modules/buyer-requests/controllers/buyer-requests.controller.ts | ||
| git commit -m "feat: add close endpoint for buyer requests" | ||
|
|
||
| # Add and commit buyer-requests service | ||
| git add src/modules/buyer-requests/services/buyer-requests.service.ts | ||
| git commit -m "feat: implement manual close request functionality" | ||
|
|
||
| # Add and commit scheduler service (new file) | ||
| git add src/modules/buyer-requests/services/buyer-request-scheduler.service.ts | ||
| git commit -m "feat: create auto-expire scheduler for buyer requests" | ||
|
|
||
| # Add and commit controller tests | ||
| git add src/modules/buyer-requests/tests/buyer-requests.controller.spec.ts | ||
| git commit -m "test: add tests for close request endpoint" | ||
|
|
||
| # Add and commit service tests | ||
| git add src/modules/buyer-requests/tests/buyer-requests.service.spec.ts | ||
| git commit -m "test: add comprehensive tests for close request method" | ||
|
|
||
| # Add and commit scheduler service tests (new file) | ||
| git add src/modules/buyer-requests/tests/buyer-request-scheduler.service.spec.ts | ||
| git commit -m "test: add scheduler service tests with error handling" | ||
|
|
||
| # Add and commit the script itself | ||
| git add commit_files.sh | ||
| git commit -m "feat: add script for individual file commits" | ||
|
|
||
| echo "All files committed individually!" | ||
| echo "Total commits created: 12" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and file existence validation.
The script lacks error handling and doesn't verify file existence before attempting to commit, which could result in failed operations or empty commits.
+# Function to safely add and commit a file
+safe_commit() {
+ local file_path="$1"
+ local commit_message="$2"
+
+ if [ ! -f "$file_path" ]; then
+ echo "Warning: File $file_path does not exist, skipping..."
+ return 1
+ fi
+
+ if ! git add "$file_path"; then
+ echo "Error: Failed to add $file_path"
+ return 1
+ fi
+
+ if ! git commit -m "$commit_message"; then
+ echo "Error: Failed to commit $file_path"
+ return 1
+ fi
+
+ echo "Successfully committed: $file_path"
+}
-# Add and commit package.json
-git add package.json
-git commit -m "feat: add schedule package dependency for cron jobs"
+# Add and commit package.json
+safe_commit "package.json" "feat: add schedule package dependency for cron jobs"Apply similar changes to all other file commits in the script.
🤖 Prompt for AI Agents
In commit_files.sh around lines 8 to 57, the script commits files individually
without checking if each file exists or handling errors during git add or commit
commands. To fix this, add a check before each git add to verify the file
exists, and wrap git add and git commit commands with error handling to catch
and report failures. Apply these changes consistently for all file commits in
the script to prevent empty commits or failed operations.
| @PrimaryGeneratedColumn() | ||
| id: number; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify database migration for primary key type change.
Changing from UUID string to numeric auto-generated primary key is a significant breaking change that requires careful database migration planning.
🏁 Script executed:
#!/bin/bash
# Check for database migration files related to this change
echo "=== Looking for migration files ==="
fd -e ts -e js migration src/
fd -e sql migration
echo "=== Checking for references to UUID-based BuyerRequest IDs ==="
rg -i "uuid.*buyer.*request|buyer.*request.*uuid" src/ --type tsLength of output: 657
Add a migration to support the PK type change
You’ve updated the BuyerRequest entity to use a numeric auto-generated id: number, but I don’t see any new migration that converts the existing buyer_requests.id column (currently uuid NOT NULL DEFAULT uuid_generate_v4()) into an integer/serial. Please add a migration that:
- Alters
buyer_requests.idfromUUIDtoINT(orSERIAL) - Creates/configures the sequence for the new integer PK
- Converts existing UUID values into integers (e.g., via a temporary mapping table)
- Updates any foreign-key constraints or references to point at the new integer column
Referencing the original migration at
src/migrations/1751199236860-CreateOfferTable.ts, ensure your new migration runs after it and preserves data integrity.
🤖 Prompt for AI Agents
In src/modules/buyer-requests/entities/buyer-request.entity.ts at lines 29-30,
you changed the primary key type from UUID to a numeric auto-generated integer
but did not add a migration to reflect this. Create a new migration that runs
after 1751199236860-CreateOfferTable.ts to alter the buyer_requests.id column
from UUID to INT or SERIAL, set up the necessary sequence for the integer PK,
convert existing UUID values to integers using a temporary mapping table, and
update all foreign key constraints and references to use the new integer IDs to
maintain data integrity.
| // Update all expired requests to closed status | ||
| const updateResult = await this.buyerRequestRepository.update( | ||
| { | ||
| status: BuyerRequestStatus.OPEN, | ||
| expiresAt: LessThan(new Date()), | ||
| }, | ||
| { status: BuyerRequestStatus.CLOSED } | ||
| ); | ||
|
|
||
| this.logger.log(`Successfully closed ${updateResult.affected || 0} expired buyer requests`); | ||
|
|
||
| // Log details of closed requests for debugging | ||
| expiredRequests.forEach((request) => { | ||
| this.logger.debug( | ||
| `Closed expired request: ID ${request.id}, expired at ${request.expiresAt}` | ||
| ); | ||
| }); | ||
| } catch (error) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential race condition in update operation.
There's a time gap between the find() operation and the update() operation. Expired requests could be processed twice if the cron job runs concurrently or if manual closure happens simultaneously.
Consider using a single atomic update operation:
- // Update all expired requests to closed status
- const updateResult = await this.buyerRequestRepository.update(
- {
- status: BuyerRequestStatus.OPEN,
- expiresAt: LessThan(new Date()),
- },
- { status: BuyerRequestStatus.CLOSED }
- );
-
- this.logger.log(`Successfully closed ${updateResult.affected || 0} expired buyer requests`);
-
- // Log details of closed requests for debugging
- expiredRequests.forEach((request) => {
- this.logger.debug(
- `Closed expired request: ID ${request.id}, expired at ${request.expiresAt}`
- );
- });
+ // Update all expired requests to closed status atomically
+ const updateResult = await this.buyerRequestRepository.update(
+ {
+ status: BuyerRequestStatus.OPEN,
+ expiresAt: LessThan(new Date()),
+ },
+ { status: BuyerRequestStatus.CLOSED }
+ );
+
+ this.logger.log(`Successfully closed ${updateResult.affected || 0} expired buyer requests`);This eliminates the race condition and improves performance.
🤖 Prompt for AI Agents
In src/modules/buyer-requests/services/buyer-request-scheduler.service.ts around
lines 37 to 54, there is a race condition due to separate find and update
operations on expired requests. To fix this, replace the separate find and
update calls with a single atomic update query that updates all expired requests
from OPEN to CLOSED in one operation. This ensures no expired request is
processed twice and improves performance by eliminating the time gap between
operations.
| @Column({ type: "text", nullable: true }) | ||
| description: string | ||
| const results = await queryBuilder.getRawMany(); | ||
| return results.map((r) => r.request_title || r.title); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Clarify the dual column reference in search results.
The line r.request_title || r.title suggests uncertainty about the column name in raw results. This could lead to unexpected behavior if the query structure changes.
Consider using a consistent alias in the SELECT statement:
- .select('request.title')
+ .select('request.title', 'title')Then simplify the mapping to:
- return results.map((r) => r.request_title || r.title);
+ return results.map((r) => r.title);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/modules/buyer-requests/services/buyer-requests.service.ts at line 225,
the code uses a fallback between two column names, request_title and title,
which indicates inconsistency in the query results. To fix this, update the
SELECT statement in the query to use a consistent alias for the title column,
such as AS request_title, so that only one column name is used. Then simplify
the mapping function to just return r.request_title without the fallback.
| @Roles(Role.SELLER) | ||
| create(@Body() createOfferDto: CreateOfferDto, @Request() req: AuthRequest) { | ||
| return this.offersService.create(createOfferDto, String(req.user.id)) | ||
| return this.offersService.create(createOfferDto, Number(req.user.id)); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for ID type conversions.
The Number() conversions could fail if req.user.id is not a valid number, potentially causing runtime errors.
Consider adding validation or using a helper function:
private validateUserId(userId: string | number): number {
const numericId = Number(userId);
if (isNaN(numericId)) {
throw new BadRequestException('Invalid user ID');
}
return numericId;
}Then use it consistently:
- return this.offersService.create(createOfferDto, Number(req.user.id));
+ return this.offersService.create(createOfferDto, this.validateUserId(req.user.id));Also applies to: 108-108, 125-125, 132-132, 152-152, 166-166
🤖 Prompt for AI Agents
In src/modules/offers/controllers/offers.controller.ts at line 49, the code
converts req.user.id to a number without validation, which can cause runtime
errors if the ID is invalid. To fix this, create a helper method that converts
and validates the user ID, throwing a BadRequestException if the conversion
fails. Replace all direct Number() conversions of req.user.id at lines 49, 108,
125, 132, 152, and 166 with calls to this helper method to ensure consistent and
safe ID handling.
| reject(@Param('id') id: string, @Request() req: AuthRequest) { | ||
| return this.offersService.reject(id, String(req.user.id)); | ||
| } |
There was a problem hiding this comment.
Inconsistent ID type conversion in reject method.
Similar to the accept method, this also uses String() conversion instead of Number().
reject(@Param('id') id: string, @Request() req: AuthRequest) {
- return this.offersService.reject(id, String(req.user.id));
+ return this.offersService.reject(id, Number(req.user.id));
}📝 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.
| reject(@Param('id') id: string, @Request() req: AuthRequest) { | |
| return this.offersService.reject(id, String(req.user.id)); | |
| } | |
| reject(@Param('id') id: string, @Request() req: AuthRequest) { | |
| return this.offersService.reject(id, Number(req.user.id)); | |
| } |
🤖 Prompt for AI Agents
In src/modules/offers/controllers/offers.controller.ts around lines 88 to 90,
the reject method converts the user ID to a string using String(req.user.id),
which is inconsistent with the accept method that uses Number conversion. To fix
this, change the conversion to Number(req.user.id) to maintain consistency in ID
type handling across methods.
| @IsNumber() | ||
| @IsNotEmpty() | ||
| buyerRequestId: string | ||
| @Transform(({ value }) => Number.parseInt(value)) | ||
| buyerRequestId: number; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Type change from UUID to number - verify codebase consistency.
The change from @IsUUID() to @IsNumber() with the @Transform decorator correctly handles the type change from string to number for buyerRequestId.
Verify that this type change is consistent across entities, services, and other related files:
🏁 Script executed:
#!/bin/bash
# Check if BuyerRequest entity uses numeric ID
ast-grep --pattern 'class BuyerRequest {
$$$
id: $TYPE;
$$$
}'
# Check for any remaining UUID references to buyerRequestId
rg -A 3 -B 3 "buyerRequestId.*string|@IsUUID.*buyerRequestId"Length of output: 694
Ensure BuyerRequest ID is numeric across layers
The DTO now expects buyerRequestId as a number, but we identified a couple of inconsistencies:
- In
offers.controller.ts, the route parameter is still typed asstringand manually converted. - We couldn’t confirm via AST search that the
BuyerRequestentity’sidproperty is declared as a number.
Please update the following:
• Controller (src/modules/offers/controllers/offers.controller.ts):
Replace
findByBuyerRequest(@Param('buyerRequestId') buyerRequestId: string) {
return this.offersService.findByBuyerRequest(Number(buyerRequestId));
}with
import { ParseIntPipe } from '@nestjs/common';
findByBuyerRequest(
@Param('buyerRequestId', ParseIntPipe) buyerRequestId: number
) {
return this.offersService.findByBuyerRequest(buyerRequestId);
}• Entity (BuyerRequest):
Locate the BuyerRequest entity (e.g. src/modules/buyer-requests/entities/buyer-request.entity.ts) and ensure its id field is declared as:
@PrimaryGeneratedColumn()
id: number;These updates will align the API contract, controller, and database model.
🤖 Prompt for AI Agents
In src/modules/offers/controllers/offers.controller.ts around the method
handling buyerRequestId, replace the current string-typed route parameter and
manual conversion with the NestJS ParseIntPipe to automatically parse and
validate the parameter as a number. Also, in the BuyerRequest entity file
(likely src/modules/buyer-requests/entities/buyer-request.entity.ts), ensure the
id property is declared as a number with the @PrimaryGeneratedColumn() decorator
to match the DTO and controller expectations. These changes will ensure
consistent numeric typing of buyerRequestId across layers.
| file: Express.Multer.File, | ||
| userId: string, | ||
| provider: "cloudinary" | "s3" = "cloudinary", | ||
| userId: number, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify consistency of userId type change across the codebase.
The userId parameter type was changed from string to number. This change should be consistent across all related services, controllers, and DTOs.
Also applies to: 129-129
🏁 Script executed:
#!/bin/bash
# Description: Verify userId type consistency across offer-related files
# Expected: All userId references should be consistently typed as number
echo "Searching for userId parameter types in offer-related files..."
rg -A 2 -B 2 "userId.*:" src/modules/offers/
echo -e "\nSearching for userId in method signatures..."
ast-grep --pattern $'userId: $_'
echo -e "\nChecking DTO files for userId type definitions..."
rg "userId.*:" src/modules/offers/dto/Length of output: 2842
🏁 Script executed:
#!/bin/bash
# Inspect the type of mockBuyerId in offer service tests
echo "Inspecting mockBuyerId in tests..."
rg -A2 -B2 "mockBuyerId" src/modules/offers/tests/offer.service.spec.ts
# Search for any userId references in controllers to ensure consistency
echo -e "\nSearching for userId references in controllers..."
rg -A2 -B2 "userId" src/modules/offers/controllersLength of output: 1712
🏁 Script executed:
#!/bin/bash
# Locate accept and reject method signatures in offers.service.ts
echo "Finding accept signature..."
rg -A2 -B2 "async accept" src/modules/offers/services/offers.service.ts
echo -e "\nFinding reject signature..."
rg -A2 -B2 "async reject" src/modules/offers/services/offers.service.ts
# List controller files and check for userId in method parameters
echo -e "\nListing offer controllers..."
fd -t f . src/modules/offers/controllers
echo -e "\nSearching for userId in controller method signatures..."
rg -A2 -B2 "userId" src/modules/offers/controllersLength of output: 1000
Ensure consistent numeric ID types for accept/reject methods
The accept and reject methods in offers.service.ts still declare their second parameter as string, which conflicts with the rest of the service (and attachment methods) that use number. Please update these signatures and related call sites:
• In src/modules/offers/services/offers.service.ts
- Change
to
async accept(offerId: string, buyerId: string): Promise<Offer> {
async accept(offerId: string, buyerId: number): Promise<Offer> {
- Change
to
async reject(offerId: string, buyerId: string): Promise<Offer> {
async reject(offerId: string, buyerId: number): Promise<Offer> {
• In controllers (offers.controller.ts, offersAdmin.controller.ts)
- Ensure any route handlers that accept a buyer/user ID parse it as
number(e.g.@Param('id') id: string, @Body('buyerId') buyerId: number).
• In tests (offer.service.spec.ts)
- Remove
String(mockBuyerId)wrappers and passmockBuyerId(anumber) directly toservice.acceptandservice.reject.
After applying these changes, verify all DTOs, controllers, and service methods consistently use number for user/buyer IDs.
🤖 Prompt for AI Agents
In src/modules/offers/services/offers.service.ts around lines where accept and
reject methods are defined, change the second parameter type from string to
number for both methods. Then, update all related call sites in
offers.controller.ts and offersAdmin.controller.ts to parse buyer/user IDs as
numbers in route handlers. Also, in offer.service.spec.ts tests, remove any
String() wrappers around mockBuyerId and pass it directly as a number to accept
and reject calls. Finally, verify all DTOs, controllers, and service methods
consistently use number for user/buyer IDs.
| it('should create an offer with price validation', () => { | ||
| const offer = new Offer(); | ||
| offer.requestId = 'test-request-id'; | ||
| offer.buyerRequestId = 123; | ||
| offer.sellerId = 1; | ||
| offer.productId = 123; | ||
| offer.title = 'Test Offer'; | ||
| offer.description = 'Test offer description'; | ||
| offer.price = 100.50; | ||
| offer.price = -50; // Invalid negative price | ||
|
|
||
| expect(offer.productId).toBe(123); | ||
| expect(offer.buyerRequestId).toBe(123); | ||
| expect(offer.sellerId).toBe(1); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix misleading test name and improve validation logic.
The test is named "should create an offer with price validation" but doesn't actually perform any validation - it just sets a negative price and checks basic properties.
- it('should create an offer with price validation', () => {
+ it('should allow creation with invalid price for testing', () => {
const offer = new Offer();
offer.buyerRequestId = 123;
offer.sellerId = 1;
offer.title = 'Test Offer';
offer.description = 'Test offer description';
offer.price = -50; // Invalid negative price
expect(offer.buyerRequestId).toBe(123);
expect(offer.sellerId).toBe(1);
+ expect(offer.price).toBe(-50); // Verify the invalid price is set
});Or add a separate test that actually validates price constraints if using class-validator decorators.
📝 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.
| it('should create an offer with price validation', () => { | |
| const offer = new Offer(); | |
| offer.requestId = 'test-request-id'; | |
| offer.buyerRequestId = 123; | |
| offer.sellerId = 1; | |
| offer.productId = 123; | |
| offer.title = 'Test Offer'; | |
| offer.description = 'Test offer description'; | |
| offer.price = 100.50; | |
| offer.price = -50; // Invalid negative price | |
| expect(offer.productId).toBe(123); | |
| expect(offer.buyerRequestId).toBe(123); | |
| expect(offer.sellerId).toBe(1); | |
| }); | |
| it('should allow creation with invalid price for testing', () => { | |
| const offer = new Offer(); | |
| offer.buyerRequestId = 123; | |
| offer.sellerId = 1; | |
| offer.title = 'Test Offer'; | |
| offer.description = 'Test offer description'; | |
| offer.price = -50; // Invalid negative price | |
| expect(offer.buyerRequestId).toBe(123); | |
| expect(offer.sellerId).toBe(1); | |
| expect(offer.price).toBe(-50); // Verify the invalid price is set | |
| }); |
🤖 Prompt for AI Agents
In src/modules/offers/tests/offer.entity.spec.ts around lines 25 to 35, the test
name suggests it validates the price but it only sets a negative price without
asserting validation behavior. Rename the test to reflect what it actually tests
or add assertions to check that the price validation fails as expected. If using
class-validator decorators, create a separate test that triggers validation and
asserts errors for invalid price values.
2 participants
Overview
Closes #103
This pull request implements a comprehensive auto-expire system for BuyerRequests, automatically closing expired requests through scheduled background tasks while maintaining manual control capabilities.
Changes Made
BuyerRequest Module Enhancements
Core Service Updates:
BuyerRequestsServicewith manual close functionalitycloseRequestmethod with proper ownership validation and status checksController Improvements:
PATCH /:id/closeendpoint for manual buyer request closureModule Configuration:
BuyerRequestSchedulerServiceinto the BuyerRequests moduleAuto-expire Implementation
Background Scheduler Service:
BuyerRequestSchedulerServicewith automated cron job execution@Cron('0 */10 * * * *')decorator for 10-minute interval checkshandleExpiredRequestsmethod for automatic expiration processingManual Expiration Control:
closeExpiredRequestsmethod for on-demand executionmain.tsto process expired requests on application bootApplication-Level Integration:
@nestjs/schedulepackage dependency for cron job supportScheduleModule.forRoot()inAppModuleTechnical Implementation Details
Automated Processing
expiresAt< current time ANDstatus!= 'closed'Manual Controls
PATCH /api/v1/buyer-requests/:id/closeError Handling
Test Coverage
All functionality has been thoroughly tested with comprehensive test suites:
BuyerRequestSchedulerService Tests
Core Service and Controller Tests
Total Test Coverage: 36 tests passed covering all new functionality including:
Test Success
Files Modified
package.json- Added @nestjs/schedule dependencypackage-lock.json- Updated lockfile with new dependenciessrc/app.module.ts- Configured ScheduleModule integrationsrc/main.ts- Added startup expired request cleanupsrc/modules/buyer-requests/buyer-requests.module.ts- Integrated scheduler servicesrc/modules/buyer-requests/controllers/buyer-requests.controller.ts- Added close endpointsrc/modules/buyer-requests/services/buyer-requests.service.ts- Added manual close functionalityFiles Added
src/modules/buyer-requests/services/buyer-request-scheduler.service.ts- Auto-expire schedulersrc/modules/buyer-requests/tests/buyer-request-scheduler.service.spec.ts- Scheduler testsAPI Documentation
New Endpoint
Close Buyer Request
Response:
{ "id": 1, "title": "Request Title", "status": "closed", "userId": 1, "expiresAt": "2024-12-31T23:59:59Z", "createdAt": "2024-01-01T00:00:00Z", "updatedAt": "2024-01-15T10:30:00Z" }Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests
Chores