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

Bug/1255 Duplicating a objective does not bring all properties of the KeyResults #1294

Open
wants to merge 27 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
0510cc6
Refactor duplicating methods to include values of keyResult
ManuelMoeri Jan 13, 2025
1f664d5
Modify duplicateObjectiveDto to only include ids
ManuelMoeri Jan 13, 2025
ff50c43
fix actionlist shared refrence error
nevio18324 Jan 14, 2025
b684569
fix action being asigned to the wrong action
nevio18324 Jan 14, 2025
654b0ce
run formatter
nevio18324 Jan 14, 2025
ee07995
fix duplicate tests
nevio18324 Jan 14, 2025
17102de
make new method to duplicate Actionlists
nevio18324 Jan 14, 2025
19ac64a
make shouldDupllicateActionList check if every attribute was copied e…
nevio18324 Jan 14, 2025
3978c61
fix duplicate keyresult tests
nevio18324 Jan 14, 2025
4640678
run formatter
nevio18324 Jan 14, 2025
07c6238
write new e2e test testing if all keyresult attributes get duplicated
nevio18324 Jan 14, 2025
f090a48
fix duplicate so you can choose which key results you wanna duplicate
nevio18324 Jan 14, 2025
7f8184e
remove not needed commented line
nevio18324 Jan 14, 2025
b9d41c8
remove id out of pathvariable for duplicate
nevio18324 Jan 14, 2025
9c0a6a1
run formatter
nevio18324 Jan 14, 2025
c788007
fix id still being passed as a param in objective form
nevio18324 Jan 14, 2025
5a855e9
fix duplicateObjective mapping
nevio18324 Jan 14, 2025
af53c4b
remove unused import
nevio18324 Jan 14, 2025
d92b109
remove outdated comment
nevio18324 Jan 14, 2025
211e56c
revert formatter changes
nevio18324 Jan 14, 2025
8bfb604
remove unnecessary wait
nevio18324 Jan 14, 2025
70d3ebd
remove action list attribute
nevio18324 Jan 14, 2025
047be01
Finish logic of duplicating objectives with the action plan
ManuelMoeri Jan 15, 2025
5d6a6cb
Delete unused test, add two new ones and apply formatters
ManuelMoeri Jan 15, 2025
c917f59
Fix tests and enhance logic
ManuelMoeri Jan 15, 2025
d1866dc
Format code
ManuelMoeri Jan 15, 2025
cc94827
Resolve conversations
ManuelMoeri Jan 15, 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 @@ -11,7 +11,6 @@
import ch.puzzle.okr.mapper.keyresult.KeyResultMapper;
import ch.puzzle.okr.models.Action;
import ch.puzzle.okr.models.Objective;
import ch.puzzle.okr.models.keyresult.KeyResult;
import ch.puzzle.okr.service.authorization.ActionAuthorizationService;
import ch.puzzle.okr.service.authorization.ObjectiveAuthorizationService;
import io.swagger.v3.oas.annotations.Operation;
Expand Down Expand Up @@ -100,17 +99,13 @@ public ResponseEntity<ObjectiveDto> createObjective(@io.swagger.v3.oas.annotatio
@Operation(summary = "Duplicate Objective", description = "Duplicate a given Objective")
@ApiResponses(value = { @ApiResponse(responseCode = "201", description = "Duplicated a given Objective", content = {
@Content(mediaType = "application/json", schema = @Schema(implementation = ObjectiveDto.class)) }) })
@PostMapping("/{id}")
public ResponseEntity<ObjectiveDto> duplicateObjective(@Parameter(description = "The ID for duplicating an Objective.", required = true)
@PathVariable Long id, @io.swagger.v3.oas.annotations.parameters.RequestBody(description = "The Objective which should be duplicated as JSON", required = true) @RequestBody DuplicateObjectiveDto duplicateObjectiveDto) {
@PostMapping("/duplicate")
public ResponseEntity<ObjectiveDto> duplicateObjective(@io.swagger.v3.oas.annotations.parameters.RequestBody(description = "The Objective which should be duplicated as JSON", required = true)
@RequestBody DuplicateObjectiveDto duplicateObjectiveDto) {
Objective objective = objectiveMapper.toObjective(duplicateObjectiveDto.objective());
List<KeyResult> keyResults = duplicateObjectiveDto
.keyResults()
.stream()
.map(keyResultMapper::toKeyResult)
.toList();
List<Long> keyResultIds = duplicateObjectiveDto.keyResultIds();
ObjectiveDto duplicatedObjectiveDto = objectiveMapper
.toDto(objectiveAuthorizationService.duplicateEntity(id, objective, keyResults));
.toDto(objectiveAuthorizationService.duplicateEntity(objective, keyResultIds));
return ResponseEntity.status(HttpStatus.CREATED).body(duplicatedObjectiveDto);
}

Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package ch.puzzle.okr.dto;

import ch.puzzle.okr.dto.keyresult.KeyResultDto;
import java.util.List;

public record DuplicateObjectiveDto(ObjectiveDto objective, List<KeyResultDto> keyResults) {
public record DuplicateObjectiveDto(ObjectiveDto objective, List<Long> keyResultIds) {
}
Original file line number Diff line number Diff line change
@@ -1,14 +1,12 @@
package ch.puzzle.okr.models.keyresult;

import ch.puzzle.okr.models.MessageKey;
import ch.puzzle.okr.models.Objective;
import ch.puzzle.okr.models.User;
import ch.puzzle.okr.models.WriteableInterface;
import ch.puzzle.okr.models.*;
import jakarta.persistence.*;
import jakarta.validation.constraints.NotBlank;
import jakarta.validation.constraints.NotNull;
import jakarta.validation.constraints.Size;
import java.time.LocalDateTime;
import java.util.List;
import java.util.Objects;

@Entity
Expand Down Expand Up @@ -206,6 +204,7 @@ public abstract static class Builder<T> {
private LocalDateTime createdOn;
private LocalDateTime modifiedOn;
private final String keyResultType;
private List<Action> actionList;

protected Builder(String keyResultType) {
this.keyResultType = keyResultType;
Expand Down Expand Up @@ -256,6 +255,11 @@ public T withModifiedOn(LocalDateTime modifiedOn) {
return (T) this;
}

public T withActionList(List<Action> actionList) {
this.actionList = actionList;
ManuelMoeri marked this conversation as resolved.
Show resolved Hide resolved
return (T) this;
}

public abstract KeyResult build();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,10 @@ public ObjectiveAuthorizationService(ObjectiveBusinessService objectiveBusinessS
super(objectiveBusinessService, authorizationService);
}

public Objective duplicateEntity(Long id, Objective objective, List<KeyResult> keyResults) {
public Objective duplicateEntity(Objective objective, List<Long> keyResultIds) {
AuthorizationUser authorizationUser = getAuthorizationService().updateOrAddAuthorizationUser();
hasRoleCreateOrUpdate(objective, authorizationUser);
return getBusinessService().duplicateObjective(id, objective, authorizationUser, keyResults);
return getBusinessService().duplicateObjective(objective, authorizationUser, keyResultIds);
}

public List<KeyResult> getAllKeyResultsByObjective(Long objectiveId) {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
package ch.puzzle.okr.service.business;

import ch.puzzle.okr.models.Action;
import ch.puzzle.okr.models.keyresult.KeyResult;
import ch.puzzle.okr.service.persistence.ActionPersistenceService;
import ch.puzzle.okr.service.validation.ActionValidationService;
import jakarta.transaction.Transactional;
import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
import org.springframework.stereotype.Service;

Expand Down Expand Up @@ -74,4 +76,25 @@ public void deleteEntityById(Long id) {
public void deleteEntitiesByKeyResultId(Long keyResultId) {
getActionsByKeyResultId(keyResultId).forEach(action -> deleteEntityById(action.getId()));
}

public List<Action> createDuplicates(KeyResult oldKeyResult, KeyResult newKeyResult) {
List<Action> actionList = actionPersistenceService
.getActionsByKeyResultIdOrderByPriorityAsc(oldKeyResult.getId());
if (actionList == null) {
return Collections.emptyList();
}

return actionList.stream().map(action -> {
Action newAction = Action.Builder
.builder()
.withAction(action.getActionPoint())
.isChecked(action.isChecked())
.withPriority(action.getPriority())
.withVersion(action.getVersion())
.withKeyResult(newKeyResult)
.build();
validator.validate(newAction);
return newAction;
}).toList();
}
}
Original file line number Diff line number Diff line change
@@ -1,9 +1,12 @@
package ch.puzzle.okr.service.business;

import ch.puzzle.okr.models.Action;
import ch.puzzle.okr.models.Objective;
import ch.puzzle.okr.models.authorization.AuthorizationUser;
import ch.puzzle.okr.models.checkin.CheckIn;
import ch.puzzle.okr.models.keyresult.KeyResult;
import ch.puzzle.okr.models.keyresult.KeyResultMetric;
import ch.puzzle.okr.models.keyresult.KeyResultOrdinal;
import ch.puzzle.okr.models.keyresult.KeyResultWithActionList;
import ch.puzzle.okr.service.persistence.KeyResultPersistenceService;
import ch.puzzle.okr.service.validation.KeyResultValidationService;
Expand Down Expand Up @@ -137,4 +140,30 @@ private boolean isKeyResultTypeChangeable(Long id) {
public List<KeyResult> getKeyResultsOwnedByUser(long id) {
return keyResultPersistenceService.getKeyResultsOwnedByUser(id);
}

public KeyResult makeCopyOfKeyResultMetric(KeyResult keyResult, Objective duplicatedObjective) {
return KeyResultMetric.Builder
.builder() //
.withObjective(duplicatedObjective) //
.withTitle(keyResult.getTitle()) //
.withDescription(keyResult.getDescription()) //
.withOwner(keyResult.getOwner()) //
.withUnit(((KeyResultMetric) keyResult).getUnit()) //
.withBaseline(((KeyResultMetric) keyResult).getBaseline()) //
.withStretchGoal(((KeyResultMetric) keyResult).getStretchGoal()) //
.build();
}

public KeyResult makeCopyOfKeyResultOrdinal(KeyResult keyResult, Objective duplicatedObjective) {
return KeyResultOrdinal.Builder
.builder() //
.withObjective(duplicatedObjective) //
.withTitle(keyResult.getTitle()) //
.withDescription(keyResult.getDescription()) //
.withOwner(keyResult.getOwner()) //
.withCommitZone(((KeyResultOrdinal) keyResult).getCommitZone()) //
.withTargetZone(((KeyResultOrdinal) keyResult).getTargetZone()) //
.withStretchZone(((KeyResultOrdinal) keyResult).getStretchZone()) //
.build();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,13 @@
import static ch.puzzle.okr.Constants.KEY_RESULT_TYPE_METRIC;
import static ch.puzzle.okr.Constants.KEY_RESULT_TYPE_ORDINAL;

import ch.puzzle.okr.models.Action;
import ch.puzzle.okr.models.Objective;
import ch.puzzle.okr.models.authorization.AuthorizationUser;
import ch.puzzle.okr.models.keyresult.KeyResult;
import ch.puzzle.okr.models.keyresult.KeyResultMetric;
import ch.puzzle.okr.models.keyresult.KeyResultOrdinal;
import ch.puzzle.okr.service.persistence.ActionPersistenceService;
import ch.puzzle.okr.service.persistence.ObjectivePersistenceService;
import ch.puzzle.okr.service.validation.ObjectiveValidationService;
import jakarta.transaction.Transactional;
Expand All @@ -26,15 +28,21 @@ public class ObjectiveBusinessService implements BusinessServiceInterface<Long,
private final ObjectiveValidationService validator;
private final KeyResultBusinessService keyResultBusinessService;
private final CompletedBusinessService completedBusinessService;
private final ActionPersistenceService actionPersistenceService;
private final ActionBusinessService actionBusinessService;

public ObjectiveBusinessService(@Lazy KeyResultBusinessService keyResultBusinessService,
ObjectiveValidationService validator,
ObjectivePersistenceService objectivePersistenceService,
CompletedBusinessService completedBusinessService) {
CompletedBusinessService completedBusinessService,
ActionPersistenceService actionPersistenceService,
ActionBusinessService actionBusinessService) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

action business service should be enough there should be no need to use action persisstence service here

this.keyResultBusinessService = keyResultBusinessService;
this.validator = validator;
this.objectivePersistenceService = objectivePersistenceService;
this.completedBusinessService = completedBusinessService;
this.actionPersistenceService = actionPersistenceService;
this.actionBusinessService = actionBusinessService;
}

private static boolean hasQuarterChanged(Objective objective, Objective savedObjective) {
Expand Down Expand Up @@ -107,65 +115,45 @@ public Objective createEntity(Objective objective, AuthorizationUser authorizati
}

/**
* Create a new Objective (with a new ID) and copy from a source Objective
* (identified by id) the KeyResults. The CheckIns are not copied.
* Create a new Objective and copy the KeyResults from the source Objective. The
* CheckIns are not copied.
*
* @param id
* ID of the source Objective
* @param objective
* New Objective with no KeyResults
* @param authorizationUser
* AuthorizationUser
* @param keyResults
* KeyResults to copy
*
* @param keyResultIds
* KeyResultIds to get and copy
* @return New Objective with copied KeyResults form the source Objective
*/
@Transactional
public Objective duplicateObjective(Long id, Objective objective, AuthorizationUser authorizationUser,
List<KeyResult> keyResults) {
public Objective duplicateObjective(Objective objective, AuthorizationUser authorizationUser,
List<Long> keyResultIds) {
Objective duplicatedObjective = createEntity(objective, authorizationUser);
for (KeyResult keyResult : keyResults) {
duplicateKeyResult(authorizationUser, keyResult, duplicatedObjective);
for (Long keyResult : keyResultIds) {
duplicateKeyResult(authorizationUser,
keyResultBusinessService.getEntityById(keyResult),
duplicatedObjective);
}
return duplicatedObjective;
}

private void duplicateKeyResult(AuthorizationUser authorizationUser, KeyResult keyResult,
Objective duplicatedObjective) {
public void duplicateKeyResult(AuthorizationUser authorizationUser, KeyResult keyResult,
Objective duplicatedObjective) {
KeyResult newKeyResult = null;

if (keyResult.getKeyResultType().equals(KEY_RESULT_TYPE_METRIC)) {
KeyResult keyResultMetric = makeCopyOfKeyResultMetric(keyResult, duplicatedObjective);
keyResultBusinessService.createEntity(keyResultMetric, authorizationUser);
KeyResult keyResultMetric = keyResultBusinessService.makeCopyOfKeyResultMetric(keyResult, duplicatedObjective);
newKeyResult = keyResultBusinessService.createEntity(keyResultMetric, authorizationUser);
} else if (keyResult.getKeyResultType().equals(KEY_RESULT_TYPE_ORDINAL)) {
KeyResult keyResultOrdinal = makeCopyOfKeyResultOrdinal(keyResult, duplicatedObjective);
keyResultBusinessService.createEntity(keyResultOrdinal, authorizationUser);
KeyResult keyResultOrdinal = keyResultBusinessService.makeCopyOfKeyResultOrdinal(keyResult, duplicatedObjective);
newKeyResult = keyResultBusinessService.createEntity(keyResultOrdinal, authorizationUser);
} else {
throw new UnsupportedOperationException("Unsupported KeyResultType: " + keyResult.getKeyResultType());
}
}

private KeyResult makeCopyOfKeyResultMetric(KeyResult keyResult, Objective duplicatedObjective) {
return KeyResultMetric.Builder
.builder() //
.withObjective(duplicatedObjective) //
.withTitle(keyResult.getTitle()) //
.withDescription(keyResult.getDescription()) //
.withOwner(keyResult.getOwner()) //
.withUnit(((KeyResultMetric) keyResult).getUnit()) //
.withBaseline(0D) //
.withStretchGoal(1D) //
.build();
}

private KeyResult makeCopyOfKeyResultOrdinal(KeyResult keyResult, Objective duplicatedObjective) {
return KeyResultOrdinal.Builder
.builder() //
.withObjective(duplicatedObjective) //
.withTitle(keyResult.getTitle()) //
.withDescription(keyResult.getDescription()) //
.withOwner(keyResult.getOwner()) //
.withCommitZone("-") //
.withTargetZone("-") //
.withStretchZone("-") //
.build();
List<Action> copiedActions = actionBusinessService.createDuplicates(keyResult, newKeyResult);
copiedActions.forEach(actionPersistenceService::save);
}

@Transactional
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ class ObjectiveControllerIT {
private static final String URL_BASE_OBJECTIVE = "/api/v2/objectives";
private static final String URL_OBJECTIVE_5 = "/api/v2/objectives/5";
private static final String URL_OBJECTIVE_10 = "/api/v2/objectives/10";
private static final String URL_DUPLICATE_OBJECTIVE_5 = "/api/v2/objectives/5";
private static final String URL_DUPLICATE_OBJECTIVE_5 = "/api/v2/objectives/duplicate";
private static final String JSON = """
{
"title": "FullObjective", "ownerId": 1, "ownerFirstname": "Bob", "ownerLastname": "Kaufmann",
Expand All @@ -62,7 +62,7 @@ class ObjectiveControllerIT {
"objective": %s,
"keyResults": [%s,%s]
}
""".formatted(JSON, KEY_RESULT_METRIC_JSON, KEY_RESULT_ORDINAL_JSON);
""".formatted(JSON, 1L, 2L);

private static final String CREATE_NEW_OBJECTIVE = """
{
Expand Down Expand Up @@ -341,16 +341,13 @@ void shouldThrowExceptionWhenObjectiveWithIdCantBeFoundWhileDeleting() throws Ex
.andExpect(MockMvcResultMatchers.status().isNotFound());
}

@DisplayName("Should return is created when an ojective was duplicated")
@DisplayName("Should return is created when an objective was duplicated")
@Test
void shouldReturnIsCreatedWhenObjectiveWasDuplicated() throws Exception {
BDDMockito
.given(objectiveAuthorizationService.duplicateEntity(anyLong(), any(), anyList()))
.willReturn(objective1);
BDDMockito.given(keyResultMapper.toDto(any(KeyResultMetric.class), any())).willReturn(keyResultMetricDto);
BDDMockito.given(keyResultMapper.toDto(any(KeyResultOrdinal.class), any())).willReturn(keyResultOrdinalDto);
BDDMockito.given(objectiveAuthorizationService.getAuthorizationService()).willReturn(authorizationService);
BDDMockito.given(objectiveMapper.toDto(objective1)).willReturn(objective1Dto);
BDDMockito.given(objectiveMapper.toDto(any())).willReturn(objective1Dto);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

be careful using any here, could lead to unexpected behavior. Generally it's considered to be best practice to mock as precise as possible.


mvc
.perform(post(URL_DUPLICATE_OBJECTIVE_5)
Expand Down
Loading
Loading