EES-4046: Add functionality to delete draft releases

[tomjonesdev]

This PR adds the functionality for BAU users to delete draft releases which have not yet been approved. This allows users to tidy up clutter within the Admin portal, database and storage by removing entities which are no longer necessary.

This functionality has been merged with the existing code to "cancel" (i.e. soft-delete) release amendments, the difference being that non-amendment drafts are removed using a hard-delete.

To avoid over-complicating the existing method with additional conditional trees, some of the code has been split into private sub-functions, primarily for readability, but also for potential future expansion/reusability.

[mmyoungman]

I think with DeleteSpecificReleaseAuthorizationHandler set up as it is, it's possible for a BAU user to delete an original release that is currently set to HigherLevelReview, and when they do it'll end up calling SoftDeleteForAmendment. Although fixing that should probably happen in the auth handler, not here.

I think I'd remove the ApprovalStatus check in this if. I'd rely on CheckCanDeleteReleaseVersion to prevent any approved release from ever making it this far, and so there is no reason to check it again here.

[tomjonesdev]

Removing this causes a ReleaseServiceTest to fail because it doesn't (by design) go through the authorisation handler. I'd argue that the check should remain as this is re-usable business logic, i.e. even though one of the implementations includes the restriction already, there's a chance that another implementation might not.

[mmyoungman]

My thought is that if we're checking the approval status here and not relying on CheckCanDeleteReleaseVersion, for consistency we will also want to check it for all the preceeding OnSuccessDos.

We could do something like this instead:

.OnSuccess(_userService.CheckCanDeleteReleaseVersion)
.OnSuccessDo(releaseVersion =>
{
                    if (releaseVersion.ApprovalStatus != ReleaseApprovalStatus.Draft)
                    {
                        throw new Exception("Can only delete draft releases");
                    }
})
.OnSuccessDo(async () => await _processorClient.BulkDeleteDataSetVersions(releaseVersionId))
[....]

Then the approval status is ensured across all the method calls we do, not just the final removal of the release version itself.

[mmyoungman]

I'm still keen on removing the approval status check here - although I will accept moving it higher up. My personal opinion is that this check is redundant - for better or worse, we haven't generally kept business logic separate.

If we wanted this logic to work without the permission check so it could function as reusable business logic, I believe the logic is currently wrong. Without the permission check, it is possible to remove cache/data/general files of an approved release. And while the database entries wouldn't be removed for an approved release, they would be soft deleted.

So I'm personally inclined to remove the check entirely, but if you want this code to function as reusable business logic, then I think we need to check it's a draft release first - before calling BulkDeleteDataSetVersions, DeleteCacheFolderAsync, etc. - like the code sample I gave in the previous comment.

[mmyoungman]

Might want a frontender to give the frontend parts a look.

[mmyoungman]

I'm still keen on removing the approval status check here - although I will accept moving it higher up. My personal opinion is that this check is redundant - for better or worse, we haven't generally kept business logic separate.

If we wanted this logic to work without the permission check so it could function as reusable business logic, I believe the logic is currently wrong. Without the permission check, it is possible to remove cache/data/general files of an approved release. And while the database entries wouldn't be removed for an approved release, they would be soft deleted.

So I'm personally inclined to remove the check entirely, but if you want this code to function as reusable business logic, then I think we need to check it's a draft release first - before calling BulkDeleteDataSetVersions, DeleteCacheFolderAsync, etc. - like the code sample I gave in the previous comment.