Facade migration T4/ Migrate Student to Facade pattern

[constantine0621]

#85 - Student facade

[PIPetkova19]

Add tests for student mapper

[DjesikaV]

Shouldn't StudentValidator validate the StudentRequestDto instead of the Student entity?

[DjesikaV]

fix StudentValidator

[PIPetkova19]

The naming convention we agreed on is findById_shouldReturnEntity_whenFacultyExists

[claude]

Code Review: PR 121 - Facade migration T4 / Migrate Student to Facade pattern

---

SUMMARY

This PR migrates the Student feature from the legacy Controller->Service pattern to the new Facade architecture (Controller->WebFacade->Service->Repository). The layering restructuring is solid: the Controller delegates exclusively to StudentWebFacade, the Service is stripped of @transactional and DTO logic, and separate test classes cover Facade, Service, Validator, and Mapper in isolation. Two issues undercut the otherwise clean migration: the create (and update) endpoints now return empty bodies, breaking the REST API contract, and @NotNull was silently dropped from all @PathVariable parameters during the rewrite.

---

CRITICAL (must fix)

None.

---

IMPORTANT (should fix)

1. [StudentWebFacade.java:23-28 / StudentController.java:39-41] POST /students now returns 201 Created with an empty body. Before this migration the endpoint returned the created StudentResponseDto, giving clients the server-assigned UUID. After migration clients receive no body and cannot learn the ID. Fix: change createStudent to return StudentResponseDto (return studentMapper.toResponseDto(student)) and the Controller to return ResponseEntity.status(HttpStatus.CREATED).body(...). This matches the reference pattern in backend.md Appendix A.

2. [StudentController.java:51, 73, 87] @NotNull was removed from all three @PathVariable UUID id parameters during the migration (getStudentById, updateStudent, deleteStudent). The old controller carried @PathVariable @NotNull final UUID id on every path param; the rewrite silently dropped the annotation. Per backend.md Appendix A, path variables must carry @PathVariable @NotNull.

---

MINOR (could fix)

1. [StudentValidator.java:17] Validator method is named "validate" rather than a specific verb like validateForCreate / validateForUpdate. The rules require: "Methods are verbs that throw on failure (e.g. validateForCreate(request))."

2. [StudentValidator.java:14, 18, 20] Three style violations in the newly added file: (a) local variable "UUID id = request.courseId();" is missing "final"; (b) class declaration "StudentValidator{" is missing a space before "{"; (c) "if (!courseRepository.existsById(id)){" is missing a space before "{". Coding conventions require final on all local variables and standard Java brace spacing.

3. [StudentMapperTest.java:16] Wrong import: "import static org.junit.jupiter.api.AssertionsKt.assertNull" references the Kotlin extension class. Correct: "import static org.junit.jupiter.api.Assertions.assertNull".

4. [StudentMapperTest.java / StudentValidatorTest.java / StudentWebFacadeTest.java] Test method names use uppercase _Should and _When segments (e.g. toEntity_ShouldMapFieldsCorrectly). Project convention from backend-test.md: all-lowercase methodName_shouldExpectedBehavior_whenCondition.

---

SUGGESTIONS (max 3, does not block merge)

1. [StudentWebFacadeTest.java] Add a test for getAllStudents - currently the test class covers all five facade methods except this one. A minimal test verifying that studentService.getAll() is called and its result passed to studentMapper.toResponseDtoList() completes coverage.

2. [StudentWebFacade.java:27, 48, 54] Log statements fire after side effects with past tense ("created student with ID: {}"). The reference pattern logs at the start of the method before work is done (e.g. log.info("Creating student {}", request.firstName())).

3. [StudentMapperTest.java:505-519] toEntity_ShouldMapFieldsCorrectly already asserts assertNull(result.getId()); toEntity_ShouldIgnoreId asserts the same thing. The second test is redundant - remove it or fold into the first.

---

PRE-EXISTING (out of scope)

1. [StudentRequestDto.java] courseId lacks @NotNull. The new StudentValidator calls request.courseId().toString() in the error path - NPE risk if null. Not touched in this diff.

2. [CategoryController.java] CategoryController (already on main) has the same missing @NotNull on path variables and the same void-create pattern; those issues pre-date this branch.

---

PRAISE

• Facade, Service, and Validator are correctly structured: @transactional lives exclusively on the WebFacade, the Service is a clean thin wrapper with no DTO logic, and the Validator calls courseRepository.existsById before any mutation.
• Separate test classes for each layer (StudentWebFacadeTest, StudentServiceTest, StudentValidatorTest, StudentMapperTest) follow the project convention precisely.
• StudentWebFacadeTest correctly uses InOrder to verify that studentValidator.validate() is called before the mapper and service in both createStudent and updateStudent - exactly the orchestration-order verification the rules require.
• Service tests were thoughtfully trimmed to repository-delegation verification only, correctly removing exception-type and DTO assertions that belong on the Facade.
• StudentMapper cleanly simplified: the intermediate StudentDto round-trip is gone, mapping now goes directly StudentRequestDto -> Student -> StudentResponseDto, and toEntity correctly ignores id so @PrePersist can set it.

[claude]

IMPORTANT: createStudent returns void, breaking the REST API contract. Clients get 201 Created with no body and cannot learn the server-assigned UUID. Fix: change the return type to StudentResponseDto, return studentMapper.toResponseDto(student), and have the Controller return ResponseEntity.status(HttpStatus.CREATED).body(facade.createStudent(request)). Matches the reference pattern in backend.md Appendix A.

[claude]

IMPORTANT: @NotNull was removed from @PathVariable here (and on updateStudent line 50, deleteStudent line 58). The old controller carried @PathVariable @NotNull final UUID id on every path param. Per backend.md Appendix A, path variables must carry @PathVariable @NotNull. Please restore the annotation on all three methods.