From c9791b01b8b72b711025903276f2a1a30e3e85a2 Mon Sep 17 00:00:00 2001 From: Holger Hagen <19706592+holgerhagen@users.noreply.github.com> Date: Mon, 9 Mar 2020 17:24:14 +0100 Subject: [PATCH] TSK-1029: Proper distinction between delete and markForFeletion for workbaskets --- .../workbasket/api/WorkbasketService.java | 3 +- .../taskana/rest/WorkbasketController.java | 24 +++++-- ...WorkbasketControllerRestDocumentation.java | 8 +-- .../rest/WorkbasketControllerIntTest.java | 64 +++++++++++-------- .../src/test/resources/asciidoc/rest-api.adoc | 19 +++++- .../workbasket-information.component.ts | 10 +-- .../services/workbasket/workbasket.service.ts | 2 +- 7 files changed, 83 insertions(+), 47 deletions(-) diff --git a/lib/taskana-core/src/main/java/pro/taskana/workbasket/api/WorkbasketService.java b/lib/taskana-core/src/main/java/pro/taskana/workbasket/api/WorkbasketService.java index 01d924be7..882fa2f9e 100644 --- a/lib/taskana-core/src/main/java/pro/taskana/workbasket/api/WorkbasketService.java +++ b/lib/taskana-core/src/main/java/pro/taskana/workbasket/api/WorkbasketService.java @@ -292,7 +292,8 @@ public interface WorkbasketService { * Deletes the workbasket by the given ID of it. * * @param workbasketId Id of the workbasket which should be deleted. - * @return true if the workbasket is marked for deletion. False in another case. + * @return true if the workbasket was deleted successfully. + * false if the workbasket is marked for deletion. * @throws NotAuthorizedException if the current user got no permissions for this interaction. * @throws WorkbasketNotFoundException if the workbasket does not exist. * @throws WorkbasketInUseException if the workbasket does contain task-content. diff --git a/rest/taskana-rest-spring/src/main/java/pro/taskana/rest/WorkbasketController.java b/rest/taskana-rest-spring/src/main/java/pro/taskana/rest/WorkbasketController.java index e50563a6c..f5a39ebc8 100644 --- a/rest/taskana-rest-spring/src/main/java/pro/taskana/rest/WorkbasketController.java +++ b/rest/taskana-rest-spring/src/main/java/pro/taskana/rest/WorkbasketController.java @@ -140,14 +140,24 @@ public class WorkbasketController extends AbstractPagingController { @DeleteMapping(path = Mapping.URL_WORKBASKET_ID) @Transactional(rollbackFor = Exception.class, noRollbackFor = WorkbasketNotFoundException.class) - public ResponseEntity markWorkbasketForDeletion( + public ResponseEntity deleteWorkbasket( @PathVariable(value = "workbasketId") String workbasketId) throws NotAuthorizedException, InvalidArgumentException, WorkbasketNotFoundException, WorkbasketInUseException { LOGGER.debug("Entry to markWorkbasketForDeletion(workbasketId= {})", workbasketId); - // http status code accepted because workbaskets will not be deleted immediately - ResponseEntity response = - ResponseEntity.accepted().body(workbasketService.deleteWorkbasket(workbasketId)); + ResponseEntity response; + + boolean workbasketDeleted = workbasketService.deleteWorkbasket(workbasketId); + + if (workbasketDeleted) { + LOGGER.debug("Workbasket successfully deleted."); + response = ResponseEntity.noContent().build(); + } else { + LOGGER.debug( + "Workbasket was only marked for deletion and will be physically deleted later on."); + response = ResponseEntity.accepted().build(); + } + LOGGER.debug("Exit from markWorkbasketForDeletion(), returning {}", response); return response; } @@ -203,7 +213,8 @@ public class WorkbasketController extends AbstractPagingController { return result; } - @GetMapping(path = Mapping.URL_WORKBASKET_ID_ACCESSITEMS, + @GetMapping( + path = Mapping.URL_WORKBASKET_ID_ACCESSITEMS, produces = MediaTypes.HAL_JSON_UTF8_VALUE) @Transactional(readOnly = true, rollbackFor = Exception.class) public ResponseEntity getWorkbasketAccessItems( @@ -253,7 +264,8 @@ public class WorkbasketController extends AbstractPagingController { return response; } - @GetMapping(path = Mapping.URL_WORKBASKET_ID_DISTRIBUTION, + @GetMapping( + path = Mapping.URL_WORKBASKET_ID_DISTRIBUTION, produces = MediaTypes.HAL_JSON_UTF8_VALUE) @Transactional(readOnly = true, rollbackFor = Exception.class) public ResponseEntity getDistributionTargets( diff --git a/rest/taskana-rest-spring/src/test/java/pro/taskana/doc/api/WorkbasketControllerRestDocumentation.java b/rest/taskana-rest-spring/src/test/java/pro/taskana/doc/api/WorkbasketControllerRestDocumentation.java index 425354b2f..b568f13ec 100644 --- a/rest/taskana-rest-spring/src/test/java/pro/taskana/doc/api/WorkbasketControllerRestDocumentation.java +++ b/rest/taskana-rest-spring/src/test/java/pro/taskana/doc/api/WorkbasketControllerRestDocumentation.java @@ -483,15 +483,15 @@ class WorkbasketControllerRestDocumentation extends BaseRestDocumentation { } @Test - void markWorkbasketForDeletionDocTest() throws Exception { + void deleteWorkbasketDocTest() throws Exception { this.mockMvc .perform( RestDocumentationRequestBuilders.delete( restHelper.toUrl( - Mapping.URL_WORKBASKET_ID, "WBI:100000000000000000000000000000000005")) + Mapping.URL_WORKBASKET_ID, "WBI:100000000000000000000000000000000008")) .header("Authorization", "Basic YWRtaW46YWRtaW4=")) - .andExpect(MockMvcResultMatchers.status().isAccepted()) - .andDo(MockMvcRestDocumentation.document("MarkWorkbasketForDeletionDocTest")); + .andExpect(MockMvcResultMatchers.status().isNoContent()) + .andDo(MockMvcRestDocumentation.document("DeleteWorkbasketDocTest")); } @Test diff --git a/rest/taskana-rest-spring/src/test/java/pro/taskana/rest/WorkbasketControllerIntTest.java b/rest/taskana-rest-spring/src/test/java/pro/taskana/rest/WorkbasketControllerIntTest.java index eaca79f42..21fd2fdad 100644 --- a/rest/taskana-rest-spring/src/test/java/pro/taskana/rest/WorkbasketControllerIntTest.java +++ b/rest/taskana-rest-spring/src/test/java/pro/taskana/rest/WorkbasketControllerIntTest.java @@ -90,10 +90,9 @@ class WorkbasketControllerIntTest { @Test void testThrowsExceptionIfInvalidFilterIsUsed() { - assertThatThrownBy( - () -> - template.exchange( - restHelper.toUrl(Mapping.URL_WORKBASKET) + "?invalid=PERSONAL", + assertThatThrownBy(() -> + template.exchange( + restHelper.toUrl(Mapping.URL_WORKBASKET) + "?invalid=PERSONAL", HttpMethod.GET, restHelper.defaultRequest(), ParameterizedTypeReference.forType(WorkbasketSummaryListResource.class))) @@ -126,14 +125,13 @@ class WorkbasketControllerIntTest { workbasketResource.setOwner("Joerg"); workbasketResource.setModified(String.valueOf(Instant.now())); - assertThatThrownBy( - () -> - template.exchange( - restHelper.toUrl(Mapping.URL_WORKBASKET_ID, workbasketId), - HttpMethod.PUT, - new HttpEntity<>( - mapper.writeValueAsString(workbasketResource), restHelper.getHeaders()), - ParameterizedTypeReference.forType(WorkbasketResource.class))) + assertThatThrownBy(() -> + template.exchange( + restHelper.toUrl(Mapping.URL_WORKBASKET_ID, workbasketId), + HttpMethod.PUT, + new HttpEntity<>( + mapper.writeValueAsString(workbasketResource), restHelper.getHeaders()), + ParameterizedTypeReference.forType(WorkbasketResource.class))) .extracting(ex -> ((HttpClientErrorException) ex).getStatusCode()) .isEqualTo(HttpStatus.CONFLICT); } @@ -143,13 +141,12 @@ class WorkbasketControllerIntTest { String workbasketId = "WBI:100004857400039500000999999999999999"; - assertThatThrownBy( - () -> - template.exchange( - restHelper.toUrl(Mapping.URL_WORKBASKET_ID, workbasketId), - HttpMethod.GET, - new HttpEntity(restHelper.getHeaders()), - ParameterizedTypeReference.forType(WorkbasketResource.class))) + assertThatThrownBy(() -> + template.exchange( + restHelper.toUrl(Mapping.URL_WORKBASKET_ID, workbasketId), + HttpMethod.GET, + new HttpEntity(restHelper.getHeaders()), + ParameterizedTypeReference.forType(WorkbasketResource.class))) .isInstanceOf(HttpClientErrorException.class) .extracting(ex -> ((HttpClientErrorException) ex).getStatusCode()) .isEqualTo(HttpStatus.NOT_FOUND); @@ -175,12 +172,8 @@ class WorkbasketControllerIntTest { assertThat(response.getBody().getLink(Link.REL_SELF).getHref().endsWith(parameters)).isTrue(); } - /** - * Bug Ticket TSK-1029. Businessadmin is allowed to delete any workbasket ticket without user - * related access restrictions. - */ @Test - void testDeleteWorkbasketAsBusinessAdminWithoutExplicitReadPermission() { + void testMarkWorkbasketForDeletionAsBusinessAdminWithoutExplicitReadPermission() { String workbasketID = "WBI:100000000000000000000000000000000005"; @@ -193,6 +186,22 @@ class WorkbasketControllerIntTest { assertThat(response.getStatusCode()).isEqualTo(HttpStatus.ACCEPTED); } + @Test + void statusCode423ShouldBeReturnedIfWorkbasketContainsNonCompletedTasks() { + String workbasketWithNonCompletedTasks = "WBI:100000000000000000000000000000000004"; + + assertThatThrownBy(() -> + template.exchange( + restHelper.toUrl(Mapping.URL_WORKBASKET_ID, workbasketWithNonCompletedTasks), + HttpMethod.DELETE, + new HttpEntity<>(restHelper.getHeadersBusinessAdmin()), + Void.class)) + .isInstanceOf(HttpClientErrorException.class) + .extracting(ex -> ((HttpClientErrorException) ex).getStatusCode()) + .isEqualTo(HttpStatus.LOCKED); + + } + @Test void testRemoveWorkbasketAsDistributionTarget() { ResponseEntity response = @@ -213,9 +222,9 @@ class WorkbasketControllerIntTest { ParameterizedTypeReference.forType(DistributionTargetListResource.class)); assertThat(response2.getStatusCode()).isEqualTo(HttpStatus.OK); assertThat( - response2.getBody().getContent().stream() - .map(DistributionTargetResource::getWorkbasketId) - .noneMatch(id -> (id.equals("WBI:100000000000000000000000000000000007")))) + response2.getBody().getContent().stream() + .map(DistributionTargetResource::getWorkbasketId) + .noneMatch("WBI:100000000000000000000000000000000007"::equals)) .isTrue(); } @@ -248,5 +257,4 @@ class WorkbasketControllerIntTest { .isEqualTo(MediaTypes.HAL_JSON_UTF8_VALUE); assertThat(response.getBody().getContent()).hasSize(4); } - } diff --git a/rest/taskana-rest-spring/src/test/resources/asciidoc/rest-api.adoc b/rest/taskana-rest-spring/src/test/resources/asciidoc/rest-api.adoc index cc776fbd8..80bcdb9bb 100644 --- a/rest/taskana-rest-spring/src/test/resources/asciidoc/rest-api.adoc +++ b/rest/taskana-rest-spring/src/test/resources/asciidoc/rest-api.adoc @@ -46,6 +46,9 @@ use of HTTP status codes. | `201 Created` | The request completed successfully und create the new resource. +| `202 Accepted` +| The request could not be processed immediatly but will be carried out later. + | `204 No Content` | The request completed successfully and there is no content to send in the response payload. @@ -72,6 +75,9 @@ use of HTTP status codes. | `415 Unsupported Media Type` | The content of the request can't be understood due to being in an unsupported media-type. + +| `423 Locked` +| The resource is currently locked and cannot be modified. |=== == Common Fields @@ -497,15 +503,22 @@ Therefore for the response fields you can refer to the structure of the < { this.requestInProgressService.setRequestInProgress(false); this.workbasketService.triggerWorkBasketSaved(); - if (response) { + if (response.status === 202) { // new Key ERROR_TYPES.MARK_ERR + // TODO: message changed this.generalModalService.triggerMessage( - new MessageModal('There was an error marking workbasket for deletion', - 'It not possible to mark the workbasket for deletion, It has been deleted.') + new MessageModal('Workbasket was marked for deletion.', + `The Workbasket ${this.workbasket.workbasketId} still contains completed tasks and could not be deleted. Instead is was marked for deletion and will be deleted automatically as soon as the completed tasks are deleted from the database.`) ); } else { // new Key ALERT_TYPES.SUCCESS_ALERT_12 + // TODO: message changed this.alertService.triggerAlert( - new AlertModel(AlertType.SUCCESS, `The Workbasket ${this.workbasket.workbasketId} has been marked for deletion`) + new AlertModel(AlertType.SUCCESS, `The Workbasket ${this.workbasket.workbasketId} has been deleted.`) ); } this.router.navigate(['taskana/administration/workbaskets']); diff --git a/web/src/app/shared/services/workbasket/workbasket.service.ts b/web/src/app/shared/services/workbasket/workbasket.service.ts index 4e6775b89..b1f90eafd 100644 --- a/web/src/app/shared/services/workbasket/workbasket.service.ts +++ b/web/src/app/shared/services/workbasket/workbasket.service.ts @@ -89,7 +89,7 @@ export class WorkbasketService { // delete markWorkbasketForDeletion(url: string): Observable { return this.httpClient - .delete(url); + .delete(url, {observe: 'response'}); } // GET