Phase 1 - Service Layer Refactoring Complete#9
Conversation
There was a problem hiding this comment.
Pull request overview
This PR completes Phase 1 of the service layer refactoring by introducing a BaseService<TEntity> class with common CRUD operations and creating specialized service classes for each entity. The refactoring improves code organization, reduces duplication, and establishes consistent patterns for entity management with automatic audit tracking and organization-based multi-tenancy.
Key changes:
- Introduced
BaseService<TEntity>providing CRUD operations, organization isolation, soft delete support, and automatic audit field management - Created 13 specialized service classes (PropertyService, TenantService, LeaseService, DocumentService, InvoiceService, PaymentService, MaintenanceService, InspectionService, TourService, ProspectiveTenantService, RentalApplicationService, ScreeningService, LeaseOfferService)
- Updated all Razor components and existing services to use the new refactored services
- Migrated test framework from NUnit to xUnit and renamed test project
Reviewed changes
Copilot reviewed 91 out of 97 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| Aquiis.SimpleStart/Core/Services/BaseService.cs | New abstract base service providing common CRUD operations with organization isolation and audit tracking |
| Aquiis.SimpleStart/Core/Interfaces/ICalendarEventService.cs | New interface for calendar event management |
| Aquiis.SimpleStart/Core/Interfaces/IAuditable.cs | New interface defining audit tracking properties |
| Aquiis.SimpleStart/Core/Entities/BaseModel.cs | Updated to implement IAuditable interface |
| Aquiis.SimpleStart/Application/Services/*.cs | New specialized service classes for each entity type |
| Aquiis.SimpleStart/Program.cs | Updated DI registration for new services |
| Aquiis.SimpleStart/Features/PropertyManagement/**/*.razor | Updated to inject and use new specialized services |
| Aquiis.Tests/Core/Validation/GuidValidationAttributeTests.cs | Migrated from NUnit to xUnit |
| Aquiis.sln | Renamed test project reference |
Comments suppressed due to low confidence (4)
Aquiis.SimpleStart/Core/Entities/BaseModel.cs:1
- The
LastModifiedByproperty initialization was changed fromstring.Emptytonull. This is inconsistent with theCreatedByproperty which is initialized tostring.Empty. For consistency and to avoid potential null reference issues, both should use the same default value.
Aquiis.SimpleStart/Infrastructure/Data/ApplicationDbContext.cs:1 - Suppressing Entity Framework warnings is a temporary workaround. The TODO indicates this needs proper resolution. Consider addressing the bidirectional relationship issue by reviewing navigation property configurations or using
.Ignore()on one side of the relationship in the model configuration.
Aquiis.SimpleStart/Application/Services/TourService.cs:1 - The calendar event deletion functionality is incomplete. Since the
ICalendarEventServiceinterface includes aDeleteEventAsyncmethod, this TODO should be implemented to properly clean up calendar events when tours are deleted.
Aquiis.SimpleStart/Features/PropertyManagement/Calendar.razor:1 - When marking a maintenance request as complete, the
CompletedOndate is set if it's null. However, if it already has a value, that value is preserved even though the status is being changed to Completed. Consider always settingCompletedOn = DateTime.UtcNowwhen marking as complete to reflect the actual completion time.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| errors.Add("Payment amount must be greater than zero."); | ||
| } | ||
|
|
||
| if (entity.PaidOn > DateTime.UtcNow.Date.AddDays(1)) |
There was a problem hiding this comment.
The validation allows payment dates up to 1 day in the future, which could lead to data integrity issues. Payments should not be dated in the future. Change to if (entity.PaidOn.Date > DateTime.UtcNow.Date) to prevent any future-dated payments.
| if (entity.PaidOn > DateTime.UtcNow.Date.AddDays(1)) | |
| if (entity.PaidOn.Date > DateTime.UtcNow.Date) |
| } | ||
|
|
||
| // Validate dates | ||
| if (entity.RequestedOn > DateTime.Today) |
There was a problem hiding this comment.
The validation uses DateTime.Today while other date validations in the method use .Date. For consistency, use entity.RequestedOn.Date > DateTime.Today or consistently use DateTime.UtcNow.Date throughout the validation method.
| if (entity.RequestedOn > DateTime.Today) | |
| if (entity.RequestedOn.Date > DateTime.Today) |
| // TODO: Delete associated calendar event when interface method is available | ||
| // await _calendarEventService.DeleteEventBySourceAsync(id, nameof(Inspection)); |
There was a problem hiding this comment.
Similar to tours, the calendar event deletion is incomplete for inspections. Implement the deletion using the available DeleteEventAsync method from ICalendarEventService.
|
Rejected. Successful CI Build and Test is required. |
Phase 1 - Service layer refactoring complete per Roadmap.