Feature/104 add soft delete on faculty; Bug/105 Delete Major doesn't work

[PIPetkova19]

Upgraded the soft deletion mechanism across the application. Instead of using the custom SoftDeletableEntity base class combined with explicit @SQLDelete and @SqlRestriction annotations, we are now using @softdelete annotation.

Changes Made
Completely removed SoftDeletableEntity base class.
Removed @SQLDelete and @SqlRestriction annotations from entities.
Applied the @softdelete annotation to Faculty and Major and Course entities.
Adjusted Major and Faculty to comply with the new soft delete approach.
Bug Fix: Fixed an issue with the soft delete implementation in the Major entity to ensure it marks records correctly without throwing unexpected errors.

[claude]

Test comment from automated review - checking gh CLI works

[claude]

---

Code Review: PR 106 — Feature/104 add soft delete on faculty; Bug/105 Delete Major doesn't work

Summary

This PR replaces the hand-rolled soft-delete mechanism on Major with Hibernate's built-in @softdelete annotation, extends the same mechanism to Faculty and Course, and restructures the Liquibase changelog directory from a YYYY/MM/ hierarchy to a flat timestamp-prefix layout. The architectural direction is sound, but the execution has three correctness defects (including a schema-management regression that will break existing deployments) and several convention violations that must be addressed before merge.

Critical (must fix)

1. [src/main/resources/db/changelog/db.changelog-master.yaml:3] Renaming pre-existing changelog file paths will re-run already-applied changeSets on any existing database. Liquibase identifies applied changeSets by the triple (id, author, filePath). Renaming 2025/08/0001-baseline-init-changelog.yaml to 20250801144000-baseline-init-changelog.yaml (and 0002/0003 likewise) changes the filePath component. On any database that has already executed these changeSets, Liquibase will treat them as new and attempt to re-run them, causing object-already-exists / unique-constraint errors at startup. The files must either keep their original paths, or the renames must be accompanied by a changeLogSync runbook step. This is a data-safety regression.

2. [src/main/java/org/unilab/uniplan/major/Major.java:6,19,20] Unused imports left behind. FetchType was added (line 6) but is never referenced on any field. SQLDelete (line 19) and SQLRestriction (line 20) class-level annotations were removed in this PR, but their import statements were not cleaned up. Unused imports violate the project rule of specific imports, no wildcards, and no unused imports.

3. [src/main/java/org/unilab/uniplan/student/Student.java:21] FetchType.LAZY regressed to FetchType.EAGER on Student.course. The rule is FetchType.LAZY on every relationship. Changing to EAGER forces an extra SELECT on every Student load regardless of whether Course is needed, creating an N+1 footgun with no justification in the diff. Revert to FetchType.LAZY.

Important (should fix)

1. [All 8 new changelogs 20260626143800-20260626144500] Missing objectQuotingStrategy: QUOTE_ONLY_RESERVED_WORDS on every new changeSet. The rules file is explicit: this attribute is required on every changeSet. None of the 8 new changeSets carry it.

2. [All 8 new changelogs] Missing rollback: blocks. Every new changelog must declare rollback: with inverse operations, or rollback: [] with a comment explaining irreversibility. For addColumn changeSets the inverse is dropColumn; for dropColumn changeSets (IDs 9-10) use rollback: [] with a justifying comment; for modifyDataType the inverse is the original type. None of the 8 changeSets provide rollback declarations.

3. [src/main/resources/db/changelog/20260626143800-alter-faculty-for-soft-delete.yaml:3] Non-unique integer changeSet IDs (4-11 authored by petya) are fragile. Liquibase duplicate-detection uses (id, author, filepath). Any future changelog from a different file reusing the same integer ID and author will collide. The project's established convention is timestamp-prefixed IDs (e.g. 1755084381762-1). All new changeSet IDs should follow this pattern.

4. [Changelogs 20260626143800 through 20260626144400] Intermediate migration artifacts add unnecessary schema noise. deleted_at on faculty is added (changeSet 4), type-modified (changeSet 5), then dropped (changeSet 9). On major, deleted_at is type-modified (changeSet 6) then dropped (changeSet 10). Consolidate into a minimal set: add is_deleted columns and drop deleted_at columns, no intermediate modifyDataType on a column that will be dropped.

5. [src/main/java/org/unilab/uniplan/major/MajorRepository.java:12] findAllByFaculty(Faculty faculty) is added but never called. MajorService continues to use findAllByFacultyId(UUID). Dead repository methods bloat the API surface. Remove it or wire it up.

Minor (could fix)

1. [All 8 new changelog YAML files] Missing trailing newline. Every new changelog file ends without a trailing newline. .editorconfig enforces insert_final_newline = true for YAML files.

Suggestions (max 3, does not block merge)

1. [Liquibase changelog structure] Consider collapsing the 8 new changeSets into fewer well-named ones: (a) add is_deleted boolean NOT NULL DEFAULT false to faculty, (b) same for major, (c) same for course, (d) drop major.deleted_at. The current trail reflects iterative exploration but complicates production migration reasoning and rollbacks.

2. [src/main/java/org/unilab/uniplan/major/Major.java:42] @onetomany could declare fetch = FetchType.LAZY explicitly. Although @onetomany defaults to LAZY, the project rules call for explicit declaration on every relationship.

3. [src/main/java/org/unilab/uniplan/major/Major.java] Verify CascadeType.REMOVE interaction with @softdelete. Deleting a Major via JPA cascades a JPA DELETE to its Course children. With @softdelete on both entities, confirm that cascade fires a soft-delete on courses rather than a hard delete.

Pre-existing (out of scope)

1. src/main/java/org/unilab/uniplan/major/Major.java — @manytoone on faculty missing explicit fetch = FetchType.LAZY. Present in main before this branch diverged.
2. src/main/java/org/unilab/uniplan/faculty/dto/FacultyResponseDto.java — validation annotations on a response DTO. Pre-existing in main.
3. src/main/java/org/unilab/uniplan/major/MajorController.java — Controller injects MajorMapper and MajorService directly (old layering, no Facade). Pre-existing; this PR does not migrate the feature.

Praise

• Replacing @SQLDelete / @SQLRestriction / SoftDeletableEntity with Hibernate's native @softdelete is the correct direction — it removes bespoke infrastructure in favour of a well-tested framework feature and makes soft-delete semantics declarative and uniform across Faculty, Major, and Course.
• The FacultyWebFacade correctly owns @transactional, the Controller delegates only to the Facade, and Optional is properly unwrapped with ResourceNotFoundException.
• The FacultyServiceTest is clean Mockito-only with good Given-When-Then blank-line structure and the new methodName_shouldBehavior_whenCondition naming convention.
• The driver-class-name correction in application.yaml is a legitimate fix.
• @softdelete is applied consistently on Course in addition to Faculty and Major.

---

[claude]

Code Review: PR #106

Summary
This PR replaces the hand-rolled @SQLDelete/@SQLRestriction/SoftDeletableEntity approach with Hibernate's native @softdelete annotation on Faculty, Major, and Course, and reorganises Liquibase changelogs to a flat, timestamp-prefixed naming scheme. The direction is correct and the core implementation is idiomatic, but four changelog files referenced in db.changelog-master.yaml do not exist — the app will not boot in production until resolved. There is also a FetchType.EAGER regression on Student and missing rollback blocks on new addColumn changelogs that must be fixed before merge.

Critical (must fix)

db.changelog-master.yaml — four missing changelog files. Four files registered in db.changelog-master.yaml do not exist in the repository: 20250801144200-alter-major-for-soft-delete.yaml, 20260626143800-alter-faculty-for-soft-delete.yaml, 20260626143900-alter-faculty-type-column.yaml, and 20260626144000-alter-major-type-column.yaml. Liquibase resolves includes at startup — these cause a ChangeLogParseException and the application will not boot in production. Each missing file must be created or its include entry removed from the master.

Important (should fix)

1. Student.java:24 — FetchType.EAGER regression. This PR changed the course association from LAZY to EAGER. Project rules require FetchType.LAZY on every relationship. Every Student query will now eagerly fetch Course. Revert to FetchType.LAZY.

2. 20260626144100-add-column-is-deleted-to-faculty.yaml and 20260626144200-add-column-is-deleted-to-major.yaml — missing rollback blocks. Project rules require an inverse rollback (dropColumn) or rollback: [] with a comment. Add the inverse dropColumn rollback to each.

3. 20260626144500-add-column-is-deleted-to-course.yaml — missing rollback block. The addColumn changeset for course.is_deleted has no rollback block. Add an inverse dropColumn rollback.

Minor (could fix)

1. Major.java:3 — unused CascadeType import. CascadeType was added to imports in this diff but never referenced. Remove it.

2. FacultyResponseDto.java:3-4 — dead imports. NotNull and Size imports are unused after removing all validation annotations from this record. Remove them.

Suggestions (does not block merge)

1. The preConditions block with onFail: MARK_RAN is good for idempotency, but document the rationale in a comment so future readers know why MARK_RAN was chosen over HALT.

2. In 20250801144100-seed-university-data-from-csv.yaml, only changeset 1 received objectQuotingStrategy. Changesets 2-16 are still missing it — align them with the project convention.

3. The dropColumn changelog rollback restores deleted_at as TIMESTAMP WITH TIME ZONE, but the original baseline declared it TIMESTAMP (no timezone). Harmless but worth aligning.

Pre-existing (out of scope)

Major.java, Course.java, Faculty.java — @manytoone associations lacking FetchType.LAZY — pre-existing on main. FacultyWebFacade.java — createFaculty logs post-save rather than pre, differing from the reference pattern — pre-existing. Seed changesets 2-16 missing objectQuotingStrategy — carried over from main.

Praise

The migration from the hand-rolled @SQLDelete/@SQLRestriction/SoftDeletableEntity approach to Hibernate's native @softdelete annotation is a clean, idiomatic improvement — it removes bespoke SQL strings and a custom base class in favour of a well-maintained Hibernate feature. The changelog reorganisation (flat directory, timestamp prefix, consistent IDs) improves legibility. The rollback blocks on the dropColumn changelogs are thorough. The FacultyServiceTest is well-structured, using AssertJ and proper Given-When-Then separation.