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

Ot.delete template #175

Merged
merged 8 commits into from
Jul 2, 2023
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -6,4 +6,6 @@
public interface IProtectedEmailerProcessor {
/** Adds an HTML email template to cloud storage. */
void addTemplate(JWTData userData, AddTemplateRequest addTemplateRequest);
/** Deletes an HTML email template with the given name in cloud storage. */
void deleteTemplate(JWTData userData, String templateName);
}
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ public Router initializeRouter(Vertx vertx) {
Router router = Router.router(vertx);

registerAddTemplate(router);
registerDeleteTemplate(router);

return router;
}
Expand All @@ -43,4 +44,18 @@ private void handleAddTemplate(RoutingContext ctx) {

end(ctx.response(), 200);
}

private void registerDeleteTemplate(Router router) {
Route deleteTemplate = router.post("/delete_template/:template_name");
Copy link
Contributor

Choose a reason for hiding this comment

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

⛏️ 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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

deleteTemplate.handler(this::handleDeleteTemplate);
}

private void handleDeleteTemplate(RoutingContext ctx) {
JWTData userData = ctx.get("jwt_data");
String templateName = RestFunctions.getRequestParameterAsString(ctx.request(), "template_name");

processor.deleteTemplate(userData, templateName);

end(ctx.response(), 200);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -17,4 +17,10 @@ public void addTemplate(JWTData userData, AddTemplateRequest addTemplateRequest)
userData.getUserId(),
addTemplateRequest.getTemplate());
}

@Override
public void deleteTemplate(JWTData userData, String templateName) {
assertAdminOrSuperAdmin(userData.getPrivilegeLevel());
S3Requester.deleteHTML(templateName, "email_templates");
Copy link
Member

Choose a reason for hiding this comment

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

⛏️ Can we use the TEMPLATE_DIR constant here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, I forgot to change that once load template was merged

}
}
Original file line number Diff line number Diff line change
@@ -1,18 +1,20 @@
package com.codeforcommunity.requester;

import com.amazonaws.AmazonServiceException;
import com.amazonaws.SdkClientException;
import com.amazonaws.regions.Regions;
import com.amazonaws.services.s3.AmazonS3;
import com.amazonaws.services.s3.AmazonS3ClientBuilder;
import com.amazonaws.services.s3.model.CannedAccessControlList;
import com.amazonaws.services.s3.model.DeleteObjectRequest;
import com.amazonaws.services.s3.model.ObjectMetadata;
import com.amazonaws.services.s3.model.PutObjectRequest;
import com.codeforcommunity.aws.EncodedImage;
import com.codeforcommunity.exceptions.BadRequestHTMLException;
import com.codeforcommunity.exceptions.BadRequestImageException;
import com.codeforcommunity.exceptions.InvalidURLException;
import com.codeforcommunity.exceptions.S3FailedUploadException;
import com.codeforcommunity.propertiesLoader.PropertiesLoader;

import java.io.File;
import java.io.FileOutputStream;
import java.io.IOException;
Expand Down Expand Up @@ -206,8 +208,8 @@ public static String getFileNameWithoutExtension(String eventTitle, String suffi
}

/**
* Validate the given string encoding of HTML and upload it to the user upload S3 bucket.
* HTML will be overwritten in S3 if another file of the same name is uploaded.
* Validate the given string encoding of HTML and upload it to the user upload S3 bucket. HTML
* will be overwritten in S3 if another file of the same name is uploaded.
*
* @param name the desired name of the new file in S3 (without a file extension).
* @param directoryName the desired directory of the file in S3 (without leading or trailing '/').
Expand Down Expand Up @@ -273,4 +275,31 @@ public static String uploadHTML(

return String.format("%s/%s/%s", externs.getBucketPublicUrl(), directoryName, name);
}

/**
* Delete the existing HTML file with the given name from the user uploads S3 bucket.
*
* @param name the name of the HTML file in S3 to be deleted.
* @param directoryName the directory of the file in S3 (without leading or trailing '/').
* @return previously existing HTML file URL if the deletion was successful.
* @throws InvalidURLException if the file does not exist.
* @throws SdkClientException if the deletion from S3 failed.
*/
public static String deleteHTML(String name, String directoryName) {
String HTMLPath = directoryName + "/" + name;
Copy link
Member

@chromium-52 chromium-52 Jun 18, 2023

Choose a reason for hiding this comment

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

⛏️ 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point-- I forgot to add this for delete template like with load and add template.


// if the object does not exist, it throws a 403 for metadata?
try {
Copy link
Member

Choose a reason for hiding this comment

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

🔧 Now that we're initializing s3Client with the access/secret keys, we might not need this try-catch

Boolean htmlExists = externs.getS3Client().doesObjectExist(externs.getBucketPublic(), HTMLPath);
} catch(AmazonServiceException e) {
throw new InvalidURLException();
}

// Create the request to delete the HTML
DeleteObjectRequest awsRequest = new DeleteObjectRequest(externs.getBucketPublic(), HTMLPath);

externs.getS3Client().deleteObject(awsRequest);

return String.format("%s/%s/%s", externs.getBucketPublicUrl(), directoryName, name);
Copy link
Contributor

Choose a reason for hiding this comment

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

⛏️ Do we use the return value anywhere? If not, I think we can just remove it, and make the method void instead.

}
}