-
Notifications
You must be signed in to change notification settings - Fork 0
Fix same-day reservation blocking, invalid date display, and add comprehensive test coverage #291
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Draft
Copilot
wants to merge
8
commits into
main
Choose a base branch
from
copilot/update-reserving-logic
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
e84a54b
Initial plan
Copilot 80ef800
Fix reservation logic to allow same-day bookings and fix date display
Copilot 5c4ef28
Fixed Invalid-Date format
78e5da5
Fix: Solar Issue
bcb4182
Add test cases for same-day reservation fix
Copilot 5b518ed
Add comprehensive test coverage for date-utils.ts (100%)
Copilot e575094
Fix unused imports in date-utils.test.ts
426446c
Fix: Cloud sonar issue
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,206 @@ | ||
| # Test Documentation for Reservation Logic Fix | ||
|
|
||
| ## Overview | ||
| This document describes the test cases added to validate the fix for same-day reservation bookings and date display issues. | ||
|
|
||
| ## Backend Tests (Domain Layer) | ||
|
|
||
| ### File: `packages/sthrift/domain/src/domain/contexts/reservation-request/reservation-request/reservation-request.test.ts` | ||
|
|
||
| #### New Test Scenario: "Setting reservation period start to today" | ||
|
|
||
| **Purpose**: Validates that users can create reservations starting on the current day. | ||
|
|
||
| **Test Steps**: | ||
| 1. **Given**: A new ReservationRequest aggregate being created | ||
| 2. **When**: The reservationPeriodStart is set to today's date (noon) | ||
| 3. **Then**: The reservation request should be created successfully | ||
|
|
||
| **Implementation Details**: | ||
| ```typescript | ||
| Scenario('Setting reservation period start to today', ({ Given, When, Then }) => { | ||
| Given('a new ReservationRequest aggregate being created', () => {}); | ||
| When('I set the reservationPeriodStart to today\'s date', () => { | ||
| const today = new Date(); | ||
| today.setHours(12, 0, 0, 0); // Set to noon today | ||
| const endDate = new Date(Date.now() + 86_400_000 * 7); // One week from now | ||
| try { | ||
| aggregate = ReservationRequest.getNewInstance( | ||
| { ...baseProps, reservationPeriodStart: today, reservationPeriodEnd: endDate }, | ||
| toStateEnum('REQUESTED'), | ||
| listing, | ||
| reserver, | ||
| today, | ||
| endDate, | ||
| passport | ||
| ); | ||
| } catch (e) { | ||
| error = e; | ||
| } | ||
| }); | ||
| Then('the reservation request should be created successfully with today\'s date', () => { | ||
| expect(error).toBeUndefined(); | ||
| expect(aggregate).toBeDefined(); | ||
| expect(aggregate.reservationPeriodStart).toBeInstanceOf(Date); | ||
| }); | ||
| }); | ||
| ``` | ||
|
|
||
| **What This Tests**: | ||
| - Ensures the date validation logic correctly compares against start of day (midnight) rather than current timestamp | ||
| - Verifies that a reservation can be created for today at any time (e.g., noon) | ||
| - Confirms no error is thrown when using today's date | ||
| - Validates the aggregate is properly instantiated | ||
|
|
||
| **Related Code Change**: | ||
| The test validates the fix in `reservation-request.ts` where date validation was changed from: | ||
| ```typescript | ||
| if (value.getTime() < Date.now()) | ||
| ``` | ||
| to: | ||
| ```typescript | ||
| const startOfToday = new Date(); | ||
| startOfToday.setHours(0, 0, 0, 0); | ||
| if (value.getTime() < startOfToday.getTime()) | ||
| ``` | ||
|
|
||
| ### Feature File Update | ||
|
|
||
| **File**: `packages/sthrift/domain/src/domain/contexts/reservation-request/reservation-request/features/reservation-request.feature` | ||
|
|
||
| Added new scenario: | ||
| ```gherkin | ||
| Scenario: Setting reservation period start to today | ||
| Given a new ReservationRequest aggregate being created | ||
| When I set the reservationPeriodStart to today's date | ||
| Then the reservation request should be created successfully with today's date | ||
| ``` | ||
|
|
||
| ## Frontend Tests (UI Layer) | ||
|
|
||
| ### Current State | ||
| The UI layer (`apps/ui-sharethrift`) does not currently have test files for the modified components: | ||
| - `reservation-card.tsx` | ||
| - `reservations-table.tsx` | ||
|
|
||
| ### Recommended Frontend Tests (For Future Implementation) | ||
|
|
||
| When setting up UI tests, the following test cases should be added: | ||
|
|
||
| #### Test Case 1: Date Display in Reservation Card | ||
| **Component**: `reservation-card.tsx` | ||
|
|
||
| **Test**: "Should display formatted dates correctly" | ||
| ```typescript | ||
| describe('ReservationCard', () => { | ||
| it('should convert BigInt timestamps to readable dates', () => { | ||
| const mockReservation = { | ||
| id: '1', | ||
| reservationPeriodStart: '1700000000000', // BigInt as string | ||
| reservationPeriodEnd: '1700086400000', | ||
| // ... other fields | ||
| }; | ||
|
|
||
| render(<ReservationCard reservation={mockReservation} />); | ||
|
|
||
| // Should not show "Invalid Date" | ||
| expect(screen.queryByText(/Invalid Date/i)).not.toBeInTheDocument(); | ||
|
|
||
| // Should show formatted dates | ||
| expect(screen.getByText(/11\/15\/2023/)).toBeInTheDocument(); // Example format | ||
| }); | ||
| }); | ||
| ``` | ||
|
|
||
| #### Test Case 2: Date Display in Reservations Table | ||
| **Component**: `reservations-table.tsx` | ||
|
|
||
| **Test**: "Should render reservation periods correctly in table" | ||
| ```typescript | ||
| describe('ReservationsTable', () => { | ||
| it('should format reservation period dates correctly', () => { | ||
| const mockReservations = [{ | ||
| id: '1', | ||
| reservationPeriodStart: '1700000000000', | ||
| reservationPeriodEnd: '1700086400000', | ||
| // ... other fields | ||
| }]; | ||
|
|
||
| render(<ReservationsTable reservations={mockReservations} />); | ||
|
|
||
| // Should not show "Invalid Date" | ||
| expect(screen.queryByText(/Invalid Date/i)).not.toBeInTheDocument(); | ||
| }); | ||
| }); | ||
| ``` | ||
|
|
||
| ### Why Frontend Tests Are Not Included | ||
| 1. The UI package does not have a `test` script defined in package.json | ||
| 2. No existing test files pattern in `apps/ui-sharethrift/src/components` | ||
| 3. The project uses Storybook for component testing rather than unit tests | ||
| 4. Vitest is configured but not actively used for UI component tests | ||
|
|
||
| ### Validation Approach | ||
| The frontend date display fix has been validated through: | ||
| 1. **Code Review**: Verifying the `Number()` conversion is correctly applied | ||
| 2. **Linting**: Passing biome lint checks | ||
| 3. **TypeScript Compilation**: No type errors | ||
| 4. **Build Process**: Successfully builds without errors | ||
|
|
||
| ## Test Execution | ||
|
|
||
| ### Running Backend Tests | ||
| ```bash | ||
| cd /home/runner/work/sharethrift/sharethrift/packages/sthrift/domain | ||
| npm test -- reservation-request.test.ts | ||
| ``` | ||
|
|
||
| ### Expected Results | ||
| - All existing tests continue to pass (117 tests) | ||
| - New test "Setting reservation period start to today" passes | ||
| - Test coverage includes: | ||
| - Past date validation (still blocks yesterday) | ||
| - Today's date validation (now allows today) | ||
| - Future date validation (continues to allow) | ||
|
|
||
| ## Summary | ||
|
|
||
| ### Backend Tests ✅ | ||
| - Added comprehensive test case for same-day reservations | ||
| - Test validates the core bug fix in date validation logic | ||
| - Follows existing BDD/Cucumber style testing pattern | ||
| - Integrated into existing test suite | ||
|
|
||
| ### Frontend Tests ⚠️ | ||
| - No test files exist in UI package currently | ||
| - Provided documentation for future test implementation | ||
| - Code changes validated through linting and type checking | ||
| - Storybook can be used for visual testing | ||
|
|
||
| ## Related Files | ||
|
|
||
| ### Modified Files | ||
| 1. `packages/sthrift/domain/src/domain/contexts/reservation-request/reservation-request/reservation-request.ts` | ||
| 2. `apps/ui-sharethrift/src/components/layouts/home/my-reservations/components/reservation-card.tsx` | ||
| 3. `apps/ui-sharethrift/src/components/layouts/home/my-reservations/components/reservations-table.tsx` | ||
|
|
||
| ### Test Files | ||
| 1. `packages/sthrift/domain/src/domain/contexts/reservation-request/reservation-request/reservation-request.test.ts` (Modified) | ||
| 2. `packages/sthrift/domain/src/domain/contexts/reservation-request/reservation-request/features/reservation-request.feature` (Modified) | ||
|
|
||
| ## Future Improvements | ||
|
|
||
| 1. **Set up UI Testing Infrastructure** | ||
| - Add test script to `apps/ui-sharethrift/package.json` | ||
| - Configure React Testing Library | ||
| - Create test files for reservation components | ||
|
|
||
| 2. **Integration Tests** | ||
| - Test end-to-end flow of creating same-day reservations | ||
| - Verify date display in actual rendered components | ||
|
|
||
| 3. **Additional Edge Cases** | ||
| - Test timezone handling | ||
| - Test daylight saving time transitions | ||
| - Test leap year dates | ||
| - Test reservation spanning multiple days starting today |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,3 +23,4 @@ dist-ssr | |
| *.sln | ||
| *.sw? | ||
| .turbo | ||
| coverage | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using
parseGraphQLDateTime()on ISO string values fromdayjsToGraphQLDate()is unnecessary and adds an extra conversion step. TheformData.sharingPeriodarray contains ISO strings fromdayjsToGraphQLDate(), which can be directly converted to Date objects usingnew Date().The
parseGraphQLDateTime()function is designed to handle GraphQL DateTime scalars that might be BigInt/string timestamps from the server, but here we're dealing with ISO strings we just created on the client. This adds unnecessary complexity.Consider simplifying to: