Ot.delete template #175
Ot.delete template #175
Conversation
There was a problem hiding this comment.
Overall LGTM, nice work! Most of my comments from the load template PR also apply here, specifically about the expected HTML path and adding AWS credentials.
|
|
||
| externs.getS3Client().deleteObject(awsRequest); | ||
|
|
||
| return String.format("%s/%s/%s", externs.getBucketPublicUrl(), directoryName, name); |
There was a problem hiding this comment.
⛏️ Do we use the return value anywhere? If not, I think we can just remove it, and make the method void instead.
| @Override | ||
| public void deleteTemplate(JWTData userData, String templateName) { | ||
| assertAdminOrSuperAdmin(userData.getPrivilegeLevel()); | ||
| S3Requester.deleteHTML(templateName, "email_templates"); |
There was a problem hiding this comment.
⛏️ Can we use the TEMPLATE_DIR constant here?
There was a problem hiding this comment.
Yep, I forgot to change that once load template was merged
| String HTMLPath = directoryName + "/" + name; | ||
|
|
||
| // if the object does not exist, it throws a 403 for metadata? | ||
| try { |
There was a problem hiding this comment.
🔧 Now that we're initializing s3Client with the access/secret keys, we might not need this try-catch
| * @throws SdkClientException if the deletion from S3 failed. | ||
| */ | ||
| public static void deleteHTML(String name, String directoryName) { | ||
| String HTMLPath = directoryName + "/" + name; |
There was a problem hiding this comment.
⛏️ Could we append _template.html at the end of htmlPath here so that the caller won't have to specify that (for instance, if someone wants to delete a template called some_name_template.html, then they would only have to pass in some_name as the name of the template to delete)? The fact that we append _template.html to the name of the template when we store it in S3 is an implementation detail and if possible, we want to hide it from the user so that they won't have to care about it to make their life easier.
There was a problem hiding this comment.
Good point-- I forgot to add this for delete template like with load and add template.
| @@ -60,4 +61,18 @@ private void handleLoadTemplate(RoutingContext ctx) { | |||
|
|
|||
| end(ctx.response(), 200, JsonObject.mapFrom(loadTemplateResponse).toString()); | |||
| } | |||
|
|
|||
| private void registerDeleteTemplate(Router router) { | |||
| Route deleteTemplate = router.post("/delete_template/:template_name"); | |||
There was a problem hiding this comment.
⛏️ Small nitpick - can we use route.delete() here to register a delete route? This way the request we make will be a little more idiomatic and match what we're trying to do (if you're not familiar with http methods, MDN has good info about them)
There was a problem hiding this comment.
Sure that makes sense; I was just copying the style of the other routes according to the API like delete site or delete user. Do you think it makes sense to refactor these too?
There was a problem hiding this comment.
I think it's definitely a good housekeeping thing to do, but not that important right now - if we change the backend routes, we'll need to make changes in the frontend as well.
|
|
||
| Boolean htmlExists = externs.getS3Client().doesObjectExist(externs.getBucketPublic(), HTMLPath); | ||
|
|
There was a problem hiding this comment.
🔧 Can we use the pathExists() method here as well? We should also throw an InvalidURLException if the file doesn't exist
Checklist
Why
ClickUp Ticket
Allows admins to delete existing email templates in the email-templates folder of the sftt-user-uploads bucket
This PR
First checks if the file exists with
doesObjectExist; however instead of returningfalseif the file does not exist, it throws an exception (seems to come from trying to access metadata) for a 403 forbidden error. Tried to enable attribute permissions on the bucket but doesn't work. Might have to instantiate S3 client with credentials for it to work? Works fine (returnstrue) with the file does actually exist.There might be a better workaround, but I catch the S3 exception and throw an
InvaliURLExceptioninstead.Verification Steps
Calling a POST request
http://localhost:8081/api/v1/protected/emailer/delete_template/[some existing name].htmlremoves that file from the S3 folder