Skip to content
This repository has been archived by the owner on Jul 28, 2022. It is now read-only.

Update SwiftMailerConsumer.php #364

Merged
merged 2 commits into from Feb 23, 2019
Merged

Conversation

red-smeg
Copy link
Contributor

Add ability to set the returnPath for mail messages sent via the notificationBundle

Subject

I am targeting this branch, because the change is backward compatible and adds another email parameter setting option but does not affect if it is not submitted.

Closes #363

Changelog

### Changed

sendEmail function to allow for there setting of a return path 

@core23
Copy link
Member

core23 commented Jan 29, 2019

Please add at least one test

@red-smeg
Copy link
Contributor Author

@core23 Hi was the request to add a test something you wanted me to do ? I am new to this so I'm honestly not sure what to do in that case. I would be happy to learn if someone would be happy to guide.

@OskarStark
Copy link
Member

Sure thing

Please have a look at SwiftMailerConsumerTest, you can check cc for reference in the test and „copy and adjust“ this part.
I am on a phone right now and not sure if this test exists nor if cc is tested properly 😎🤔

Please don’t hesitate to ask if sth is unclear

red-smeg pushed a commit to red-smeg/SonataNotificationBundle that referenced this pull request Jan 30, 2019
red-smeg referenced this pull request in red-smeg/SonataNotificationBundle Jan 30, 2019
correct the test changes
@red-smeg
Copy link
Contributor Author

I added a change to the swift mailer test

red-smeg referenced this pull request in red-smeg/SonataNotificationBundle Jan 30, 2019
@red-smeg
Copy link
Contributor Author

Can we check the test now ?

@greg0ire
Copy link
Contributor

greg0ire commented Feb 5, 2019

I cannot see any test in your PR.

@red-smeg
Copy link
Contributor Author

red-smeg commented Feb 7, 2019

I cannot see any test in your PR.

commit 0980e91

@greg0ire
Copy link
Contributor

greg0ire commented Feb 7, 2019

That's great but that commit is not part of your PR. I don't see it in the "Commits" tab.

@red-smeg
Copy link
Contributor Author

red-smeg commented Feb 9, 2019

That's great but that commit is not part of your PR. I don't see it in the "Commits" tab.

git hub says add commits by pushing to patch-1 branch. I did that this is my first PR I'm not sure what more I can do without making it more messy.

@greg0ire
Copy link
Contributor

greg0ire commented Feb 9, 2019

It does not look like you actually pushed that commit to the branch. What commands did you run?

OskarStark
OskarStark previously approved these changes Feb 12, 2019
@greg0ire
Copy link
Contributor

Please fix the build

@red-smeg
Copy link
Contributor Author

Please fix the build

Is that meant for me ? I honestly don't know how to do that or even why it failed those steps ? will gladly help if someone can explain what I need to do ?

@greg0ire
Copy link
Contributor

Yes it is meant for you. See the "Some checks were not successful" section below? There are few things you need to fix. Luckily, it is not unit tests, just code style issues, you can fix them by running make cs-fix

@greg0ire
Copy link
Contributor

greg0ire commented Feb 12, 2019

Alternatively, you can wait for #367 to be merged, since the issues do not come from your changes, but from a php-cs-fixer upgrade. Sorry for this 🙏

@OskarStark
Copy link
Member

I merged #367, can you please rebase?

Add ability to set the returnPath
@greg0ire
Copy link
Contributor

I just did it for you @red-smeg :)

@red-smeg
Copy link
Contributor Author

So did this get approved and incorporated ?

@greg0ire
Copy link
Contributor

So did this get approved and incorporated ?

No, we cannot merge unless the build is green.

remove trailing spaces to make build run clean

Co-Authored-By: red-smeg <mark_bateman@icloud.com>
@red-smeg
Copy link
Contributor Author

so what happens now. Looks approved do I close the PR or is that for whomever merges the change to do ?

@greg0ire greg0ire merged commit 5566688 into sonata-project:3.x Feb 23, 2019
@greg0ire
Copy link
Contributor

Does this answer your question? 😄 Thanks @red-smeg !

@OskarStark
Copy link
Member

Congratulations @red-smeg 🎉

@red-smeg
Copy link
Contributor Author

thanks for the help

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

set return path for SwiftmailerConsumer
4 participants