search

jdavidbakr / mail-tracker

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

fix for inject links with urls having a ' in the path #108

Closed ivang76 closed 4 years ago

jdavidbakr commented 4 years ago

Thanks, can you add a test for this change?

ivang76 commented 4 years ago

I tested on my projects and server but tell me if you need something else

jdavidbakr commented 4 years ago

There needs to be a phpunit test in the tests/MailTrackerTest.php file that fails without your change but passes with your change. You can run the tests with vendor/bin/phpunit

jdavidbakr commented 4 years ago

You could probably duplicate the it_handles_ampersands_in_links test and create a new test email with your use case

ivang76 commented 4 years ago

I never used phpunit to make test, so I'm gonna try to understand quickly how to use it...

ivang76 commented 4 years ago

Hi @jdavidbakr I added the phpunittest and done a commit... do you see it or should I open another pr now? (I don't undersand why but it seems I cannot push the last commit... maybe because there is already this pr opened?)

λ git log
commit eb1a52e33079cabb71fb5f86b6bdb0260c7245db
Author: Ivan <g...>
Date:   Thu Aug 6 18:06:13 2020 +0200

added phpunit test
jdavidbakr commented 4 years ago

You should be able to push it to your repo and it automatically update the pull request.

Add more commits by pushing to the master branch on ivang76/mail-tracker.

ivang76 commented 4 years ago

ok done... let me know

jdavidbakr commented 4 years ago

Looks like your test didn't pass

ivang76 commented 4 years ago

can you help me a bit to sort it out? as I told you I'm not used to phpunit tests

ivang76 commented 4 years ago

I have problems with my version of laravel and Orchestra\Testbench\TestCase... could you have a look and close this phpunit test so that the fix can be merged? Actually I'm still using my fix in prod without problem but in this way I can't update the vendors... thanks

jdavidbakr commented 4 years ago

Your issue is that lines 689-691 of the test is expecting there to be two links in your email. Note that the TestAmpersand.blade.php has a link without the ampersand, but your testApostrophe.blade.php doesn't have that. You can just change line 691 to be $aLink = $links[0].

Ideally with TDD you will write the test first, get a fail (it would have failed on line 698), and then write the code to get the test to pass.

ivang76 commented 4 years ago

ok done

jdavidbakr commented 4 years ago

Thanks - released version 4.0.6