[CCM-13003] Prevent templates being deleted when part of a message plan #799
Conversation
…emplate-management into feature/CCM-13003-delete-templates-in-message-plan
| logger.error('Failed to save template', error); | ||
|
|
||
| if ( | ||
| error.errorMeta?.description?.includes('linked to active message plans') |
There was a problem hiding this comment.
Not sure about relying on the error text here for error handling. I guess it's okay because it's our own API but just a gut feeling that this isn't the best way.
There was a problem hiding this comment.
I feel the same way, but unfortunately the error code just ends up as a 400 which we use for other things... any thoughts on what we should do instead?
There was a problem hiding this comment.
you could use the details property returned from the API on a failure response. So something like
errorMeta?.details?.errorCode === 'ERR_001'
IIRC details is an unknown type so might need some shenangians there.
There was a problem hiding this comment.
yeah, but aren't we still just comparing strings if we put something in there?
There was a problem hiding this comment.
a potentially shorter one, I grant you, but do you feel that adding another field to compare a different string seems like it solves the problem of 'using string comparison to determine the error' enough?
There was a problem hiding this comment.
I don't see an issue/problem with string comparison. It's how errors are handled all over the place and I don't see any difference here.
I guess it's more of what we're checking? Using what looks like an error description/text seems less intentional than an error code.
Core recently went through and removed alot of cases where error descriptions were used and replaced them with errorCodes. Having an API return an error code is pretty standard practice too.
--
Altnernative thoughts with what currently exsists (not saying do these or that they're good ideas):
- Delete template API ernd point could really only return a few HTTP status codes: 204, 400 and maybe a 500. We could instead say "When delete API returns a 400 then it means it's being used elsewhere so trying to delete it is a bad request".
- Use the
errorMeta.actualErrorto throw a custom error and on the consumer side new-up the error and do some sort ofinstanceof MyCustomErrorcheck.
There was a problem hiding this comment.
Just my 2 cents.
Not saying anything needs to change. The PR is functionally fine. I'm not all that bothered.
…emplate-management into feature/CCM-13003-delete-templates-in-message-plan
…emplate-management into feature/CCM-13003-delete-templates-in-message-plan
| ); | ||
|
|
||
| expect(deleteResponse.status()).toBe(409); | ||
| expect(deleteResponse.status()).toBe(400); |
There was a problem hiding this comment.
Maybe a discussion to be had about how much we want in these API tests, but the blocked delete case potentially should be included?
5 participants
Description
Context
Type of changes
Checklist
Sensitive Information Declaration
To ensure the utmost confidentiality and protect your and others privacy, we kindly ask you to NOT including PII (Personal Identifiable Information) / PID (Personal Identifiable Data) or any other sensitive data in this PR (Pull Request) and the codebase changes. We will remove any PR that do contain any sensitive information. We really appreciate your cooperation in this matter.