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

chore: Replace update with simple delete for removeDeliveryOption #4825

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

dsamojlenko
Copy link
Member

@dsamojlenko dsamojlenko commented Dec 12, 2024

Summary | Résumé

removeDeliveryOption was attempting to use a prisma.update to remove the deliveryOption record when changing to API mode. This was not working as expected, and was throwing an incorrect TemplateAlreadyPublishedError.

Modified to:

  • Initial isPublished check and throw exception as published templates cannot be changed
  • Delete any DeliveryOptions for template

@dsamojlenko dsamojlenko changed the title Replace update with simple delete for removeDeliveryOption chore: Replace update with simple delete for removeDeliveryOption Dec 12, 2024
Copy link
Contributor

lib/templates.ts Outdated
try {
await authorization.canEditForm(ability, formID);

const updatedTemplate = await prisma.template
.update({
await prisma.deliveryOption
Copy link
Contributor

Choose a reason for hiding this comment

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

Just confirming that we're allowing the removeDeliveryOption to run on published forms?
The initial reason to use the prisma.template.update() was the ability to check if the form was published in the unique where:{ } before deleting the delivery option.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll take this back to draft, as there seems to be another bug introduced.

But yeah maybe this will just have to be two queries either way:

Check to make sure it's not published, then delete.
OR
Check to see if there is a DeliveryOption and use the update syntax.

The update syntax fails if there was no DeliveryOption set.

Copy link
Contributor

Choose a reason for hiding this comment

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

We can also catch that specific error in the prisma chain and ignore it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Seems to throw the same error in both cases.

Template with a DeliveryOption and published = true : P2025

And

Template with no DeliveryOption and published = false : P2025

...doesn't make a whole lot of sense as it says "No 'DeliveryOption' record was found ..." in both cases.

Copy link
Member Author

Choose a reason for hiding this comment

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

Pushed a commit with the split check, then delete.
I couldn't get it to work with just the update call.

Copy link
Contributor

Choose a reason for hiding this comment

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

If we want it to fail silently do we even need to return a descriptive error if the template is already published?

Copy link
Contributor

Choose a reason for hiding this comment

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

In other words shouldn't the client side be verifying if the template is already published before trying to make the action call? And if someone tries to hit the action endpoint directly it just returns a generic error.

Copy link
Member

Choose a reason for hiding this comment

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

For the "live api key version" --- there is code that does the published check.

... depends on where we calling the code from.

Part of this was looking to ensure the server is cleaned up when saving to "the vault".

Copy link
Member

Choose a reason for hiding this comment

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

that said looks like Dave already made some further edits and checks

@dsamojlenko dsamojlenko marked this pull request as draft December 12, 2024 17:21
@dsamojlenko dsamojlenko marked this pull request as ready for review December 12, 2024 17:55
Copy link
Contributor

@bryan-robitaille bryan-robitaille left a comment

Choose a reason for hiding this comment

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

I'm good with this solution for now. Ideally when we go back to refactoring the templates lib we can see if we can reduce the number of calls to the db.
For example, one of the reasons we have to check externally of the update function is to return a specific error. Quickly scanning the code-base I see that the TemplateAlreadyPublishedError and TemplateNotFound errors are generated in a few places but are never consumed or checked against. So the question becomes do we need to actually throw these custom errors?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants