-
Notifications
You must be signed in to change notification settings - Fork 2
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
Ot.delete template #175
Changes from 2 commits
7942b7e
b51daea
fd61527
468ca84
85b4ffc
bd0dfaf
4ead3b1
a2d0f19
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ⛏️ Can we use the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
|
@@ -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 '/'). | ||
|
@@ -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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ⛏️ Could we append There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔧 Now that we're initializing |
||
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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
} | ||
} |
There was a problem hiding this comment.
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)There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.