Merge pull request #662 from puzzle/bug/sonar-smells

Fix sonar bugs

backend/pom.xml

@@ -99,12 +99,16 @@
                 <version>0.8.8</version>
                 <configuration>
                     <excludes>
-                        <exclude>ch/puzzle/okr/service/persistence/OverviewPersistenceService*</exclude>
                         <exclude>ch/puzzle/okr/models/**</exclude>
                         <exclude>ch/puzzle/okr/dto/**</exclude>
                         <exclude>ch/puzzle/okr/mapper/**</exclude>
-                        <exclude>**/OkrApplication.*</exclude>
+                        <exclude>ch/puzzle/okr/OkrApplication.*</exclude>
+                        <exclude>ch/puzzle/okr/Constants.java</exclude>
+                        <exclude>ch/puzzle/okr/ErrorKey.java</exclude>
                         <exclude>ch/puzzle/okr/SecurityConfig.java</exclude>
+                        <exclude>ch/puzzle/okr/SpringCachingConfig.java</exclude>
+                        <exclude>ch/puzzle/okr/OkrErrorAttributes.java</exclude>
+                        <exclude>ch/puzzle/okr/OpenAPI30Configuration.java</exclude>
                     </excludes>
                 </configuration>
                 <executions>

backend/src/main/java/ch/puzzle/okr/service/authorization/AuthorizationServiceBase.java

@@ -16,7 +16,7 @@ public abstract class AuthorizationServiceBase<ID, T extends WriteableInterface,
     private final BusinessServiceInterface<ID, T> businessService;
     private final AuthorizationService authorizationService;
 
-    public AuthorizationServiceBase(BusinessServiceInterface<ID, T> businessService,
+    protected AuthorizationServiceBase(BusinessServiceInterface<ID, T> businessService,
             AuthorizationService authorizationService) {
         this.businessService = businessService;
         this.authorizationService = authorizationService;

backend/src/main/java/ch/puzzle/okr/service/business/TeamBusinessService.java

@@ -23,8 +23,8 @@ public class TeamBusinessService {
     private final CacheService cacheService;
 
     public TeamBusinessService(TeamPersistenceService teamPersistenceService,
-            ObjectiveBusinessService objectiveBusinessService, QuarterBusinessService quarterBusinessService,
-            TeamValidationService validator, CacheService cacheService) {
+            ObjectiveBusinessService objectiveBusinessService, TeamValidationService validator,
+            CacheService cacheService) {
         this.teamPersistenceService = teamPersistenceService;
         this.objectiveBusinessService = objectiveBusinessService;
         this.validator = validator;

backend/src/test/java/ch/puzzle/okr/controller/ObjectiveControllerIT.java

@@ -5,7 +5,6 @@
 import ch.puzzle.okr.models.*;
 import ch.puzzle.okr.service.authorization.AuthorizationService;
 import ch.puzzle.okr.service.authorization.ObjectiveAuthorizationService;
-import ch.puzzle.okr.service.business.ObjectiveBusinessService;
 import org.hamcrest.core.Is;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
@@ -66,7 +65,7 @@ class ObjectiveControllerIT {
             """;
     private static final String RESPONSE_NEW_OBJECTIVE = """
             {"id":null,"version":1,"title":"Program Faster","teamId":1,"quarterId":1,"description":"Just be faster","state":"DRAFT","createdOn":null,"modifiedOn":null,"writeable":true}""";
-
+    private static final String JSON_PATH_TITLE = "$.title";
     private static final Objective objective1 = Objective.Builder.builder().withId(5L).withTitle(OBJECTIVE_TITLE_1)
             .build();
     private static final Objective objective2 = Objective.Builder.builder().withId(7L).withTitle(OBJECTIVE_TITLE_2)
@@ -88,8 +87,6 @@ class ObjectiveControllerIT {
     @MockBean
     private ObjectiveAuthorizationService objectiveAuthorizationService;
     @Mock
-    private ObjectiveBusinessService objectiveBusinessService;
-    @Mock
     private AuthorizationService authorizationService;
     @MockBean
     private ObjectiveMapper objectiveMapper;
@@ -106,7 +103,7 @@ void getObjectiveById() throws Exception {
 
         mvc.perform(get(URL_OBJECTIVE_5).contentType(MediaType.APPLICATION_JSON))
                 .andExpect(MockMvcResultMatchers.status().isOk()).andExpect(jsonPath("$.id", Is.is(5)))
-                .andExpect(jsonPath("$.title", Is.is(OBJECTIVE_TITLE_1)));
+                .andExpect(jsonPath(JSON_PATH_TITLE, Is.is(OBJECTIVE_TITLE_1)));
     }
 
     @Test
@@ -158,7 +155,7 @@ void shouldReturnUpdatedObjective() throws Exception {
                 .with(SecurityMockMvcRequestPostProcessors.csrf())).andExpect(MockMvcResultMatchers.status().isOk())
                 .andExpect(jsonPath("$.id", Is.is(1)))
                 .andExpect(jsonPath("$.description", Is.is(EVERYTHING_FINE_DESCRIPTION)))
-                .andExpect(jsonPath("$.title", Is.is(TITLE)));
+                .andExpect(jsonPath(JSON_PATH_TITLE, Is.is(TITLE)));
     }
 
     @Test
@@ -224,7 +221,7 @@ void shouldReturnIsCreatedWhenObjectiveWasDuplicated() throws Exception {
                 .andExpect(MockMvcResultMatchers.status().isCreated())
                 .andExpect(jsonPath("$.id", Is.is(objective1Dto.id().intValue())))
                 .andExpect(jsonPath("$.description", Is.is(objective1Dto.description())))
-                .andExpect(jsonPath("$.title", Is.is(objective1Dto.title())));
+                .andExpect(jsonPath(JSON_PATH_TITLE, Is.is(objective1Dto.title())));
 
         verify(objectiveMapper, times(1)).toObjective(any());
         verify(objectiveMapper, times(1)).toDto(any());

backend/src/test/java/ch/puzzle/okr/controller/OverviewControllerIT.java

@@ -43,6 +43,7 @@ class OverviewControllerIT {
     private OverviewAuthorizationService overviewAuthorizationService;
     @MockBean
     OrganisationBusinessService organisationBusinessService;
+    // Dashboard and OverviewMapper are required for testing
     @SpyBean
     private DashboardMapper dashboardMapper;
     @SpyBean
@@ -55,6 +56,7 @@ class OverviewControllerIT {
     public static final String CHF = "CHF";
     public static final String JSON_PATH_TEAM_NAME = "$.overviews[0].team.name";
     public static final String JSON_PATH_TEAM_ID = "$.overviews[0].team.id";
+    public static final String JSON_PATH_OVERVIEWS = "$.overviews";
 
     static List<Overview> overviewPuzzle = List.of(
             Overview.Builder.builder().withOverviewId(OverviewId.of(1L, 1L, 20L, 20L)).withTeamName(PUZZLE)
@@ -100,7 +102,7 @@ void shouldGetAllTeamsWithObjective() throws Exception {
 
         mvc.perform(get("/api/v2/overview?quarter=2&team=1,2,3,4").contentType(MediaType.APPLICATION_JSON))
                 .andExpect(MockMvcResultMatchers.status().isOk())
-                .andExpect(jsonPath("$.overviews", Matchers.hasSize(3)))
+                .andExpect(jsonPath(JSON_PATH_OVERVIEWS, Matchers.hasSize(3)))
                 .andExpect(jsonPath("$.adminAccess", Is.is(true))).andExpect(jsonPath(JSON_PATH_TEAM_ID, Is.is(1)))
                 .andExpect(jsonPath(JSON_PATH_TEAM_NAME, Is.is(PUZZLE)))
                 .andExpect(jsonPath("$.overviews[0].objectives[0].id", Is.is(1)))
@@ -122,7 +124,7 @@ void shouldGetAllTeamsWithObjectiveIfNoTeamsExists() throws Exception {
 
         mvc.perform(get("/api/v2/overview").contentType(MediaType.APPLICATION_JSON))
                 .andExpect(MockMvcResultMatchers.status().isOk())
-                .andExpect(jsonPath("$.overviews", Matchers.hasSize(0)))
+                .andExpect(jsonPath(JSON_PATH_OVERVIEWS, Matchers.hasSize(0)))
                 .andExpect(jsonPath("$.adminAccess", Is.is(true)));
     }
 
@@ -135,7 +137,7 @@ void shouldReturnOnlyFilteredObjectivesByQuarterAndTeam() throws Exception {
 
         mvc.perform(get("/api/v2/overview?quarter=2&team=1,3").contentType(MediaType.APPLICATION_JSON))
                 .andExpect(MockMvcResultMatchers.status().isOk())
-                .andExpect(jsonPath("$.overviews", Matchers.hasSize(2)))
+                .andExpect(jsonPath(JSON_PATH_OVERVIEWS, Matchers.hasSize(2)))
                 .andExpect(jsonPath(JSON_PATH_TEAM_ID, Is.is(1)))
                 .andExpect(jsonPath(JSON_PATH_TEAM_NAME, Is.is(PUZZLE)))
                 .andExpect(jsonPath("$.overviews[0].objectives[0].id", Is.is(1)))
@@ -152,7 +154,7 @@ void shouldReturnTeamWithEmptyObjectiveListWhenNoObjectiveInFilteredQuarter() th
 
         mvc.perform(get("/api/v2/overview?quarter=2&team=4").contentType(MediaType.APPLICATION_JSON))
                 .andExpect(MockMvcResultMatchers.status().isOk())
-                .andExpect(jsonPath("$.overviews", Matchers.hasSize(1)))
+                .andExpect(jsonPath(JSON_PATH_OVERVIEWS, Matchers.hasSize(1)))
                 .andExpect(jsonPath(JSON_PATH_TEAM_ID, Is.is(4)))
                 .andExpect(jsonPath(JSON_PATH_TEAM_NAME, Is.is(TEAM_KUCHEN)))
                 .andExpect(jsonPath("$.overviews[0].objectives.size()", Is.is(0)));

backend/src/test/java/ch/puzzle/okr/service/authorization/AuthorizationRegistrationServiceIT.java

@@ -91,10 +91,11 @@ void registerAuthorizationUserShouldSetTeamOrganisations() {
 
     @Test
     void registerAuthorizationUserShouldSetChampions() {
-        User user = User.Builder.builder().withFirstname("firstname").withLastname("lastname").withUsername("peggimann")
-                .withEmail("[email protected]").build();
-        Jwt token = mockJwtToken(user, List.of(ORGANISATION_FIRST_LEVEL, ORGANISATION_SECOND_LEVEL));
-        AuthorizationUser authorizationUser = authorizationRegistrationService.registerAuthorizationUser(user, token);
+        User testUser = User.Builder.builder().withFirstname("firstname").withLastname("lastname")
+                .withUsername("peggimann").withEmail("[email protected]").build();
+        Jwt token = mockJwtToken(testUser, List.of(ORGANISATION_FIRST_LEVEL, ORGANISATION_SECOND_LEVEL));
+        AuthorizationUser authorizationUser = authorizationRegistrationService.registerAuthorizationUser(testUser,
+                token);
 
         assertRoles(List.of(READ_ALL_DRAFT, READ_ALL_PUBLISHED, WRITE_ALL), authorizationUser);
     }

backend/src/test/java/ch/puzzle/okr/service/persistence/ActionPersistenceServiceIT.java

@@ -36,6 +36,8 @@ private static Action createAction(Long id, int version) {
                 .build();
     }
 
+    private static final String UPDATED_ACTION = "Updated Action";
+
     @AfterEach
     void tearDown() {
         try {
@@ -67,21 +69,21 @@ void saveActionShouldSaveNewAction() {
     void updateActionShouldUpdateAction() {
         Action action = createAction(null);
         createdAction = actionPersistenceService.save(action);
-        createdAction.setAction("Updated Action");
+        createdAction.setAction(UPDATED_ACTION);
         createdAction.setPriority(4);
 
         Action updateAction = actionPersistenceService.save(createdAction);
 
         assertEquals(createdAction.getId(), updateAction.getId());
-        assertEquals("Updated Action", updateAction.getAction());
+        assertEquals(UPDATED_ACTION, updateAction.getAction());
         assertEquals(4, updateAction.getPriority());
     }
 
     @Test
     void updateActionShouldThrowExceptionWhenAlreadyUpdated() {
         createdAction = actionPersistenceService.save(createAction(null));
         Action changedAction = createAction(createdAction.getId(), 0);
-        changedAction.setAction("Updated Action");
+        changedAction.setAction(UPDATED_ACTION);
 
         OkrResponseStatusException exception = assertThrows(OkrResponseStatusException.class,
                 () -> actionPersistenceService.save(changedAction));

backend/src/test/java/ch/puzzle/okr/service/persistence/AuthorizationCriteriaIT.java

@@ -1,6 +1,5 @@
 package ch.puzzle.okr.service.persistence;
 
-import ch.puzzle.okr.dto.ErrorDto;
 import ch.puzzle.okr.models.Objective;
 import ch.puzzle.okr.models.authorization.AuthorizationUser;
 import ch.puzzle.okr.models.overview.Overview;
@@ -16,7 +15,6 @@
 
 @SpringIntegrationTest
 class AuthorizationCriteriaIT {
-    private static final String REASON = "not authorized to read objective";
 
     @Autowired
     ObjectivePersistenceService objectivePersistenceService;

backend/src/test/java/ch/puzzle/okr/service/persistence/CompletedPersistenceServiceIT.java

@@ -34,6 +34,8 @@ private static Completed createCompleted(Long id, int version) {
                 .withComment("Wir haben es gut geschafft").build();
     }
 
+    private static final String COMPLETED = "Completed";
+
     @AfterEach
     void tearDown() {
         try {
@@ -79,7 +81,7 @@ void updateCompletedShouldThrowExceptionWhenAlreadyUpdated() {
         OkrResponseStatusException exception = assertThrows(OkrResponseStatusException.class,
                 () -> completedPersistenceService.save(updateCompleted));
 
-        List<ErrorDto> expectedErrors = List.of(new ErrorDto("DATA_HAS_BEEN_UPDATED", List.of("Completed")));
+        List<ErrorDto> expectedErrors = List.of(new ErrorDto("DATA_HAS_BEEN_UPDATED", List.of(COMPLETED)));
 
         assertEquals(UNPROCESSABLE_ENTITY, exception.getStatus());
         assertThat(expectedErrors).hasSameElementsAs(exception.getErrors());
@@ -104,7 +106,7 @@ void deleteCompletedIdShouldDeleteExistingCompletedByObjectiveId() {
         OkrResponseStatusException exception = assertThrows(OkrResponseStatusException.class,
                 () -> completedPersistenceService.findById(3L));
 
-        List<ErrorDto> expectedErrors = List.of(new ErrorDto("MODEL_WITH_ID_NOT_FOUND", List.of("Completed", "3")));
+        List<ErrorDto> expectedErrors = List.of(new ErrorDto("MODEL_WITH_ID_NOT_FOUND", List.of(COMPLETED, "3")));
 
         assertEquals(NOT_FOUND, exception.getStatus());
         assertThat(expectedErrors).hasSameElementsAs(exception.getErrors());
@@ -120,7 +122,7 @@ void deleteCompletedShouldThrowExceptionWhenCompletedNotFound() {
         OkrResponseStatusException exception = assertThrows(OkrResponseStatusException.class,
                 () -> completedPersistenceService.findById(completedId));
 
-        List<ErrorDto> expectedErrors = List.of(new ErrorDto("MODEL_WITH_ID_NOT_FOUND", List.of("Completed", "200")));
+        List<ErrorDto> expectedErrors = List.of(new ErrorDto("MODEL_WITH_ID_NOT_FOUND", List.of(COMPLETED, "200")));
 
         assertEquals(NOT_FOUND, exception.getStatus());
         assertThat(expectedErrors).hasSameElementsAs(exception.getErrors());

backend/src/test/java/ch/puzzle/okr/service/persistence/KeyResultPersistenceServiceIT.java

@@ -50,6 +50,8 @@ private static KeyResult createKeyResultOrdinal(Long id, int version) {
 
     private static final String KEY_RESULT_UPDATED = "Updated Key Result";
     private static final String THIS_IS_DESCRIPTION = "This is a new description";
+    private static final String MODEL_NOT_FOUND = "MODEL_WITH_ID_NOT_FOUND";
+    private static final String KEYRESULT = "KeyResult";
 
     @AfterEach
     void tearDown() {
@@ -92,7 +94,7 @@ void getKeyResultByIdShouldThrowExceptionWhenKeyResultNotFound() {
         OkrResponseStatusException exception = assertThrows(OkrResponseStatusException.class,
                 () -> keyResultPersistenceService.findById(321L));
 
-        List<ErrorDto> expectedErrors = List.of(new ErrorDto("MODEL_WITH_ID_NOT_FOUND", List.of("KeyResult", "321")));
+        List<ErrorDto> expectedErrors = List.of(new ErrorDto(MODEL_NOT_FOUND, List.of(KEYRESULT, "321")));
 
         assertEquals(NOT_FOUND, exception.getStatus());
         assertThat(expectedErrors).hasSameElementsAs(exception.getErrors());
@@ -104,7 +106,7 @@ void getKeyResultByIdShouldThrowExceptionWhenKeyResultIdIsNull() {
         OkrResponseStatusException exception = assertThrows(OkrResponseStatusException.class,
                 () -> keyResultPersistenceService.findById(null));
 
-        List<ErrorDto> expectedErrors = List.of(new ErrorDto("ATTRIBUTE_NULL", List.of("ID", "KeyResult")));
+        List<ErrorDto> expectedErrors = List.of(new ErrorDto("ATTRIBUTE_NULL", List.of("ID", KEYRESULT)));
 
         assertEquals(BAD_REQUEST, exception.getStatus());
         assertThat(expectedErrors).hasSameElementsAs(exception.getErrors());
@@ -129,8 +131,7 @@ void recreateEntityShouldUpdateKeyResultNoTypeChange() {
         // Should delete the old KeyResult
         OkrResponseStatusException exception = assertThrows(OkrResponseStatusException.class, this::execute);
 
-        List<ErrorDto> expectedErrors = List
-                .of(ErrorDto.of("MODEL_WITH_ID_NOT_FOUND", List.of("KeyResult", keyResultId)));
+        List<ErrorDto> expectedErrors = List.of(ErrorDto.of(MODEL_NOT_FOUND, List.of(KEYRESULT, keyResultId)));
 
         assertEquals(NOT_FOUND, exception.getStatus());
         assertThat(expectedErrors).hasSameElementsAs(exception.getErrors());
@@ -164,8 +165,7 @@ void recreateEntityShouldUpdateKeyResultWithTypeChange() {
         OkrResponseStatusException exception = assertThrows(OkrResponseStatusException.class,
                 () -> keyResultPersistenceService.findById(keyResultId));
 
-        List<ErrorDto> expectedErrors = List
-                .of(ErrorDto.of("MODEL_WITH_ID_NOT_FOUND", List.of("KeyResult", keyResultId)));
+        List<ErrorDto> expectedErrors = List.of(ErrorDto.of(MODEL_NOT_FOUND, List.of(KEYRESULT, keyResultId)));
 
         assertEquals(NOT_FOUND, exception.getStatus());
         assertThat(expectedErrors).hasSameElementsAs(exception.getErrors());
@@ -204,7 +204,7 @@ void updateEntityShouldThrowExceptionWhenAlreadyUpdated() {
 
         OkrResponseStatusException exception = assertThrows(OkrResponseStatusException.class,
                 () -> keyResultPersistenceService.updateEntity(updateKeyResult));
-        List<ErrorDto> expectedErrors = List.of(new ErrorDto("DATA_HAS_BEEN_UPDATED", List.of("KeyResult")));
+        List<ErrorDto> expectedErrors = List.of(new ErrorDto("DATA_HAS_BEEN_UPDATED", List.of(KEYRESULT)));
 
         assertEquals(UNPROCESSABLE_ENTITY, exception.getStatus());
         assertThat(expectedErrors).hasSameElementsAs(exception.getErrors());
@@ -228,8 +228,7 @@ void deleteKeyResultByIdShouldDeleteExistingKeyResult() {
         OkrResponseStatusException exception = assertThrows(OkrResponseStatusException.class,
                 () -> keyResultPersistenceService.findById(keyResultId));
 
-        List<ErrorDto> expectedErrors = List
-                .of(ErrorDto.of("MODEL_WITH_ID_NOT_FOUND", List.of("KeyResult", keyResultId)));
+        List<ErrorDto> expectedErrors = List.of(ErrorDto.of(MODEL_NOT_FOUND, List.of(KEYRESULT, keyResultId)));
 
         assertEquals(NOT_FOUND, exception.getStatus());
         assertThat(expectedErrors).hasSameElementsAs(exception.getErrors());
@@ -246,8 +245,7 @@ void deleteKeyResultShouldThrowExceptionWhenKeyResultNotFound() {
         OkrResponseStatusException exception = assertThrows(OkrResponseStatusException.class,
                 () -> keyResultPersistenceService.findById(keyResultId));
 
-        List<ErrorDto> expectedErrors = List
-                .of(ErrorDto.of("MODEL_WITH_ID_NOT_FOUND", List.of("KeyResult", keyResultId)));
+        List<ErrorDto> expectedErrors = List.of(ErrorDto.of(MODEL_NOT_FOUND, List.of(KEYRESULT, keyResultId)));
 
         assertEquals(NOT_FOUND, exception.getStatus());
         assertThat(expectedErrors).hasSameElementsAs(exception.getErrors());