Added Check For Existing Files And Attachments

[Gamma169]

This is a fix for #65
I created a method for the message struct that gets called before the send() function. It checks to see if the files listed in the attachments and embedded fields of the message struct actually exist. If they do, it returns nil for an error, if they don't, it returns the appropriate file does not exist error as specified in the golang os library docs

This gets caught before the send() function gets called, and therefore does not send any part of the email at all as to before, the error would be caught by the io.WriterTo in the smtp.go file only after it writes the body of the email. That's why the email would be sent, but then not send attachments and say that the email isn't sent.

I did not include a test for this because the testing suite seems to only tests for positive cases and not negative cases that are supposed to throw errors.

[Gamma169]

Note that I changed tests slightly with my second commit. Since we're now testing that the files ACTUALLY exist, the first commit failed tests with attachments because the tests just mocked the files. I created some empty files in the tests to make sure my added checks pass.

If anything, this can act showing that my amendments actually work.

[ivy]

The added functionality looks good to me but we need to break this out into its own test (e.g. TestNonExistentEmbedsAndAttachments) and fix TestRename so that it's still representative of testing that the Rename method is behaving as expected.

[ivy]

Correct me if I'm wrong, but won't this also create a bunch of files in the working directory? We need a teardown function to remove them when the test suite finishes. Also, this ends up being a step away from an actual copyFile and not a mock.

I need to think about this more. I'll come back after I've reviewed some of the other pull requests.

[ivy]

@Gamma169 If you could come up with a solution to test setup/teardown and write a separate test TestNonExistentEmbedsAndAttachments, that would be wonderful. If not, I'll see what I can do later.

[pedromorgan]

@Gamma169 Any chance u can submit this PR against the fork https://github.com/go-mail/gomail

[Gamma169]

I'll see if I can update these this weekend to address the comments. @pedromorgan, I will also make the PR on the fork specified.

@ivy I'll look into doing a test for it, but since the command is supposed to fail when missing an attachment and there are no analogous tests testing failure, I'm not sure how you want it set up. I can look into it, but if you give me a starting point to help figure it out and do it in the way you want, that would be helpful.

[ivy]

Thanks @Gamma169, looking forward to your pull request. 😄 Just as a heads up, there's a minor merge conflict to look out for between your changes and the current HEAD. If you're not familiar with rebasing, I can help you out.

As for the tests, I was thinking something along the lines of:

diff --git a/send_test.go b/send_test.go
index 9bf05b9..ffa1f0f 100644
--- a/send_test.go
+++ b/send_test.go
@@ -4,6 +4,7 @@ import (
 	"bytes"
 	"io"
 	"reflect"
+	"strings"
 	"testing"
 )
 
@@ -50,6 +51,25 @@ func TestSend(t *testing.T) {
 	}
 }
 
+func TestSendValidatesEmbeds(t *testing.T) {
+	s := &mockSendCloser{
+		mockSender: stubSend(t, testFrom, []string{testTo1, testTo2}, testMsg),
+		close: func() error {
+			t.Error("Close() should not be called in Send()")
+			return nil
+		},
+	}
+
+	m := getTestMessage()
+	m.Embed("this-file-does-not-exist")
+
+	err := Send(s, m)
+	if err == nil || !strings.HasSuffix(err.Error(),
+		"no such file or directory") {
+		t.Errorf("Send(): expected stat error but got %v", err)
+	}
+}
+
 func getTestMessage() *Message {
 	m := NewMessage()
 	m.SetHeader("From", testFrom)

There would also need to be an additional TestSendValidatesAttachments doing the same for Message.Attach().

[Gamma169]

I should be fine fixing the merge conflict hopefully. And thanks for the test code. I think I can take it from here.

[Gamma169]

Just finished updating the code and adding in the tests. I included a teardown function at the end of every test where we call mockcopy and therefore create the file.

I also found that my code broke the Rename functionality. I fixed this by adding a private field originalName to the file struct. When the checks for attachments and enbeds happens, it looks at this original name of the file.

The reason it needed this is because the Rename function changes the Name field but doesn't change the field originally passed in as the name in the CopyFunc field. Therefore it has the original name in the copyfunc but a different Name parameter. Therefore the only way I could see a fix to the problem was to add an originalName field. I hope that this is acceptable

[Gamma169]

PS I don't see any conflicts, by the way. Was it somehow only on your side?

[Gamma169]

@pedromorgan Submitted the PR to other repo. Let me know if that works for you.

[Gamma169]

Crap, it looks like I screwed everything up when I merged this branch into the other fork... I'm looking into fixing this.

[Gamma169]

Okay... I finally reverted it back to where I did the first comment. Will make a new branch off of this one and make the PR on the other fork on that other branch.