Clot.email attchment #217

CerberusLatrans · 2023-11-19T18:19:23Z

Why

Resolves #201

This allows admins to send a lot of different types of attachments to volunteers.

This PR

Converts base64 to bytearraydata (which takes the mime type and data as byte[]) to use simplejavamail api.
Frontend devs need to convert attachment to base 64 before using route.

Verification Steps

Sent emails with jpg, docx, csv, zip, pdf

huang0h

Awesome work guys! Great job with figuring out how to send attachments 🥳

huang0h · 2023-11-27T02:59:33Z

api/src/main/java/com/codeforcommunity/dto/emailer/EmailAttachment.java

+
+    private EmailAttachment() {}
+
+    private static String getFileExtension(String base64Image) {


⛏️ We should clean up this method so the variable names and comments better reflect what we're working with, so anybody in the future (including you!) that looks at this code can quickly and clearly understand what the method does.
For example, the variable base64Image doesn't only contain base 64 image data - it could contain base 64 data for any kind of file now, so something more accurate (maybe base64File?) would better communicate what's going on here.

Adding onto that, since this method is getting a file's mime type, could we rename it so it better describe what it's doing - maybe getFileMimeType?

huang0h · 2023-11-27T03:06:59Z

api/src/main/java/com/codeforcommunity/dto/emailer/EmailAttachment.java

+        //String[] data = dataSplit[1].split("/", 2); // Split the image type here (e.g. "image/png")
+        //if (data.length != 2 || !data[0].equals("image")) {
+        //    // Ensure the encoded data is an image
+        //    return null;
+        //}


⛏️ Remember to clean up any blocks that are commented out! If you don't need this code, make sure to delete it

huang0h · 2023-11-27T03:17:38Z

api/src/main/java/com/codeforcommunity/dto/emailer/EmailAttachment.java

+
+        try {
+            // Temporarily writes the image to disk to decode
+            mimeType = getFileExtension(data);


⛏️ It looks like getFileExtension() could potentially return null, but we don't ever check for that here. Once we call getFileExtension(), we should check if its result is null (in which case we should probably throw an exception) so we deal with the failure as soon as possible, rather than have it break something else down the line.

huang0h · 2023-11-27T03:28:35Z

api/src/main/java/com/codeforcommunity/dto/emailer/EmailAttachment.java

+        try {
+            // Temporarily writes the image to disk to decode
+            mimeType = getFileExtension(data);
+            System.out.println(mimeType);


⛏️ Similar to the commented-out blocks of code - make sure to remove any prints before you merge! If you think this information is important enough to print out, we can use the SLogger.java class for more robust logging

huang0h · 2023-11-27T03:30:21Z

service/src/main/java/com/codeforcommunity/processor/ProtectedNeighborhoodsProcessorImpl.java

+    List<AttachmentResource> attachments = sendEmailRequest.getAttachments().stream()
+            .map(ea -> ea.getAttachmentResource()).collect(Collectors.toList());


huang0h · 2023-11-27T21:04:13Z

service/src/main/java/com/codeforcommunity/processor/ProtectedSiteProcessorImpl.java

@@ -660,7 +660,7 @@ private SiteEntriesRecord latestSiteEntry(int siteId) {
  @Override
  public void uploadSiteImage(
      JWTData userData, int siteEntryId, UploadSiteImageRequest uploadSiteImageRequest) {
-    checkEntryExists(siteEntryId);
+    checkSiteExists(siteEntryId);


❓ Forgot to mention (sorry for the double review 😞) - was there a reason for changing this method? IIRC site images are associated with site entries, not sites, which is why we use checkEntryExists() rather than checkSiteExists()

…r-the-trees-backend-v2 into CLOT.emailAttchment update old branch

huang0h

lgtm!

huang0h · 2024-02-11T17:21:40Z

api/src/main/java/com/codeforcommunity/dto/emailer/EmailAttachment.java

-            System.out.println(mimeType);
+            mimeType = getFileMimeType(data);
+            if (mimeType==null) {
+                throw new MalformedParameterException("data");


Didn't know we had this exception, good find!

huang0h · 2024-02-11T17:23:21Z

node_modules/.yarn-integrity

+{
+  "systemParams": "darwin-arm64-83",
+  "modulesFolders": [
+    "node_modules"
+  ],
+  "flags": [],
+  "linkedModules": [],
+  "topLevelPatterns": [],
+  "lockfileEntries": {},
+  "files": [],
+  "artifacts": {}
+}


🔧 If you'd like, you can remove this folder along with package-lock.json and yarn.lock - not sure how these ended up here but they shouldn't affect anything.

SurabhiKeesara

Looks great, just a couple of comments

SurabhiKeesara · 2024-02-11T20:18:25Z

api/src/main/java/com/codeforcommunity/dto/emailer/EmailAttachment.java

@@ -0,0 +1,105 @@
+package com.codeforcommunity.dto.emailer;


All the same comments as on reject email

avoid using comments and instead focus on good names (which it looks like you have, so you can delete these)

baseEncoding validation based on length

SurabhiKeesara · 2024-02-11T20:19:54Z

service/src/main/java/com/codeforcommunity/requester/Emailer.java

    String filePath = "/emails/Email.html";

    Map<String, String> templateValues = new HashMap<>();
    templateValues.put("body", body.replaceAll("\n", "<br />"));
    Optional<String> emailBody = emailOperations.getTemplateString(filePath, templateValues);

+


Super nitpicky but avoid adding extraneous spaces

CerberusLatrans · 2024-04-07T21:10:44Z

Retested both emailing with attachments and rejecting images after merge

attachment mime type fix, testing

fbf0caa

CerberusLatrans requested review from chromium-52, SurabhiKeesara and huang0h November 19, 2023 18:19

huang0h reviewed Nov 27, 2023

View reviewed changes

CerberusLatrans added 2 commits February 4, 2024 13:17

Merge branch 'master' of https://github.com/Code-4-Community/speak-fo…

ebcbcdb

…r-the-trees-backend-v2 into CLOT.emailAttchment update old branch

address bad practices and bugs

fd44a53

CerberusLatrans requested a review from huang0h February 4, 2024 18:31

huang0h approved these changes Feb 11, 2024

View reviewed changes

SurabhiKeesara reviewed Feb 11, 2024

View reviewed changes

CerberusLatrans closed this Apr 7, 2024

CerberusLatrans reopened this Apr 7, 2024

Merge branch 'master' into CLOT.emailAttchment

d951a4a

CerberusLatrans merged commit 269355e into master Apr 7, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clot.email attchment #217

Clot.email attchment #217

CerberusLatrans commented Nov 19, 2023

huang0h left a comment

huang0h Nov 27, 2023

huang0h Nov 27, 2023

huang0h Nov 27, 2023

huang0h Nov 27, 2023

huang0h Nov 27, 2023

huang0h Nov 27, 2023

huang0h Nov 27, 2023

huang0h left a comment

huang0h Feb 11, 2024

huang0h Feb 11, 2024

SurabhiKeesara left a comment

SurabhiKeesara Feb 11, 2024

SurabhiKeesara Feb 11, 2024

CerberusLatrans commented Apr 7, 2024


		private EmailAttachment() {}

		private static String getFileExtension(String base64Image) {

		List<AttachmentResource> attachments = sendEmailRequest.getAttachments().stream()
		.map(ea -> ea.getAttachmentResource()).collect(Collectors.toList());

Clot.email attchment #217

Clot.email attchment #217

Conversation

CerberusLatrans commented Nov 19, 2023

Why

This PR

Verification Steps

huang0h left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huang0h left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SurabhiKeesara left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CerberusLatrans commented Apr 7, 2024