LNK-4730: Added Tenant Soft Delete - #1399
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the
📝 WalkthroughWalkthroughThis pull request implements a soft delete feature for facilities across the full stack. It adds an Changes
Sequence Diagram(s)sequenceDiagram
participant User as User / UI
participant Ctrl as FacilityController
participant Mgr as FacilityManager
participant DB as Database (Facility)
User->>Ctrl: DELETE /soft/{facilityId}
Ctrl->>Mgr: SoftDeleteAsync(facilityId)
Mgr->>DB: Retrieve Facility by facilityId
alt Facility exists
Mgr->>DB: Set IsDeleted = true, ModifyDate = now
DB->>DB: Update Facility
Mgr->>Ctrl: Return facilityId
Ctrl->>User: HTTP 200 (success)
else Facility not found
Mgr->>Ctrl: Error logged & wrapped
Ctrl->>User: HTTP 404 / Error response
end
sequenceDiagram
participant User as User / UI
participant Svc as TenantService
participant Ctrl as FacilityController
participant Qry as FacilityQueries
participant DB as Database
User->>Svc: listFacilities(..., showDeleted=true)
Svc->>Ctrl: GET /facilities?includeDeleted=true
Ctrl->>Qry: PagedSearchAsync(..., includeDeleted=true)
Qry->>DB: Query Facilities
alt includeDeleted = true
DB->>DB: Return all facilities (IsDeleted = true or false)
else includeDeleted = false
DB->>DB: Filter WHERE IsDeleted = false
end
DB->>Qry: Facilities with IsDeleted property
Qry->>Ctrl: PagedConfigModel<FacilityModel>
Ctrl->>Svc: HTTP 200 with results
Svc->>User: Display facilities in table
User->>User: Show deleted facilities dimmed if showDeleted enabled
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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. Comment |
arianamihailescu
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
DotNet/Tenant/Controllers/FacilityController.cs (1)
150-165: IsDeleted filter only applies when search parameter is provided.The
IsDeleted = falsefilter (lines 159-162) is only set inside theif (!string.IsNullOrEmpty(search))block. Whensearchis empty/null butincludeDeleted=false, the filter won't be applied and deleted facilities may be returned.Suggested fix — apply filter regardless of search parameter
public async Task<IActionResult> GetFacilityList([FromQuery] string? search, bool includeDeleted = false) { try { FacilitySearchModel searchModel = new FacilitySearchModel(); + if (!includeDeleted) + { + searchModel.IsDeleted = false; + } + if (!string.IsNullOrEmpty(search)) { searchModel.FacilityName = search; searchModel.FacilityNameContains = true; - if (!includeDeleted) - { - searchModel.IsDeleted = false; - } } var facilities = await _facilityQueries.SearchAsync(searchModel, HttpContext.RequestAborted);
🤖 Fix all issues with AI agents
In `@DotNet/Tenant/Business/Managers/FacilityManager.cs`:
- Around line 302-348: SoftDeleteAsync is missing an audit event after
successfully marking the facility as soft deleted; after the SaveChangesAsync
call in FacilityManager.SoftDeleteAsync, invoke the same audit creation flow
used in DeleteAsync (call _createAuditEventCommand.Execute with appropriate
parameters) to record an AuditEventType.Delete (or a dedicated SoftDelete type
if available), include facility id and relevant metadata (service name via
TenantConstants.ServiceName, action, resource id, and actor/context), and ensure
this call is placed inside the try block after persistence so failures to write
the audit bubble up; add/adjust unit tests to cover the successful soft-delete
path and verify the command is executed.
In `@DotNet/Tenant/Business/Queries/FacilityQueries.cs`:
- Around line 23-25: The interface default for sortBy in the PagedSearchAsync
signature (Task<PagedConfigModel<FacilityModel>> PagedSearchAsync) is
"FacilityName" but the concrete FacilityQueries implementation defaults to
"FacilityId"; update the interface signature's sortBy default to "FacilityId" so
both IFacilityQueries and FacilityQueries use the same default and callers get
consistent ordering behavior.
In
`@Web/Admin.UI/src/app/components/tenant/tenant-dashboard/tenant-dashboard.component.ts`:
- Around line 188-229: The soft-delete flow in onSoftDeleteFacility currently
only calls tenantService.softDeleteFacilityConfiguration and leaves report
deletion commented out; update the sequential deletion sequence (in
onSoftDeleteFacility) to include
safeDelete(this.reportService.deleteReports(facilityId)) (or the appropriate
soft-delete/hide method on reportService) in the concat so reports are processed
after configuration, keeping the existing safeDelete wrapper and error/404
handling; if report deletion is intended to be handled by the backend cascade,
remove the commented call and add a short comment referencing the backend
contract instead.
🧹 Nitpick comments (5)
Web/Admin.UI/src/app/components/tenant/tenant-dashboard/tenant-dashboard.component.scss (1)
33-35: Avoid global opacity on deleted rows.
Row-level opacity can reduce contrast and dim buttons/icons; consider styling text/background instead.♻️ Suggested adjustment
.deleted-row { - opacity: 0.5; + color: rgba(0, 0, 0, 0.6); + background-color: rgba(0, 0, 0, 0.04); }Web/Admin.UI/src/app/services/gateway/tenant/tenant.service.ts (1)
70-78: Log message could distinguish soft delete from hard delete.The log message
Delete Facility configuration.(line 73) is identical to the one indeleteFacilityConfiguration(line 63). Consider making it more specific to aid debugging.Suggested improvement
softDeleteFacilityConfiguration(facilityId: string): Observable<IEntityDeletedResponse> { return this.http.delete<IEntityDeletedResponse>(`${this.appConfigService.config?.baseApiUrl}/facility/soft/${facilityId}`) .pipe( - tap(_ => console.log(`Delete Facility configuration.`)), + tap(_ => console.log(`Soft delete Facility configuration.`)), catchError((error) => { return this.errorHandler.handleError(error); }) ) }DotNet/Tenant/Controllers/FacilityController.cs (1)
387-421: Consider disabling jobs instead of deleting them for soft delete.The comment on line 416 raises a valid consideration. For soft delete semantics where data is preserved for potential restoration, deleting the jobs permanently may not be ideal. If a facility is ever "undeleted," the jobs would need to be recreated.
Would you like me to help design an approach to pause/disable jobs instead of deleting them, allowing for future restoration?
Web/Admin.UI/src/app/components/tenant/tenant-dashboard/tenant-dashboard.component.html (1)
41-52: Consider adding aria-label or title for disabled facility IDs.When
row.isDeletedis true, the facility ID is shown as disabled text without any indication of why it's disabled. Adding a title or aria-label could improve user experience.Optional enhancement
<ng-template `#disabledText`> - <span class="disabled-text"> + <span class="disabled-text" title="Facility has been deleted"> {{ row.facilityId }} </span> </ng-template>DotNet/Tenant/Business/Queries/FacilityQueries.cs (1)
69-100: Add xUnit coverage for new filter/sort branches.Please add unit tests that exercise includeDeleted true/false, FacilityNameContains true/false, and SortOrder ascending/descending. Use Moq (or equivalent) for repository dependencies and avoid network activity.
As per coding guidelines, suggest unit tests using XUnit when If/Else or Switch/Case blocks are introduced or modified.
2 participants
🛠️ Description of Changes
Added Tenant Soft Delete
🧪 Testing Performed
Tested locally
🧑🔬 Unit Testing
📓 Documentation Updated
Please update any relevant sections in the project documentation that were impacted by the changes in the PR.
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.