Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/863 Objektive Abschluss Kommentar anzeigen #1273

Merged
merged 27 commits into from
Jan 14, 2025
Merged
Show file tree
Hide file tree
Changes from 26 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
1d0dd01
add backend request for completed with unit tests
Miguel7373 Jan 7, 2025
f9013eb
display the completed comment in the frontend
Miguel7373 Jan 7, 2025
9d5711b
update import
Miguel7373 Jan 7, 2025
c12ef1a
backend formatting changes
Miguel7373 Jan 7, 2025
ab1b1d3
remove 404 in the completed get request
Miguel7373 Jan 7, 2025
f33357a
apply backend formatter
Miguel7373 Jan 7, 2025
7f67420
inclued test for the completed comment
Miguel7373 Jan 7, 2025
c0ccc8a
Fix naming of endpoint and the corresponding documentation and add be…
ManuelMoeri Jan 8, 2025
89427ee
Rename service with typo, fix naming of e2e tests and change some log…
ManuelMoeri Jan 8, 2025
7fd7d6d
Try to generalize cypress testing
ManuelMoeri Jan 8, 2025
eb0144f
Fix e2e tests by using correct state for selecting the objectives
ManuelMoeri Jan 9, 2025
3acd7f5
Finish abstraction of completeDialog in e2e testing
ManuelMoeri Jan 9, 2025
d24859f
add unit test for completed
Miguel7373 Jan 9, 2025
0b1d06b
add right role check for get of completed
Miguel7373 Jan 9, 2025
3f1e4ca
fix unit test to use right state
Miguel7373 Jan 9, 2025
c88e2dc
fix completed is undefined error by not loading in always
Miguel7373 Jan 13, 2025
ed39406
fix takeUntilDestroy
Miguel7373 Jan 13, 2025
8526018
change state enum to no longer hold svg
Miguel7373 Jan 13, 2025
dcd362f
change backend and add tests for completed services
Miguel7373 Jan 13, 2025
5d08d55
formatting changes and remove useless methode
Miguel7373 Jan 13, 2025
7413fd9
change name of completed service file
Miguel7373 Jan 13, 2025
a191cb4
move exception throwing to business service to prevent error while de…
Miguel7373 Jan 13, 2025
6f52826
formatt backend
Miguel7373 Jan 13, 2025
2f7c663
update backend tests for get completed logic
Miguel7373 Jan 14, 2025
a3f4081
formatt backend changes
Miguel7373 Jan 14, 2025
cfbcc41
remove empty test
Miguel7373 Jan 14, 2025
707cb04
fix logic so title of completed comment does not get shown if there i…
Miguel7373 Jan 14, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
Original file line number Diff line number Diff line change
@@ -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);
Miguel7373 marked this conversation as resolved.
Show resolved Hide resolved
}
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -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;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,6 @@ public String getModelName() {
}

public Completed getCompletedByObjectiveId(Long objectiveId) {
return getRepository().findByObjectiveId(objectiveId);
return getRepository().findByObjectiveId(objectiveId).orElse(null);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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"))
Miguel7373 marked this conversation as resolved.
Show resolved Hide resolved
.when(completedAuthorizationService)
.getCompletedByObjectiveId(anyLong());

mvc
.perform(get("/api/v2/completed/1000").with(SecurityMockMvcRequestPostProcessors.csrf()))
.andExpect(MockMvcResultMatchers.status().isNotFound());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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());
}
}
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -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);
Expand All @@ -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);
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}
59 changes: 16 additions & 43 deletions frontend/cypress/e2e/objective.cy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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)
ManuelMoeri marked this conversation as resolved.
Show resolved Hide resolved
.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', () => {
Expand Down
Original file line number Diff line number Diff line change
@@ -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');
}
}
22 changes: 22 additions & 0 deletions frontend/cypress/support/helper/dom-helper/pages/overviewPage.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 = {
Expand Down Expand Up @@ -187,6 +188,27 @@ export default class CyOverviewPage extends Page {
return new ObjectiveDialog();
}

completeObjective(title: string) {
this.addObjective()
.fillObjectiveTitle(title)
.submit();
ManuelMoeri marked this conversation as resolved.
Show resolved Hide resolved

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();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand All @@ -54,6 +53,12 @@ <h3 class="mb-1">Beschrieb</h3>
Objective bearbeiten
</button>
</div>
<ng-template #isCompleted>
<h3 [attr.data-testId]="'completed-comment'" class="mb-1">Abschlusskommentar</h3>
<div class="linebreak">
<p>{{ (completed | async)?.comment }}</p>
</div>
</ng-template>
</section>
</div>
</ng-container>
Expand Down
Loading
Loading