jdavidbakr / mail-tracker

Package for Laravel to inject tracking code into outgoing emails.
MIT License
574 stars 129 forks source link

Fix issue #201 #248

Closed mirkos93 closed 4 months ago

mirkos93 commented 5 months ago
jdavidbakr commented 5 months ago

I like this concept - there are a couple of changes I'd like to see to this PR:

mirkos93 commented 5 months ago

If domains_in_context is empty it should ignore this check to ensure backwards compatibility for anyone who doesn't set it

I renamed domains_in_content to domains_in_context and added a check for empty($tracker->domains_in_context), although I believe that the linkClicked function cannot be called (logically speaking) if domains_in_context is empty, as this would mean that there are no domains in the content of the email. Please let me know if I'm misreading what you asked.

Be sure to add this to the test suite

I added the test MailTrackerTest.php it_redirects_to_fallback_for_invalid_domain, and I also modified and renamed the test it_redirects_even_if_no_sent_email_exists to it_redirects_to_fallback_if_the_sent_email_does_not_exists because, at least in the pull request I created, for security reasons, there should be no redirects that can be arbitrarily modified

I didn't notice any changes to the default config file to include this new attribute

There should be no need to add changes to the configuration file: mail-tracker.redirect-missing-links-to is already present in the configuration file.

Also please update the readme to include instructions about how to use this

I don't think there are any specific instructions to implement in the README, because this change cannot be disabled, it is a small security measure that ensures that redirects are always consistent with the body of the reference email.

jdavidbakr commented 5 months ago

Ok, I see what you did, I didn't fully understand it. I do like what you did. I'll take a closer look at it when I get a chance before merging it but I think it's going to work.