Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CoreBundle] Adding MailerListenerSpec, fixing CS, small refactor #3161

Merged
merged 1 commit into from
Aug 27, 2015
Merged

[CoreBundle] Adding MailerListenerSpec, fixing CS, small refactor #3161

merged 1 commit into from
Aug 27, 2015

Conversation

mgonyan
Copy link
Contributor

@mgonyan mgonyan commented Aug 20, 2015

Q A
Bug fix? no
New feature? enhancement
BC breaks? no
Deprecations? no
Fixed tickets no
License MIT
Doc PR no
  • Adding specs for OrderMailerListener class
  • Refactoring conditions to send the email

@@ -32,6 +30,9 @@ class MailerListener
*/
protected $emailSender;

/**
* @param SenderInterface $emailSender
*/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should remove it, attributes have already comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry but, i don't see the issue to document the constructor. if the attributes have already a comment

@pjedrzejewski pjedrzejewski added the Enhancement Minor issues and PRs improving the current solutions (optimizations, typo fixes, etc.). label Aug 20, 2015
@mgonyan
Copy link
Contributor Author

mgonyan commented Aug 25, 2015

@pjedrzejewski could you please check the errors

pjedrzejewski pushed a commit that referenced this pull request Aug 27, 2015
[CoreBundle] Adding MailerListenerSpec, fixing CS, small refactor
@pjedrzejewski pjedrzejewski merged commit 20ffc2f into Sylius:master Aug 27, 2015
@pjedrzejewski
Copy link
Member

Scrutinizer errored and I can't retry the build. :( But Travis is OK, so I think we can merge it. Thank you! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Minor issues and PRs improving the current solutions (optimizations, typo fixes, etc.).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants