Feature/863 Objektive Abschluss Kommentar anzeigen (#1273)

* add backend request for completed with unit tests

* display the completed comment in the frontend

* update import

* backend formatting changes

* remove 404 in the completed get request

* apply backend formatter

* inclued test for the completed comment

* Fix naming of endpoint and the corresponding documentation and add better spacing to frontend

* Rename service with typo, fix naming of e2e tests and change some logic in the frontend

* Try to generalize cypress testing

* Fix e2e tests by using correct state for selecting the objectives

* Finish abstraction of completeDialog in e2e testing

* add unit test for completed

* add right role check for get of completed

* fix unit test to use right state

* fix completed is undefined error by not loading in always

* fix takeUntilDestroy

* change state enum to no longer hold svg

* change backend and add tests for completed services

* formatting changes and remove useless methode

* change name of completed service file

* move exception throwing to business service to prevent error while deleting objectives

* formatt backend

* update backend tests for get completed logic

* formatt backend changes

* remove empty test

* fix logic so title of completed comment does not get shown if there is no comment written

---------

Co-authored-by: Manuel <moeri@puzzle.ch>

backend/src/main/java/ch/puzzle/okr/controller/CompletedController.java

@@ -47,4 +47,14 @@ public ResponseEntity<CompletedDto> createCompleted(@RequestBody CompletedDto co
     public void deleteCompletedByObjectiveId(@PathVariable long objectiveId) {
         completedAuthorizationService.deleteCompletedByObjectiveId(objectiveId);
     }
+
+    @Operation(summary = "Get Completed by Objective Id", description = "Get Completed from one Objective by objectiveId.")
+    @ApiResponses(value = { @ApiResponse(responseCode = "200", description = "Returned Completed by Objective Id"),
+            @ApiResponse(responseCode = "401", description = "Not authorized to get Completed Reference", content = @Content),
+            @ApiResponse(responseCode = "404", description = "Did not find the Completed with requested Objective id") })
+    @GetMapping("/{objectiveId}")
+    public CompletedDto getCompletedByObjectiveId(@PathVariable long objectiveId) {
+        Completed completedByObjectiveId = completedAuthorizationService.getCompletedByObjectiveId(objectiveId);
+        return completedMapper.toDto(completedByObjectiveId);
+    }
 }

backend/src/main/java/ch/puzzle/okr/repository/CompletedRepository.java

@@ -1,8 +1,9 @@
 package ch.puzzle.okr.repository;
 
 import ch.puzzle.okr.models.Completed;
+import java.util.Optional;
 import org.springframework.data.repository.CrudRepository;
 
 public interface CompletedRepository extends CrudRepository<Completed, Long> {
-    Completed findByObjectiveId(Long objectiveId);
+    Optional<Completed> findByObjectiveId(Long objectiveId);
 }

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

@@ -27,4 +27,10 @@ public void deleteCompletedByObjectiveId(Long objectiveId) {
         authorizationService.hasRoleDeleteByObjectiveId(objectiveId, authorizationUser);
         completedBusinessService.deleteCompletedByObjectiveId(objectiveId);
     }
+
+    public Completed getCompletedByObjectiveId(Long objectiveId) {
+        AuthorizationUser authorizationUser = authorizationService.updateOrAddAuthorizationUser();
+        authorizationService.hasRoleReadByObjectiveId(objectiveId, authorizationUser);
+        return completedBusinessService.getCompletedByObjectiveId(objectiveId);
+    }
 }

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

@@ -1,9 +1,14 @@
 package ch.puzzle.okr.service.business;
 
+import static org.springframework.http.HttpStatus.NOT_FOUND;
+
+import ch.puzzle.okr.ErrorKey;
+import ch.puzzle.okr.exception.OkrResponseStatusException;
 import ch.puzzle.okr.models.Completed;
 import ch.puzzle.okr.service.persistence.CompletedPersistenceService;
 import ch.puzzle.okr.service.validation.CompletedValidationService;
 import jakarta.transaction.Transactional;
+import java.util.List;
 import org.springframework.stereotype.Service;
 
 @Service
@@ -31,4 +36,16 @@ public void deleteCompletedByObjectiveId(Long objectiveId) {
             completedPersistenceService.deleteById(completed.getId());
         }
     }
+
+    public Completed getCompletedByObjectiveId(Long objectiveId) {
+        Completed completed = completedPersistenceService.getCompletedByObjectiveId(objectiveId);
+        // Must exist in business service in order to prevent error while deleting
+        // ongoing objectives
+        if (completed == null) {
+            throw new OkrResponseStatusException(NOT_FOUND,
+                                                 ErrorKey.MODEL_WITH_ID_NOT_FOUND,
+                                                 List.of(completedPersistenceService.getModelName(), objectiveId));
+        }
+        return completed;
+    }
 }

backend/src/main/java/ch/puzzle/okr/service/persistence/CompletedPersistenceService.java

@@ -19,6 +19,6 @@ public String getModelName() {
     }
 
     public Completed getCompletedByObjectiveId(Long objectiveId) {
-        return getRepository().findByObjectiveId(objectiveId);
+        return getRepository().findByObjectiveId(objectiveId).orElse(null);
     }
 }

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

@@ -4,6 +4,7 @@
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.ArgumentMatchers.anyLong;
 import static org.mockito.Mockito.doThrow;
+import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.*;
 import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.delete;
 import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.post;
 import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.jsonPath;
@@ -122,4 +123,24 @@ void deleteShouldThrowExceptionWhenCompletedWithIdCantBeFound() throws Exception
                 .perform(delete("/api/v2/completed/1000").with(SecurityMockMvcRequestPostProcessors.csrf()))
                 .andExpect(MockMvcResultMatchers.status().isNotFound());
     }
+
+    @DisplayName("get() should get Completed by Objective id")
+    @Test
+    void shouldGetMetricCompletedWithId() throws Exception {
+        mvc
+                .perform(get("/api/v2/completed/1").with(SecurityMockMvcRequestPostProcessors.csrf()))
+                .andExpect(MockMvcResultMatchers.status().isOk());
+    }
+
+    @DisplayName("get() should throw exception when Completed with id cant be found")
+    @Test
+    void getShouldThrowExceptionWhenCompletedWithIdCantBeFound() throws Exception {
+        doThrow(new ResponseStatusException(HttpStatus.NOT_FOUND, "Completed not found"))
+                .when(completedAuthorizationService)
+                .getCompletedByObjectiveId(anyLong());
+
+        mvc
+                .perform(get("/api/v2/completed/1000").with(SecurityMockMvcRequestPostProcessors.csrf()))
+                .andExpect(MockMvcResultMatchers.status().isNotFound());
+    }
 }

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

@@ -85,4 +85,20 @@ void shouldThrowExceptionWhenNotAuthorizedToDeleteCompletedByObjectiveId() {
         assertEquals(UNAUTHORIZED, exception.getStatusCode());
         assertEquals(reason, exception.getReason());
     }
+
+    @DisplayName("Should throw an exception when the user is not authorized to get completed object by objective ID")
+    @Test
+    void shouldThrowExceptionWhenNotAuthorizedToGetCompletedByObjectiveId() {
+        String reason = "junit test reason";
+        when(authorizationService.updateOrAddAuthorizationUser()).thenReturn(authorizationUser);
+        doThrow(new ResponseStatusException(HttpStatus.UNAUTHORIZED, reason))
+                .when(authorizationService)
+                .hasRoleReadByObjectiveId(objectiveId, authorizationUser);
+
+        ResponseStatusException exception = assertThrows(ResponseStatusException.class,
+                                                         () -> completedAuthorizationService
+                                                                 .getCompletedByObjectiveId(objectiveId));
+        assertEquals(UNAUTHORIZED, exception.getStatusCode());
+        assertEquals(reason, exception.getReason());
+    }
 }

backend/src/test/java/ch/puzzle/okr/service/business/CompletedBusinessServiceTest.java

@@ -1,9 +1,10 @@
 package ch.puzzle.okr.service.business;
 
-import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.*;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.Mockito.*;
 
+import ch.puzzle.okr.exception.OkrResponseStatusException;
 import ch.puzzle.okr.models.Completed;
 import ch.puzzle.okr.models.Objective;
 import ch.puzzle.okr.service.persistence.CompletedPersistenceService;
@@ -84,7 +85,7 @@ void shouldDeleteKeyResultAndAssociatedCheckIns() {
         verify(this.completedPersistenceService, times(1)).deleteById(1L);
     }
 
-    @DisplayName("Should do nothing if completed is null")
+    @DisplayName("Should do nothing if completed to delete is null")
     @Test
     void shouldDoNothingIfCompletedIsNull() {
         when(completedPersistenceService.getCompletedByObjectiveId(anyLong())).thenReturn(null);
@@ -93,4 +94,26 @@ void shouldDoNothingIfCompletedIsNull() {
 
         verify(validator, never()).validateOnDelete(anyLong());
     }
+
+    @DisplayName("Should get completed by objective id")
+    @Test
+    void shouldGetCompleted() {
+        when(completedPersistenceService.getCompletedByObjectiveId(anyLong())).thenReturn(successfulCompleted);
+
+        this.completedBusinessService.getCompletedByObjectiveId(1L);
+
+        verify(this.completedPersistenceService, times(1)).getCompletedByObjectiveId(1L);
+    }
+
+    @DisplayName("Should throw exception if completed is null")
+    @Test
+    void shouldThrowExceptionIfCompletedIsNull() {
+        when(completedPersistenceService.getCompletedByObjectiveId(-1L)).thenReturn(null);
+        when(completedPersistenceService.getModelName()).thenCallRealMethod();
+
+        assertThrows(OkrResponseStatusException.class, () -> completedBusinessService.getCompletedByObjectiveId(-1L));
+
+        verify(this.completedPersistenceService, times(1)).getCompletedByObjectiveId(-1L);
+    }
+
 }

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

@@ -137,21 +137,15 @@ void deleteByIdShouldDeleteExistingCompletedByObjectiveId() {
     @DisplayName("Should throw exception on findById() when id does not exist")
     @Test
     void deleteCompletedShouldThrowExceptionWhenCompletedNotFound() {
-        long noExistentId = getNonExistentId();
 
         OkrResponseStatusException exception = assertThrows(OkrResponseStatusException.class,
-                                                            () -> completedPersistenceService.findById(noExistentId));
+                                                            () -> completedPersistenceService.findById(-1L));
 
         List<ErrorDto> expectedErrors = List
-                .of(new ErrorDto("MODEL_WITH_ID_NOT_FOUND", List.of(COMPLETED, String.valueOf(noExistentId))));
+                .of(new ErrorDto("MODEL_WITH_ID_NOT_FOUND", List.of(COMPLETED, String.valueOf(-1))));
 
         assertEquals(NOT_FOUND, exception.getStatusCode());
         assertThat(expectedErrors).hasSameElementsAs(exception.getErrors());
         assertTrue(TestHelper.getAllErrorKeys(expectedErrors).contains(exception.getReason()));
     }
-
-    private long getNonExistentId() {
-        long id = completedPersistenceService.findAll().stream().mapToLong(Completed::getId).max().orElse(10L);
-        return id + 1;
-    }
 }

frontend/cypress/e2e/objective.cy.ts

@@ -31,57 +31,30 @@ describe('okr objective', () => {
         .should('exist');
     });
 
-    it('should complete objective with successful', () => {
-      overviewPage.addObjective()
-        .fillObjectiveTitle('We want to complete this successful')
+    it('should complete objective as successful and write successful closing comment', () => {
+      const title = 'This objective should be successful';
+      const comment = 'This objective has been successfully completed. Good work';
+      overviewPage.completeObjective(title)
+        .completeAs(true)
+        .writeClosingComment(comment)
         .submit();
 
-      overviewPage
-        .getObjectiveByNameAndState('We want to complete this successful', 'ongoing')
-        .findByTestId('three-dot-menu')
-        .click();
-
-      overviewPage.selectFromThreeDotMenu('Objective abschliessen');
-
-      cy.contains('Bewertung');
-      cy.contains('Objective erreicht');
-      cy.contains('Objective nicht erreicht');
-      cy.contains('Kommentar (optional)');
-      cy.contains('Objective abschliessen');
-      cy.contains('Abbrechen');
-
-      cy.getByTestId('successful')
-        .click();
-      cy.getByTestId('submit')
+      overviewPage.getObjectiveByNameAndState(title, 'successful')
         .click();
-
-      overviewPage.getObjectiveByNameAndState('We want to complete this successful', 'successful');
+      cy.contains(comment);
     });
 
-    it('should complete objective with not-successful', () => {
-      overviewPage.addObjective()
-        .fillObjectiveTitle('A not successful objective')
+    it('should complete objective as not-successful and write unsuccessful closing comment', () => {
+      const title = 'This objective should NOT be successful';
+      const comment = 'This objective has not been completed successfully. We need to work on this';
+      overviewPage.completeObjective(title)
+        .completeAs(false)
+        .writeClosingComment(comment)
         .submit();
 
-      overviewPage
-        .getObjectiveByNameAndState('A not successful objective', 'ongoing')
-        .findByTestId('three-dot-menu')
+      overviewPage.getObjectiveByNameAndState(title, 'not-successful')
         .click();
-      overviewPage.selectFromThreeDotMenu('Objective abschliessen');
-
-      cy.contains('Bewertung');
-      cy.contains('Objective erreicht');
-      cy.contains('Objective nicht erreicht');
-      cy.contains('Kommentar (optional)');
-      cy.contains('Objective abschliessen');
-      cy.contains('Abbrechen');
-
-      cy.getByTestId('not-successful')
-        .click();
-      cy.getByTestId('submit')
-        .click();
-
-      overviewPage.getObjectiveByNameAndState('A not successful objective', 'not-successful');
+      cy.contains(comment);
     });
 
     it('should reopen successful objective', () => {

frontend/cypress/support/helper/dom-helper/dialogs/completeDialog.ts

@@ -0,0 +1,25 @@
+import Dialog from './dialog';
+
+export default class CompleteDialog extends Dialog {
+  override submit() {
+    cy.getByTestId('submit')
+      .click();
+  }
+
+  completeAs(isSuccessful: boolean) {
+    isSuccessful ? cy.getByTestId('successful')
+      .click() : cy.getByTestId('not-successful')
+      .click();
+    return this;
+  }
+
+  writeClosingComment(comment: string) {
+    cy.getByTestId('completeComment')
+      .type(comment);
+    return this;
+  }
+
+  getPage(): Cypress.Chainable {
+    return cy.get('app-complete-dialog');
+  }
+}

frontend/cypress/support/helper/dom-helper/pages/overviewPage.ts

@@ -3,6 +3,7 @@ import ObjectiveDialog from '../dialogs/objectiveDialog';
 import { Page } from './page';
 import KeyResultDialog from '../dialogs/keyResultDialog';
 import { filterByKeyResultName, getKeyResults } from '../../keyResultHelper';
+import CompleteDialog from '../dialogs/completeDialog';
 
 export default class CyOverviewPage extends Page {
   elements = {
@@ -187,6 +188,27 @@ export default class CyOverviewPage extends Page {
     return new ObjectiveDialog();
   }
 
+  completeObjective(title: string) {
+    this.addObjective()
+      .fillObjectiveTitle(title)
+      .submit();
+
+    this
+      .getObjectiveByNameAndState(title, 'ongoing')
+      .findByTestId('three-dot-menu')
+      .click();
+    this.selectFromThreeDotMenu('Objective abschliessen');
+
+    cy.contains('Bewertung');
+    cy.contains('Objective erreicht');
+    cy.contains('Objective nicht erreicht');
+    cy.contains('Kommentar (optional)');
+    cy.contains('Objective abschliessen');
+    cy.contains('Abbrechen');
+
+    return new CompleteDialog();
+  }
+
   visitTeamManagement(): void {
     this.elements.teamManagement()
       .click();

frontend/src/app/components/objective-detail/objective-detail.component.html

@@ -19,7 +19,7 @@ <h2 class="title" [attr.data-testId]="'objective-title'">{{ objective.title }}</
 
       <section class="me-3">
         <h3 class="mb-1">Beschrieb</h3>
-        <div class="linebreak" *ngIf="objective.description === ''">
+        <div class="linebreak mb-3" *ngIf="objective.description === ''">
           <p>-</p>
         </div>
         <div
@@ -29,8 +29,7 @@ <h3 class="mb-1">Beschrieb</h3>
         >
           <p>{{ objective.description }}</p>
         </div>
-
-        <div class="d-flex align-items-center flex-row justify-content-start" *ngIf="objective.state.toUpperCase() !== 'SUCCESSFUL' && objective.state.toUpperCase() !== 'NOTSUCCESSFUL'">
+        <div *ngIf="objective.state !== State.SUCCESSFUL && objective.state !== State.NOTSUCCESSFUL; else isCompleted" class="d-flex align-items-center flex-row justify-content-start">
           <button
             *ngIf="objective.isWriteable"
             mat-flat-button
@@ -54,6 +53,14 @@ <h3 class="mb-1">Beschrieb</h3>
             Objective bearbeiten
           </button>
         </div>
+        <ng-template #isCompleted>
+          <div *ngIf="(completed | async)?.comment && (completed | async) as completed">
+            <h3 [attr.data-testId]="'completed-comment'" class="mb-1">Abschlusskommentar</h3>
+            <div class="linebreak">
+              <p>{{ completed.comment }}</p>
+            </div>
+          </div>
+        </ng-template>
       </section>
     </div>
   </ng-container>