feat: add delete a facility functionality per ticket 683 - #1199
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughSummary by CodeRabbit
WalkthroughFacility deletion now checks related records, exposes deletion eligibility, enforces department-admin authorization, returns blocker details, and adds frontend confirmation and conflict handling. Integration tests cover deletion outcomes, associations, roles, and listing state. ChangesFacility deletion guards
Merge Risk: ⚪ Minimal · up to The change adds facility deletion while preventing deletion when associations exist; no actionable merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/src/database/facilities.go`:
- Around line 30-43: The can_delete calculation in the facilities query adds
seven correlated EXISTS checks that execute for every row produced by the
existing joins. Restructure the query around the can_delete logic, such as
evaluating these checks after facility-level aggregation or through a
facility-keyed precomputed relation, so each facility’s checks run once while
preserving the existing deletion criteria and aggregate counts.
In `@backend/src/handlers/server.go`:
- Around line 609-620: Replace the near-duplicate
writeFacilityDeleteConflictResponse and writeDeleteConflictResponse
implementations with one generic conflict-response writer parameterized by the
blocker payload type. Reuse models.Resource[T] while preserving the existing
JSON headers, 409 status, message/data fields, and error wrapping behavior;
update all callers to use the generic helper.
In `@backend/tests/integration/facility_delete_guard_test.go`:
- Around line 71-75: Update the facility deletion assertions in the scoped count
checks and the corresponding block around the second count to use Unscoped(),
then verify the facility record remains present and has a populated deleted_at
value. Preserve the existing facility ID filtering and ensure the assertions
distinguish soft deletion from hard deletion.
- Around line 190-195: Update the response assertions near the byID map
construction to explicitly verify that both empty.ID and withResident.ID are
present in the map before checking CanDelete. Capture each lookup’s presence
result and assert it, then retain the existing eligibility assertions.
- Around line 153-157: Update the login-activity fixture creation in the
facility deletion guard test to assert that suite.env.DB.Create returns no
error. Ensure the test fails immediately when the LoginActivity insert cannot be
created, while preserving the existing fixture values and subsequent
non-blocking deletion assertion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b3c66253-8ed0-451f-86a6-264013bc699e
📒 Files selected for processing (10)
backend/src/database/delete_guards.gobackend/src/database/facilities.gobackend/src/handlers/auth.gobackend/src/handlers/facilities_handler.gobackend/src/handlers/server.gobackend/src/models/delete_guards.gobackend/src/models/facilities.gobackend/tests/integration/facility_delete_guard_test.gofrontend/src/pages/admin/FacilityManagement.tsxfrontend/src/types/facility.ts
| const query = `SELECT | ||
| f.id, f.name, f.timezone, f.created_at, f.updated_at, | ||
| COUNT(DISTINCT CASE WHEN p.is_active = true AND p.archived_at IS NULL AND p.deleted_at IS NULL THEN p.id END) AS active_programs, | ||
| COUNT(DISTINCT CASE WHEN pc.status IN ('Active') AND pc.archived_at IS NULL AND pc.deleted_at IS NULL THEN pc.id END) AS active_classes, | ||
| COUNT(DISTINCT CASE WHEN u.role = 'student' AND u.deleted_at IS NULL AND u.deactivated_at IS NULL THEN u.id END) AS total_residents | ||
| COUNT(DISTINCT CASE WHEN u.role = 'student' AND u.deleted_at IS NULL AND u.deactivated_at IS NULL THEN u.id END) AS total_residents, | ||
| NOT ( | ||
| EXISTS(SELECT 1 FROM users WHERE facility_id = f.id AND deleted_at IS NULL) | ||
| OR EXISTS(SELECT 1 FROM program_classes WHERE facility_id = f.id AND deleted_at IS NULL) | ||
| OR EXISTS(SELECT 1 FROM facilities_programs WHERE facility_id = f.id AND deleted_at IS NULL) | ||
| OR EXISTS(SELECT 1 FROM rooms WHERE facility_id = f.id AND deleted_at IS NULL) | ||
| OR EXISTS(SELECT 1 FROM open_content_activities WHERE facility_id = f.id) | ||
| OR EXISTS(SELECT 1 FROM open_content_favorites WHERE facility_id = f.id) | ||
| OR EXISTS(SELECT 1 FROM user_account_history WHERE facility_id = f.id) | ||
| ) AS can_delete |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial
Correlated EXISTS subqueries stack on top of existing join multiplication.
The pre-existing LEFT JOINs to facilities_programs/programs/program_classes/users already multiply rows before GROUP BY; the 7 new EXISTS(...) subqueries are now evaluated per multiplied row rather than once per facility. Fine at current scale (a handful of facilities), but worth keeping in mind if facility/user counts grow substantially.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/src/database/facilities.go` around lines 30 - 43, The can_delete
calculation in the facilities query adds seven correlated EXISTS checks that
execute for every row produced by the existing joins. Restructure the query
around the can_delete logic, such as evaluating these checks after
facility-level aggregation or through a facility-keyed precomputed relation, so
each facility’s checks run once while preserving the existing deletion criteria
and aggregate counts.
| func writeFacilityDeleteConflictResponse(w http.ResponseWriter, message string, blockers models.FacilityBlockingChildren) error { | ||
| w.Header().Set("Content-Type", "application/json") | ||
| w.WriteHeader(http.StatusConflict) | ||
| resp := models.Resource[models.FacilityBlockingChildren]{ | ||
| Message: message, | ||
| Data: blockers, | ||
| } | ||
| if err := json.NewEncoder(w).Encode(resp); err != nil { | ||
| return newResponseServiceError(err) | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Near-duplicate of writeDeleteConflictResponse; consider making it generic.
This function is identical to the existing writeDeleteConflictResponse except for the payload type. Since models.Resource[T] is already generic, a single generic writer would remove the duplication.
♻️ Suggested refactor
-func writeDeleteConflictResponse(w http.ResponseWriter, message string, blockers models.DeleteBlockingChildren) error {
- w.Header().Set("Content-Type", "application/json")
- w.WriteHeader(http.StatusConflict)
- resp := models.Resource[models.DeleteBlockingChildren]{
- Message: message,
- Data: blockers,
- }
- if err := json.NewEncoder(w).Encode(resp); err != nil {
- return newResponseServiceError(err)
- }
- return nil
-}
-
-func writeFacilityDeleteConflictResponse(w http.ResponseWriter, message string, blockers models.FacilityBlockingChildren) error {
- w.Header().Set("Content-Type", "application/json")
- w.WriteHeader(http.StatusConflict)
- resp := models.Resource[models.FacilityBlockingChildren]{
- Message: message,
- Data: blockers,
- }
- if err := json.NewEncoder(w).Encode(resp); err != nil {
- return newResponseServiceError(err)
- }
- return nil
-}
+func writeDeleteConflictResponse[T any](w http.ResponseWriter, message string, blockers T) error {
+ w.Header().Set("Content-Type", "application/json")
+ w.WriteHeader(http.StatusConflict)
+ resp := models.Resource[T]{
+ Message: message,
+ Data: blockers,
+ }
+ if err := json.NewEncoder(w).Encode(resp); err != nil {
+ return newResponseServiceError(err)
+ }
+ return nil
+}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/src/handlers/server.go` around lines 609 - 620, Replace the
near-duplicate writeFacilityDeleteConflictResponse and
writeDeleteConflictResponse implementations with one generic conflict-response
writer parameterized by the blocker payload type. Reuse models.Resource[T] while
preserving the existing JSON headers, 409 status, message/data fields, and error
wrapping behavior; update all callers to use the generic helper.
There was a problem hiding this comment.
This might be worth, just to stay as DRY as possible, not a deal breaker though.
| var live int64 | ||
| suite.env.DB.Model(&models.Facility{}). | ||
| Where("id = ? AND deleted_at IS NULL", facility.ID). | ||
| Count(&live) | ||
| suite.Equal(int64(0), live, "facility should be soft-deleted") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the soft-delete contract, not only disappearance.
Both scoped counts also pass after a hard delete. Query Unscoped() and verify the facility still exists with deleted_at populated.
Also applies to: 165-169
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/tests/integration/facility_delete_guard_test.go` around lines 71 -
75, Update the facility deletion assertions in the scoped count checks and the
corresponding block around the second count to use Unscoped(), then verify the
facility record remains present and has a populated deleted_at value. Preserve
the existing facility ID filtering and ensure the assertions distinguish soft
deletion from hard deletion.
| suite.env.DB.Create(&models.LoginActivity{ | ||
| TimeInterval: time.Now(), | ||
| FacilityID: facility.ID, | ||
| TotalLogins: 1, | ||
| }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Fail when the login-activity fixture cannot be created.
Ignoring Create(...).Error lets this test pass with an empty facility, so it would not prove that login activity is non-blocking. Require a nil error from the insert.
Proposed fix
- suite.env.DB.Create(&models.LoginActivity{
+ suite.Require().NoError(suite.env.DB.Create(&models.LoginActivity{
TimeInterval: time.Now(),
FacilityID: facility.ID,
TotalLogins: 1,
- })
+ }).Error)📝 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.
| suite.env.DB.Create(&models.LoginActivity{ | |
| TimeInterval: time.Now(), | |
| FacilityID: facility.ID, | |
| TotalLogins: 1, | |
| }) | |
| suite.Require().NoError(suite.env.DB.Create(&models.LoginActivity{ | |
| TimeInterval: time.Now(), | |
| FacilityID: facility.ID, | |
| TotalLogins: 1, | |
| }).Error) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/tests/integration/facility_delete_guard_test.go` around lines 153 -
157, Update the login-activity fixture creation in the facility deletion guard
test to assert that suite.env.DB.Create returns no error. Ensure the test fails
immediately when the LoginActivity insert cannot be created, while preserving
the existing fixture values and subsequent non-blocking deletion assertion.
| byID := map[uint]models.FacilityWithStats{} | ||
| for _, f := range resp.GetData() { | ||
| byID[f.ID] = f | ||
| } | ||
| suite.True(byID[empty.ID].CanDelete, "empty facility should be deletable") | ||
| suite.False(byID[withResident.ID].CanDelete, "facility with a resident should not be deletable") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert that both facilities are present in the response.
A missing withResident entry yields the zero-value struct, whose CanDelete is false; the blocked-facility assertion would pass despite the list omitting it. Require map presence before checking eligibility.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/tests/integration/facility_delete_guard_test.go` around lines 190 -
195, Update the response assertions near the byID map construction to explicitly
verify that both empty.ID and withResident.ID are present in the map before
checking CanDelete. Capture each lookup’s presence result and assert it, then
retain the existing eligibility assertions.
CK-7vn
left a comment
CK-7vn
left a comment
There was a problem hiding this comment.
Worked great! Couple trivial cleanup things that you could do, or not do, whatever works for you my friend.
| * when it is not, what is blocking the delete. | ||
| */ | ||
| func (srv *Server) handleGetFacilityDeleteCheck(w http.ResponseWriter, r *http.Request, log sLog) error { | ||
| if !userCanManageFacilities(r) { |
There was a problem hiding this comment.
We can delete this because this is a newDeptAdminRoute and it attaches that resolver that checks claims for canSwitchFacility. This doesn't break anything, but it is a double check.
| */ | ||
| func (srv *Server) handleDeleteFacility(w http.ResponseWriter, r *http.Request, log sLog) error { | ||
| if !userIsSystemAdmin(r) { | ||
| if !userCanManageFacilities(r) { |
There was a problem hiding this comment.
Should be able to delete this one as well.
| newAdminRoute("GET /api/facilities/{id}", srv.handleShowFacility), | ||
| newDeptAdminRoute("POST /api/facilities", srv.handleCreateFacility), | ||
| newSystemAdminRoute("DELETE /api/facilities/{id}", srv.handleDeleteFacility), | ||
| newDeptAdminRoute("GET /api/facilities/{id}/delete-check", srv.handleGetFacilityDeleteCheck), |
There was a problem hiding this comment.
Are we calling/using delete-check anywhere?
| */ | ||
| func (srv *Server) handleDeleteFacility(w http.ResponseWriter, r *http.Request, log sLog) error { | ||
| if !userIsSystemAdmin(r) { | ||
| if !userCanManageFacilities(r) { |
There was a problem hiding this comment.
Should be able to delete this one as well.
2 participants
Pre-Submission PR Checklist
Description of the change
Added the functionality for being able to delete a facility. If the facility has any associations the facility will not be allowed to be deleted.
Screenshot(s)