Bug: fixed edit Major not working

[constantine0621]

No description provided.

[claude]

---

Code Review: PR #103 — Bug: fixed edit Major not working

Summary

This PR fixes a bug where editing a Major entity silently corrupted the faculty relationship: the original MapStruct updateEntityFromDto @MappingTarget method set only faculty.id on the entity, producing a shallow proxy that Hibernate could not resolve on update. The fix replaces the mapper call with an explicit FacultyRepository lookup and direct setter calls. The root bug fix is correct, but the implementation introduces an inlined error message that bypasses ErrorConstants (Critical) and injects FacultyRepository directly into MajorService, crossing a feature boundary at the repository layer (Important).

Critical (must fix)

1. [src/main/java/org/unilab/uniplan/major/MajorService.java:83] Inlined error string "Faculty not found" is passed to ResourceNotFoundException instead of using ErrorConstants.FACULTY_NOT_FOUND.getMessage(String.valueOf(dto.facultyId())). This bypasses the project's consistent error message format, breaks i18n key resolution, and diverges from every other not-found throw in the same file. The fix is one line: use FACULTY_NOT_FOUND.getMessage(String.valueOf(dto.facultyId())) — FACULTY_NOT_FOUND is already defined in ErrorConstants. Add it to the static import alongside MAJOR_NOT_FOUND.

Important (should fix)

1. [src/main/java/org/unilab/uniplan/major/MajorService.java:19-21] FacultyRepository is injected directly into MajorService, crossing a feature boundary at the repository layer. The architecture rules state that cross-feature access should go through the Service layer ("A Service calling another feature's Service is a smell — usually the Facade should orchestrate both"; accessing another feature's Repository is the same problem one level lower). The correct fix is to inject FacultyService and call findById(id) from there, or move this validation to a future Facade.

2. [src/test/java/org/unilab/uniplan/major/MajorServiceTest.java:36] FacultyRepository facultyRepository; is missing the private modifier. All other @Mock fields in the class are private. Change to private FacultyRepository facultyRepository;.

Minor (could fix)

1. [src/main/java/org/unilab/uniplan/major/MajorService.java:83] The local variable Faculty faculty = ... should be final Faculty faculty = ... to follow the final style convention applied throughout the file.

Suggestions (max 3, does not block merge)

1. [src/main/java/org/unilab/uniplan/major/MajorMapper.java] The deleted updateEntityFromDto method was the root cause (shallow faculty reference via MapStruct @MappingTarget). If this pattern is needed in the future, a @BeforeMapping hook with a proper entity lookup would be cleaner than manual setters in the Service. Can be addressed when major/ is migrated to the Facade pattern.

2. [src/main/java/org/unilab/uniplan/major/MajorService.java:80-86] The manual entity.setFaculty(faculty); entity.setMajorName(dto.majorName()); block will silently miss any future fields added to MajorDto. A comment noting that new Major fields must also be added here manually would prevent silent regressions.

Pre-existing (out of scope)

1. [src/main/java/org/unilab/uniplan/major/MajorService.java] The major/ feature uses the legacy Controller → Service pattern with @Transactional on the Service and ResourceNotFoundException thrown from the Service. This is intentional pre-existing architecture, not yet migrated to Facade. Noted for completeness only.

Praise

The bug identification is accurate — the old MapStruct @MappingTarget method created a shallow faculty reference with only id set, which Hibernate would treat as a detached entity reference, leading to constraint violations or stale data on update. Replacing it with an explicit repository fetch is the correct mechanical fix. The new test case updateMajorShouldThrowResourceNotFoundExceptionIfFacultyNotFound is a good addition that covers the previously missing failure path.

---

[claude]

Important: FacultyRepository is injected directly into MajorService, crossing a feature boundary at the repository layer. The architecture rules state cross-feature access should go through the Service layer (accessing another feature's Repository from a Service is the same problem as calling another feature's Service — "usually the Facade should orchestrate both").

The preferred fix is to inject FacultyService instead of FacultyRepository, and call facultyService.findById(dto.facultyId()). This keeps the cross-feature interaction at the Service-to-Service level. Longer term, this validation belongs in a MajorWebFacade when the major/ feature is migrated.

[claude]

Important: Missing private visibility modifier on this @Mock field. All other mock fields in this class are private. Change to:

@Mock
private FacultyRepository facultyRepository;