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

feat(content-releases): Delete a Release #19000

Merged
merged 5 commits into from
Dec 11, 2023

Conversation

Feranchz
Copy link
Contributor

@Feranchz Feranchz commented Dec 7, 2023

What does it do?

Add a new /content-releases/:id/ delete endpoint to delete a release.

How to test it?

Make sure you have the content-releases feature enabled with a EE project.

  1. Make a request to DELETE /content-releases/:releaseId
  2. Release should exists, if not it should throw an error
  3. Release should not be published, if not it should throw an error
  4. If all good, it should return the details of the release

@Feranchz Feranchz self-assigned this Dec 7, 2023
Copy link
Contributor

@markkaylor markkaylor left a comment

Choose a reason for hiding this comment

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

It looks good to me and is deleting the release and removing the relation between a release and a release action. Currently though the release actions associated are not also deleted but I think want them to be. Idk with the entity service if we can cascade the delete or if we have to do it manually 🤔

But don't get trigger happy and also delete the entry 🙃

Comment on lines 245 to 258
const deletedRelease = (await strapi.entityService.delete(RELEASE_MODEL_UID, releaseId, {
populate: { actions: { fields: ['id'] } },
})) as unknown as Release;

// We delete each action related to the release
if (deletedRelease.actions) {
await strapi.db.query(RELEASE_ACTION_MODEL_UID).deleteMany({
where: {
id: {
$in: deletedRelease.actions.map(({ id }) => id),
},
},
});
}
Copy link
Contributor

Choose a reason for hiding this comment

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

So no matter what we delete the release right? If something goes wrong deleting the actions then what will happen? An error throws in the deleteMany, the release is already deleted, and we return an error? Seems a bit misleading in that case.

I'm wondering if the order of operations matters here. delete the actions, when that's a success delete the release? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the order you proposed is right, we should delete the actions firsts.

I also added a transaction for this, because in my opinion it's the perfect use case: We want to delete the release and all its actions only if we can delete all without problems 🤔

@markkaylor markkaylor self-requested a review December 11, 2023 08:48
},
},
});
await strapi.entityService.delete(RELEASE_MODEL_UID, releaseId);
Copy link
Contributor

Choose a reason for hiding this comment

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

I notice here you perform the delete of the release inside the transaction, whereas for update you update the releas outside the transaction. What's the difference?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess you mean when publishing a release...

Actually it's a good point, in the publishing case makes more sense put that update inside the transaction 🤔, so we make sure we don't publish entries if the update fails (it's a unlikely case but I guess is possible)

Are you okay if I make that change in this same PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hey yeah that's fine by me to make the same update here in this PR if you want otherwise this one looks good and we can merge.

Copy link
Contributor

@markkaylor markkaylor left a comment

Choose a reason for hiding this comment

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

Looks good to me, we need to revisit the transaction / update when publishing if the update should be done in the transaction

@Feranchz Feranchz merged commit 32d0c4f into feature/content-releases Dec 11, 2023
133 of 134 checks passed
@Feranchz Feranchz deleted the content-releases/delete-controller branch December 11, 2023 18:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants