Add comprehensive unit and integration tests for UserViewController, UserMapper, and UserService

[Martin-E-Karlsson]

Unit and Integration tests for:

• UserViewController
• UserMapper
• UserService

Changes to the controllers will have to be made if endpoints are rearranged and when further features are added.

Summary by CodeRabbit

• Tests
  • Expanded test coverage for user management: added comprehensive controller, service (unit & integration), and mapper tests covering signup/login flows, profile view/edit/update, role-based dashboards and lists, authorization checks, CRUD operations, and mapping behaviors.

[coderabbitai]

Warning

Rate limit exceeded

@Martin-E-Karlsson has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 59 minutes and 5 seconds before requesting another review.

Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 59 minutes and 5 seconds.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: c5312c70-b24e-45ae-ac92-acbb54b54bd5

📥 Commits

Reviewing files that changed from the base of the PR and between e9fe0fc and 03aea0c.

📒 Files selected for processing (2)

• src/test/java/org/example/visacasemanagementsystem/user/controller/UserViewControllerTest.java
• src/test/java/org/example/visacasemanagementsystem/user/service/UserServiceTest.java

📝 Walkthrough

Walkthrough

Adds multiple new test suites (controller, service unit, service integration, mapper) and makes TestcontainersConfiguration public; no production code logic changed.

Changes

Cohort / File(s)	Summary
Test Configuration src/test/java/.../TestcontainersConfiguration.java	Changed class visibility from package-private to public.
Controller Tests src/test/java/.../user/controller/UserViewControllerTest.java	New Spring MVC @WebMvcTest using MockMvc; tests signup/login views, profile view/edit flows, role-based access, dashboards, and user listing; mocks services and simulates authentication.
Service Layer Tests src/test/java/.../user/service/UserServiceTest.java, src/test/java/.../user/service/UserServiceIntegrationTest.java	Added unit tests (Mockito) for UserService covering create/update/delete/authorization/error mapping and retrieval; added Spring Boot integration tests with H2 for persistence-backed UserService scenarios and validateProfileAccess checks.
Mapper Tests src/test/java/.../user/mapper/UserMapperTest.java	Added unit tests for UserMapper covering toDTO, toEntity, updateEntityFromDTO, null-safety, and preservation/non-preservation of sensitive fields (password).

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

• Added create, update and delete methods for UserService. I created a … #14 — Adds tests exercising UserService APIs (create/update/delete/find/validateProfileAccess) that closely relate to these tests.
• Feature/user endpoints #26 — Adds user/controller, service, and mapper functionality that these tests target.
• Adds the security layer to the VisaViewController, and updates tests. #36 — Related to UserViewController and security changes exercised by the new MVC tests.

Suggested reviewers

• EdvinSandgren
• eeebbaandersson

Poem

"Hop hop, I test and I prance, 🐰
Mocking services in a merry dance,
DTOs and profiles checked with care,
Integration H2 gives persistence flair,
Now the codebase hops with confidence rare."

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 40.43% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the primary changes: adding comprehensive unit and integration tests for three core components (UserViewController, UserMapper, UserService).
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch user-tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (4)

src/test/java/org/example/visacasemanagementsystem/user/mapper/UserMapperTest.java (1)

39-44: Consider also asserting dto.id().

toDTO maps user.getId() into UserDTO.id, but the test never asserts it. Setting user.setId(...) in the arrange block and adding an assertThat(dto.id()).isEqualTo(...) would make the mapping contract fully covered and guard against accidental field-order regressions in the UserDTO record.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@src/test/java/org/example/visacasemanagementsystem/user/mapper/UserMapperTest.java`
around lines 39 - 44, The test UserMapperTest does not assert that toDTO
preserves the user ID; update the arrange section to set user.setId(...) with a
known value and add an assertion in the assert block verifying dto.id() equals
that value (e.g., assertThat(dto.id()).isEqualTo(...)); this ensures the toDTO
mapping and the UserDTO.id field are covered and prevents regressions in
UserMapper.toDTO or the UserDTO record.

src/test/java/org/example/visacasemanagementsystem/user/service/UserServiceIntegrationTest.java (2)

224-244: Minor: helpers bypass UserService and persist plaintext passwords.

Persisting "password123" directly to the DB is fine for tests and matches other suites, but note that createUser-path tests (line 35) go through passwordEncoder.encode(...) while update/delete/find tests read rows with plaintext passwords. If any future assertion checks the hashed password, this asymmetry will cause confusing failures. Consider encoding in the helper or routing through userService.createUser.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@src/test/java/org/example/visacasemanagementsystem/user/service/UserServiceIntegrationTest.java`
around lines 224 - 244, The helpers createAndSaveValidUser and createAndSaveUser
persist plaintext "password123" via userRepository, which bypasses UserService
password hashing; update these helpers to either call
userService.createUser(...) so the password is encoded through the normal flow,
or explicitly encode the password with passwordEncoder.encode("password123")
before saving with userRepository.save(...), ensuring tests that inspect stored
passwords see hashed values and stay consistent with createUser-path tests.

---

110-122: Test is order/seed-dependent.

findAll is asserted with hasSizeGreaterThanOrEqualTo(2). Because @Transactional on @SpringBootTest rolls back per test, this is safe in isolation, but any seed data (e.g. a data.sql or @Sql) will inflate the count and hide regressions where extra rows leak in. A stricter assertion using the two returned emails would be more precise:

Proposed fix

-        createAndSaveValidUser();
-        createAndSaveUser("Admin", UserAuthorization.ADMIN);
+        User u1 = createAndSaveValidUser();
+        User u2 = createAndSaveUser("Admin", UserAuthorization.ADMIN);
@@
-        assertThat(result).hasSizeGreaterThanOrEqualTo(2);
+        assertThat(result)
+                .extracting(UserDTO::email)
+                .contains(u1.getEmail(), u2.getEmail());

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@src/test/java/org/example/visacasemanagementsystem/user/service/UserServiceIntegrationTest.java`
around lines 110 - 122, The test findAll_shouldReturnAllUsers relies on a size
check which can be skewed by seeded data; instead capture the two users created
by createAndSaveValidUser() and createAndSaveUser("Admin",
UserAuthorization.ADMIN) and assert that userService.findAll() contains those
users (e.g. by extracting their email fields and asserting the returned list
contains both emails or containsExactlyInAnyOrder those two emails), so change
the assertion to verify the presence of the two specific created users rather
than a minimum list size.

src/test/java/org/example/visacasemanagementsystem/user/service/UserServiceTest.java (1)

44-61: Unnecessary stub on userMapper.toEntity for the short-password test.

With MockitoExtension in strict mode, stubbing is fine here because the production code invokes userMapper.toEntity(dto) before the length check. Just flagging that if the service is later refactored to validate the password first (a sensible change), this stub will become a UnnecessaryStubbingException. Not blocking.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@src/test/java/org/example/visacasemanagementsystem/user/service/UserServiceTest.java`
around lines 44 - 61, The test stubs userMapper.toEntity(dto) unnecessarily for
createUser_shouldThrowIllegalArgumentException_WhenPasswordIsTooShort; remove
the when(userMapper.toEntity(dto)).thenReturn(new User()) line so the test only
asserts that userService.createUser(dto) throws and still
verifyNoInteractions(userRepository); if you need to keep a stub for other
reasons use Mockito.lenient() on the userMapper.toEntity stub or move that
stubbing into tests that exercise mapping logic.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In
`@src/test/java/org/example/visacasemanagementsystem/user/mapper/UserMapperTest.java`:
- Around line 39-44: The test UserMapperTest does not assert that toDTO
preserves the user ID; update the arrange section to set user.setId(...) with a
known value and add an assertion in the assert block verifying dto.id() equals
that value (e.g., assertThat(dto.id()).isEqualTo(...)); this ensures the toDTO
mapping and the UserDTO.id field are covered and prevents regressions in
UserMapper.toDTO or the UserDTO record.

In
`@src/test/java/org/example/visacasemanagementsystem/user/service/UserServiceIntegrationTest.java`:
- Around line 224-244: The helpers createAndSaveValidUser and createAndSaveUser
persist plaintext "password123" via userRepository, which bypasses UserService
password hashing; update these helpers to either call
userService.createUser(...) so the password is encoded through the normal flow,
or explicitly encode the password with passwordEncoder.encode("password123")
before saving with userRepository.save(...), ensuring tests that inspect stored
passwords see hashed values and stay consistent with createUser-path tests.
- Around line 110-122: The test findAll_shouldReturnAllUsers relies on a size
check which can be skewed by seeded data; instead capture the two users created
by createAndSaveValidUser() and createAndSaveUser("Admin",
UserAuthorization.ADMIN) and assert that userService.findAll() contains those
users (e.g. by extracting their email fields and asserting the returned list
contains both emails or containsExactlyInAnyOrder those two emails), so change
the assertion to verify the presence of the two specific created users rather
than a minimum list size.

In
`@src/test/java/org/example/visacasemanagementsystem/user/service/UserServiceTest.java`:
- Around line 44-61: The test stubs userMapper.toEntity(dto) unnecessarily for
createUser_shouldThrowIllegalArgumentException_WhenPasswordIsTooShort; remove
the when(userMapper.toEntity(dto)).thenReturn(new User()) line so the test only
asserts that userService.createUser(dto) throws and still
verifyNoInteractions(userRepository); if you need to keep a stub for other
reasons use Mockito.lenient() on the userMapper.toEntity stub or move that
stubbing into tests that exercise mapping logic.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 6adccb61-0302-4908-a54c-3c0504123c1c

📥 Commits

Reviewing files that changed from the base of the PR and between 46acde2 and b621d02.

📒 Files selected for processing (5)

• src/test/java/org/example/visacasemanagementsystem/TestcontainersConfiguration.java
• src/test/java/org/example/visacasemanagementsystem/user/controller/UserViewControllerTest.java
• src/test/java/org/example/visacasemanagementsystem/user/mapper/UserMapperTest.java
• src/test/java/org/example/visacasemanagementsystem/user/service/UserServiceIntegrationTest.java
• src/test/java/org/example/visacasemanagementsystem/user/service/UserServiceTest.java

[eeebbaandersson]

Ser fint ut tycker jag!

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (2)

src/test/java/org/example/visacasemanagementsystem/user/service/UserServiceTest.java (2)

83-107: Assert the saved entity’s security-sensitive fields.

This test stubs password encoding but never proves the encoded password or forced USER authorization is saved. save(any(User.class)) would also pass if UserService saved the wrong instance.

Strengthen the assertion

         CreateUserDTO dto = new CreateUserDTO(
-                "New User", "new@test.com", "password123", UserAuthorization.USER
+                "New User", "new@test.com", "password123", UserAuthorization.SYSADMIN
         );
@@
         when(userMapper.toEntity(dto)).thenReturn(mappedUser);
         when(passwordEncoder.encode("password123")).thenReturn("encodedPassword");
-        when(userRepository.save(any(User.class))).thenReturn(savedUser);
+        when(userRepository.save(mappedUser)).thenReturn(savedUser);
         when(userMapper.toDTO(savedUser)).thenReturn(expectedDTO);
@@
         // Assert
         assertThat(result).isEqualTo(expectedDTO);
-        verify(userRepository, times(1)).save(any(User.class));
+        assertThat(mappedUser.getPassword()).isEqualTo("encodedPassword");
+        assertThat(mappedUser.getUserAuthorization()).isEqualTo(UserAuthorization.USER);
+        verify(passwordEncoder).encode("password123");
+        verify(userRepository).save(mappedUser);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@src/test/java/org/example/visacasemanagementsystem/user/service/UserServiceTest.java`
around lines 83 - 107, The test currently stubs passwordEncoder.encode and
userRepository.save but doesn't assert that UserService.createUser actually
saved a User with the encoded password and forced UserAuthorization.USER; update
the test for createUser to capture/verify the exact entity passed to
userRepository.save (use an ArgumentCaptor or verify with argThat) and assert
that the captured User's password equals the encoded value from
passwordEncoder.encode("password123") and that its authorization equals
UserAuthorization.USER; also ensure userMapper.toEntity(dto) and
userMapper.toDTO(savedUser) interactions remain verified so the flow is
validated.

---

184-205: Assert the entity role mutation, not only the mocked DTO.

userMapper.toDTO(targetUser) is mocked to return an ADMIN DTO, so this test can pass even if updateUserAuthorization() stops calling targetUser.setUserAuthorization(...).

Strengthen the assertion

         // Assert
+        assertThat(result).isEqualTo(expectedDTO);
+        assertThat(targetUser.getUserAuthorization()).isEqualTo(UserAuthorization.ADMIN);
         assertThat(result.userAuthorization()).isEqualTo(UserAuthorization.ADMIN);
         verify(userRepository).save(targetUser);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@src/test/java/org/example/visacasemanagementsystem/user/service/UserServiceTest.java`
around lines 184 - 205, The test currently only asserts the mocked UserDTO so it
can pass without the entity being mutated; after calling
userService.updateUserAuthorization(userId, UserAuthorization.ADMIN) assert the
actual entity mutation by checking targetUser.getUserAuthorization() (or capture
the saved entity via an ArgumentCaptor on userRepository.save and assert its
getUserAuthorization() is ADMIN) in addition to existing DTO assertion;
reference updateUserAuthorization, userService, targetUser, userRepository.save
and userMapper.toDTO when making the change.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In
`@src/test/java/org/example/visacasemanagementsystem/user/controller/UserViewControllerTest.java`:
- Around line 55-136: The tests for public endpoints are incorrectly annotated
with `@WithMockUser`, making them authenticated and masking regressions; remove
the `@WithMockUser` annotation from the test methods
userSignupForm_ShouldReturnSignupView,
createUser_WithValidData_ShouldRedirectToLogin,
createUser_WithDuplicateEmail_ShouldReturnSignupViewWithError,
createUser_WithShortPassword_ShouldReturnSignupViewWithError, and
userLoginForm_ShouldReturnLoginView so they run as anonymous requests (keep
existing csrf() on POST tests and existing mocks/verify calls intact).

---

Nitpick comments:
In
`@src/test/java/org/example/visacasemanagementsystem/user/service/UserServiceTest.java`:
- Around line 83-107: The test currently stubs passwordEncoder.encode and
userRepository.save but doesn't assert that UserService.createUser actually
saved a User with the encoded password and forced UserAuthorization.USER; update
the test for createUser to capture/verify the exact entity passed to
userRepository.save (use an ArgumentCaptor or verify with argThat) and assert
that the captured User's password equals the encoded value from
passwordEncoder.encode("password123") and that its authorization equals
UserAuthorization.USER; also ensure userMapper.toEntity(dto) and
userMapper.toDTO(savedUser) interactions remain verified so the flow is
validated.
- Around line 184-205: The test currently only asserts the mocked UserDTO so it
can pass without the entity being mutated; after calling
userService.updateUserAuthorization(userId, UserAuthorization.ADMIN) assert the
actual entity mutation by checking targetUser.getUserAuthorization() (or capture
the saved entity via an ArgumentCaptor on userRepository.save and assert its
getUserAuthorization() is ADMIN) in addition to existing DTO assertion;
reference updateUserAuthorization, userService, targetUser, userRepository.save
and userMapper.toDTO when making the change.

🪄 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: CHILL

Plan: Pro

Run ID: eb02c29d-a0a6-49d1-bdb0-01eceb228af3

📥 Commits

Reviewing files that changed from the base of the PR and between b621d02 and e9fe0fc.

📒 Files selected for processing (3)

• src/test/java/org/example/visacasemanagementsystem/user/controller/UserViewControllerTest.java
• src/test/java/org/example/visacasemanagementsystem/user/service/UserServiceIntegrationTest.java
• src/test/java/org/example/visacasemanagementsystem/user/service/UserServiceTest.java

🚧 Files skipped from review as they are similar to previous changes (1)

• src/test/java/org/example/visacasemanagementsystem/user/service/UserServiceIntegrationTest.java