Jack O'Reilly - #15
Conversation
…tion RE-001 & RE-002 - Added ReservationController with methods for booking and managing reservations. - Introduced ReservationValidator for validating reservation data (dates, email, room number). - Created interfaces for repositories (IReservationRepository, IRoomRepository, IGuestRepository) to adhere to SOLID principles. - Implemented in-memory SQLite database for testing repository functionality. - Added unit tests for ReservationController, ReservationRepository, and ReservationValidator. - Enhanced error handling in the booking process with user-friendly messages. - Updated UI components to reflect validation requirements and improved user experience. - Configured .gitignore to exclude build artifacts and added changelog for project documentation. Co-authored-by: Copilot <copilot@github.com>
…m controllers Co-authored-by: Copilot <copilot@github.com>
Added authorization policy on api. Removed IsNotStaff as it can be handled by the authorize attribute. Standardized schema casing. Added unit tests. Co-authored-by: Copilot <copilot@github.com>
…lidation and service integration. Added new helper for sqllite boolean conversion Added a service layer for checkin as it touches both reservation and room logic. Keep controller clean. Added logic for already checked in rooms. Updated backend unit tests Co-authored-by: Copilot <copilot@github.com>
There was a problem hiding this comment.
Pull request overview
Implements staff authentication + staff dashboard/check-in flow, and completes reservation booking backend behavior (validation + persistence) with accompanying .NET solution/test scaffolding.
Changes:
- Added staff cookie authentication, staff endpoints, and a staff dashboard UI with check-in.
- Implemented reservation validation + conflict detection and wired up real reservation booking API calls from the UI.
- Introduced repository/service interfaces and added unit/integration tests for key backend behavior.
Reviewed changes
Copilot reviewed 38 out of 40 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| ui/src/utils/toasts.tsx | Adds an error-toast hook. |
| ui/src/components/ErrorToast.tsx | New error toast component. |
| ui/src/staff/StaffPage.tsx | New staff dashboard UI with filtering + check-in. |
| ui/src/router.tsx | Adds /staff route. |
| ui/src/LandingPage.tsx | Adds staff login prompt + navigation to staff dashboard. |
| ui/src/reservations/api.ts | Implements booking POST, staff auth/reservations/check-in API calls, updates schemas. |
| ui/src/reservations/ReservationPage.tsx | Adds booking error handling; updates room schema usage. |
| ui/src/reservations/BookingDetailsModal.tsx | Adds client-side email validation, min date, and submit disabling. |
| ui/package-lock.json | Bumps UI package version. |
| reservations-interview.sln | Adds Visual Studio solution file. |
| global.json | Pins .NET SDK version. |
| changelog.md | Adds release notes for new functionality. |
| api/api.csproj | Adds InternalsVisibleTo for test project. |
| api/Program.cs | Registers DI/services for repos, validators, auth; configures JSON naming. |
| api/Controllers/StaffController.cs | Adds cookie-based staff login, staff reservations endpoint, and check-in endpoint. |
| api/Controllers/ReservationController.cs | Adds validation/room+guest checks; returns 409 on conflicts; switches to interfaces. |
| api/Controllers/RoomController.cs | Switches to repository interface; marks controller as ApiController. |
| api/Controllers/GuestController.cs | Switches to repository interface; marks controller as ApiController. |
| api/Repositories/ReservationRepository.cs | Implements create + conflict detection + check-in transaction + upcoming reservations. |
| api/Repositories/RoomRepository.cs | Implements IRoomRepository. |
| api/Repositories/GuestRepository.cs | Implements IGuestRepository. |
| api/Repositories/Interfaces/IReservationRepository.cs | New reservation repository abstraction. |
| api/Repositories/Interfaces/IRoomRepository.cs | New room repository abstraction. |
| api/Repositories/Interfaces/IGuestRepository.cs | New guest repository abstraction. |
| api/Services/ICheckInService.cs | New check-in service abstraction. |
| api/Services/CheckInService.cs | Implements check-in domain logic (email confirm, dirty-room check, transaction call). |
| api/Validators/Interfaces/IReservationValidator.cs | New reservation validator abstraction. |
| api/Validators/ReservationValidator.cs | Implements booking validation rules (dates, duration, email, room format). |
| api/Helpers/GuidTypeHandler.cs | Adds Dapper type handler for Guid <-> SQLite. |
| api/Helpers/SqlLiteBooleanHandler.cs | Adds Dapper type handler for bool <-> SQLite integer storage. |
| api/Db/Setup.cs | Adjusts schema to use INTEGER for boolean columns. |
| api/Models/Room.cs | Minor enum formatting change. |
| api.tests/api.tests.csproj | Adds xUnit/Moq + sqlite dependency; references api project. |
| api.tests/Validators/ReservationValidatorTests.cs | Adds unit tests for validator rules. |
| api.tests/Services/CheckInServiceTests.cs | Adds unit tests for check-in service behavior. |
| api.tests/Repositories/ReservationRepositoryTests.cs | Adds sqlite-backed tests for reservation persistence/conflict checks. |
| api.tests/Controllers/StaffControllerTests.cs | Adds controller tests for staff endpoints. |
| api.tests/Controllers/ReservationContollerTests.cs | Adds controller tests for reservation booking paths. |
| .gitignore | Adds ignores for .NET build artifacts. |
| .config/dotnet-tools.json | Adds csharpier tool configuration. |
Files not reviewed (1)
- ui/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| var reservation = await _reservationRepo.GetReservation(reservationId); | ||
| if (reservation == null) | ||
| return (false, "Reservation not found."); |
There was a problem hiding this comment.
IReservationRepository.GetReservation() is non-nullable and the concrete ReservationRepository throws NotFoundException when a reservation is missing. ProcessCheckIn currently assumes GetReservation can return null, so a missing reservation will bubble an exception and likely return a 500 instead of the intended (false, "Reservation not found."). Catch NotFoundException (or change the repository contract) and return the appropriate error tuple.
| var reservation = await _reservationRepo.GetReservation(reservationId); | |
| if (reservation == null) | |
| return (false, "Reservation not found."); | |
| Reservation reservation; | |
| try | |
| { | |
| reservation = await _reservationRepo.GetReservation(reservationId); | |
| } | |
| catch (Exception ex) when (ex.GetType().Name == "NotFoundException") | |
| { | |
| return (false, "Reservation not found."); | |
| } |
| const [isSubmitting, setIsSubmitting] = useState(false); | ||
| const showProcessingToast = useShowInfoToast("Processing booking..."); | ||
| const showNoInfoToast = useShowInfoToast("Missing email or dates."); | ||
| const showNoInfoToast = useShowInfoToast("Missing valid email or dates."); | ||
|
|
||
| const emailRegex = /^[^@\s]+@[^@\s]+\.[^@\s]+$/; | ||
| const isValidEmail = emailRegex.test(email); | ||
| const isEmailError = email.length > 0 && !isValidEmail; | ||
| const isFormValid = isValidEmail && dateRange[0] !== null && dateRange[1] !== null; | ||
|
|
||
| function handleSubmit(evt: React.MouseEvent<HTMLButtonElement>) { | ||
| if (!email || !dateRange[0] || !dateRange[1]) { | ||
| if (!isFormValid) { | ||
| showNoInfoToast(); | ||
| evt.preventDefault(); | ||
| return false; | ||
| } | ||
|
|
||
| if (isSubmitting) { | ||
| evt.preventDefault(); | ||
| return false; | ||
| } | ||
|
|
||
| setIsSubmitting(true); | ||
| showProcessingToast(); | ||
| onSubmit({ | ||
| RoomNumber: roomNumber, | ||
| GuestEmail: email, | ||
| Start: fromDateStringToIso(dateRange[0]), | ||
| End: fromDateStringToIso(dateRange[1]), | ||
| Start: dateRange[0], | ||
| End: dateRange[1], | ||
| }); |
There was a problem hiding this comment.
isSubmitting is set to true on submit but never reset. If the booking request fails (or if the dialog stays mounted when closed), the form can remain permanently disabled on subsequent opens. Consider resetting isSubmitting when the request completes (success or failure), or when the dialog closes/opens.
| <Dialog.Close> | ||
| <Button size="3" color="mint" mt="4" onClick={handleSubmit}> | ||
| <Button | ||
| size="3" | ||
| color="mint" | ||
| mt="4" | ||
| onClick={handleSubmit} | ||
| disabled={isSubmitting || !isFormValid} | ||
| > | ||
| Reserve | ||
| </Button> | ||
| </Dialog.Close> |
There was a problem hiding this comment.
The submit button is wrapped in Dialog.Close, so the modal closes immediately on click even though onSubmit triggers an async booking request. This makes it hard to surface/resolve booking errors in-context. Consider closing the dialog only after the booking succeeds (and keeping it open on failure).
| ### Fixed | ||
| - **Dapper Type Mapping:** Resolved a `System.InvalidCastException` by implementing custom `TypeHandler`s for `Guid` and `Boolean` types to bridge the gap between SQLite storage formats and C# models. | ||
| - **Security Vulnerabilities:** Patched a "TODO" regarding weak authentication by enforcing the `[Authorize]` attribute on staff endpoints and ensuring proper middleware ordering in `Program.cs`. | ||
| - **Casing Mismatches:** Aligned Zod frontend schemas with backend JSON serialization to correctly parse camelCase properties returned by the API. |
There was a problem hiding this comment.
Changelog: it says frontend schemas were aligned for camelCase properties, but the API sets JsonSerializerOptions.PropertyNamingPolicy = null (PascalCase). Please update this entry to avoid documenting the opposite of what the code does.
| - **Casing Mismatches:** Aligned Zod frontend schemas with backend JSON serialization to correctly parse camelCase properties returned by the API. | |
| - **Casing Mismatches:** Aligned Zod frontend schemas with backend JSON serialization to correctly parse PascalCase properties returned by the API. |
| { | ||
| options.Cookie.Name = "StaffAccess"; | ||
| options.Cookie.HttpOnly = true; | ||
| options.Cookie.SecurePolicy = CookieSecurePolicy.Always; |
There was a problem hiding this comment.
Cookie auth is configured with CookieSecurePolicy.Always, but the provided Caddy setup serves the app over plain HTTP on localhost. In that setup the browser will refuse to set/send the cookie, so staff auth will never stick. Consider SameAsRequest (or environment-specific Always only in production behind HTTPS).
| options.Cookie.SecurePolicy = CookieSecurePolicy.Always; | |
| options.Cookie.SecurePolicy = builder.Environment.IsDevelopment() | |
| ? CookieSecurePolicy.SameAsRequest | |
| : CookieSecurePolicy.Always; |
| export async function checkInGuest(id: string, email: string) { | ||
| return ky.post(`api/staff/checkin/${id}`, { json: email }).json(); | ||
| } |
There was a problem hiding this comment.
checkInGuest calls .json() on the response, but the backend POST /staff/checkin/{id} currently returns Ok() with no body. Parsing an empty body as JSON will throw, so the UI will treat successful check-ins as failures. Either have the API return a JSON payload (e.g., {}) or change the client to not call .json() (e.g., use .text() / no body consumption).
| var authServiceMock = new Mock<IAuthenticationService>(); | ||
| var serviceProviderMock = new Mock<IServiceProvider>(); | ||
| serviceProviderMock | ||
| .Setup(s => s.GetService(typeof(IAuthenticationService))) | ||
| .Returns(authServiceMock.Object); | ||
| _controller.ControllerContext.HttpContext.RequestServices = serviceProviderMock.Object; | ||
|
|
||
| // Act | ||
| var result = await _controller.CheckCode("pass"); | ||
|
|
There was a problem hiding this comment.
CheckCode awaits HttpContext.SignOutAsync/SignInAsync. In this test, IAuthenticationService.SignOutAsync/SignInAsync are not set up on the mock, so Moq will return null tasks and the controller will throw when awaiting. Configure those methods to return Task.CompletedTask (and optionally verify they were called).
| ## [2026-04-25] | ||
|
|
||
| ### Added | ||
| - **Staff Authentication System:** Implemented a secure authentication flow using ASP.NET Core Identity/Cookie middleware, replacing basic `HttpOnly` flags with signed, encrypted authentication tickets. |
There was a problem hiding this comment.
Changelog: it states staff auth uses "ASP.NET Core Identity", but the code registers cookie auth with custom claims and does not add Identity services/packages. Please adjust the wording so the changelog matches the implementation.
| - **Staff Authentication System:** Implemented a secure authentication flow using ASP.NET Core Identity/Cookie middleware, replacing basic `HttpOnly` flags with signed, encrypted authentication tickets. | |
| - **Staff Authentication System:** Implemented a secure authentication flow using ASP.NET Core cookie authentication with custom claims, replacing basic `HttpOnly` flags with signed, encrypted authentication tickets. |
| Services.AddSingleton(_ => new SqliteConnection(connectionString)); | ||
| Services.AddSingleton<IDbConnection>(sp => sp.GetRequiredService<SqliteConnection>()); | ||
| Services.AddSingleton<GuestRepository>(); | ||
| Services.AddSingleton<RoomRepository>(); | ||
| Services.AddSingleton<ReservationRepository>(); | ||
| Services.AddMvc(opt => | ||
| { | ||
| opt.EnableEndpointRouting = false; | ||
| }); | ||
| Services.AddScoped<IReservationRepository, ReservationRepository>(); | ||
| Services.AddScoped<IRoomRepository, RoomRepository>(); | ||
| Services.AddScoped<IGuestRepository, GuestRepository>(); | ||
| builder.Services.AddScoped<ICheckInService, CheckInService>(); |
There was a problem hiding this comment.
SqliteConnection/IDbConnection are registered as singletons but repositories are scoped. This makes all requests share the same connection (not thread-safe for SQLite) and also Setup.EnsureDb resolves SqliteConnection in a using block (Db/Setup.cs), which will dispose the singleton at startup and break later DB operations. Prefer registering the connection as scoped/transient (or using a scoped factory) so each scope/request gets its own connection that can be safely disposed.
| Services | ||
| .AddAuthentication("StaffAuth") | ||
| .AddCookie( | ||
| "StaffAuth", | ||
| options => | ||
| { | ||
| options.Cookie.Name = "StaffAccess"; | ||
| options.Cookie.HttpOnly = true; | ||
| options.Cookie.SecurePolicy = CookieSecurePolicy.Always; | ||
| options.Cookie.SameSite = SameSiteMode.Strict; | ||
| options.LoginPath = "/staff/login"; | ||
| options.Events.OnRedirectToLogin = context => | ||
| { | ||
| context.Response.StatusCode = StatusCodes.Status401Unauthorized; | ||
| return Task.CompletedTask; | ||
| }; | ||
| } | ||
| ); | ||
|
|
||
| Services.AddAuthorization(); |
There was a problem hiding this comment.
After adding cookie authentication/authorization services, the request pipeline still doesn't call UseAuthentication()/UseAuthorization() before MVC. Without authentication middleware the StaffAuth cookie typically won't be evaluated on requests, causing [Authorize] endpoints to behave incorrectly. Add the authentication/authorization middleware in the pipeline (before UseMvc / endpoint execution).
Jack O'Reilly
Focused on backend implementations first, followed by UI integration. Total development time was 3 hours (Started: 1:45 PM, Final commit: 4:45 PM). Leveraged GitHub Copilot for initial architectural planning before writing code, then executed the core logic and debugging manually. Implemented backend unit tests; frontend tests were omitted due to time constraints. Task RE-004 presented the most significant challenge (debugging the CheckedIn status not persisting), which was ultimately resolved by implementing a custom SQLite type helper.
Next Steps: Complete RE-005 and introduce stricter validation to existing endpoints (e.g., room creation checks).
Task Report
The commit log is an accurate representation of my time spent from first commit, and it shows I did the MVP of RE#001 and RE#002. Added a custom error toast to follow the existing success toast behaviour. This took about 40 mins. After this initial commit I took some to cleanup leftover code and fix the repository interfaces.
Next up I done the MVP for RE#003, where I added the Authorize attribute. When staff enters the 'pass' access key, they will be granted the StaffAccess cookie. When using the cookie on the login, check or other staff endpoints marked with the Authorize property any non staff user would receive 401. When debugging locally I noticed that the cookie would not be removed between sessions so I set the login endpoint to force a logout to ensure theres always a fresh, accurate login. This took about an hour.
The next commit was where I was implementing the check in system and also the marking rooms as dirty, as the two seemed to go hand in hand. While completing this I noticed there was a mismatch in the schema casing when adding a new endpoint vs the existing ones, so I resolved the naming to always be PascalCase. The staff controller was also growing larger and larger as the check in execution touched both rooms and reservations. Implemented a service class which was responsible for calling both reservations and rooms repos. Then implemented the atomic logic on ReservationRepository to only mark a room as dirty if the check in was successful, otherwise rollback the transaction.
I faced the biggest hurdle here due to Sqlite data type mapping of the CheckedIn column. Downloaded DB Browser for SQLite to help with debugging and eventually added the SqliteBooleanHandler. This took me an hour and a half. Final changes were cleanup of logs etc.
I may have gone slightly over time
Notes