Notify post author about new comments by sending email #421

voronkovich · 2017-01-05T16:49:09Z

javiereguiluz · 2017-01-08T12:17:30Z

I like this feature proposal ... but what about replacing the private method in the controller by an event listener + custom event trigger? That would allow us to show custom events in action.

voronkovich · 2017-01-08T16:51:19Z

@javiereguiluz, I've added an event listener and custom event trigger. BTW, I found that we have an example of using mailer in the ListUsersCommand https://github.com/symfony/symfony-demo/blob/master/src/AppBundle/Command/ListUsersCommand.php#L131

javiereguiluz · 2017-01-08T21:22:58Z

app/config/services.yml

+        class: AppBundle\EventListener\CommentNotificationListener
+        arguments: ['@mailer', '@router', '@translator', '%app.notifications.email_sender%']
+        tags:
+            - { name: kernel.event_listener, event: comment.created, method: onCommentCreated }


I have two questions about the onCommentCreated method name:

When the method name is on + camelCasedEventName, the method parameter is optional. Should we add it anyway?

Some people like method names that describe the listened event (e.g. on + EventName) and others prefer semantic method names that describe their purposes (e.g. notifyCommentAdded()). Which is your preference?

javiereguiluz

I like this a lot. I've left a minor question and I'll tweak some help messages later ... but this looks ready and great. Thanks!

bocharsky-bw · 2017-01-09T08:05:48Z

src/AppBundle/EventListener/CommentNotificationListener.php

+    /**
+     * @var string
+     */
+    private $sender = 'symfony-demo@localhost';


What about to make this as a parameter in config.yml?

Yes. It's already passed to the constructor of the service, so I'd say: let's remove this private $sender property and just use the container parameter.

@javiereguiluz, @bocharsky-bw, do you suggest to inject container into the listener to get the sender parameter like there https://github.com/symfony/symfony-demo/blob/master/src/AppBundle/Command/ListUsersCommand.php#L138
As you can see in the listener definition those parameter (app.notifications.email_sender) is already injected.

no, keep the property here, but remove its default value and make it a mandatory constructor argument

Ok. I've removed the default sender's value.

stof · 2017-01-09T14:20:29Z

app/config/services.yml

-    # subscriber includes a method that returns the list of listened events.
+    app.comment_notification:
+        class: AppBundle\EventListener\CommentNotificationListener
+        arguments: ['@mailer', '@router', '@translator', '%app.notifications.email_sender%']


where is the app.notifications.email_sender parameter initialized ?

OK, it is already part of the codebase although not used for notifications today

voronkovich · 2017-01-09T16:48:09Z

app/config/services.yml

+        arguments: ['@mailer', '@router', '@translator', '%app.notifications.email_sender%']
+        # There is an optional tag attribute called "method" which defines which method to execute when the event is triggered.
+        # By default the name of the method is on + "camel-cased event name".
+        # If the event is "comment.created" the method executed by default is onCommentCreated().


@javiereguiluz, I've added a note about listener method's default name.

javiereguiluz · 2017-01-10T15:27:52Z

src/AppBundle/EventListener/CommentNotificationListener.php

+        $subject = $this->translator->trans('post.recieved_comment');
+        $body = $this->translator->trans('post.recieved_comment_message', [
+            '%title%' => $post->getTitle(),
+            '%link%' => $linkToPost.'#comment_'.$comment->getId(),


@voronkovich first, thank you for your patience with this code review 😄

I have a last comment about your PR. What if we use the shinny new URL fragment support in Symfony 3.2 to forget about concatenating strings to define fragments?

@javiereguiluz, it's a good idea! I've updated an url generating to use the _fragment parameter.

yceruto · 2017-01-10T18:04:08Z

app/Resources/translations/messages.en.xlf

@@ -276,6 +276,14 @@
                <source>post.deleted_successfully</source>
                <target>Post deleted successfully!</target>
            </trans-unit>
+            <trans-unit id="post.comment_added">


Just for consistency matter (with event name and method name) shouldn't be named post.comment_created instead?

@yceruto, I think you're right! I've changed messages names and added a more appropriate prefix for them.

javiereguiluz · 2017-01-12T16:39:40Z

@voronkovich another great feature added to the Demo app! Thank you very much for contributing this.

…voronkovich, javiereguiluz) This PR was merged into the master branch. Discussion ---------- Notify post author about new comments by sending email See #420 Commits ------- 7dc958c Change prefix for translation messages f918956 Use '_fragment' parameter 1e83a1d Final round of help notes improvements bbc4077 Improved more help notes a596761 Improved some help notes ec601b6 Minor reword in the help note f37e552 Minor tweaks in the translation strings 4125af9 Add note about event listener's method f3b258a Remove default value for sender property b4a97be Use event listener and custom event trigger 7b6ba9a Notify post author about new comments

javiereguiluz added the Feature label Jan 5, 2017

javiereguiluz mentioned this pull request Jan 8, 2017

Fix deprecated swiftmailer parameter #417

Closed

voronkovich force-pushed the swiftmailer-example branch from cd1f3b5 to c166bcd Compare January 8, 2017 16:07

Notify post author about new comments

7b6ba9a

voronkovich force-pushed the swiftmailer-example branch 2 times, most recently from 638fb82 to f87b37f Compare January 8, 2017 16:23

Use event listener and custom event trigger

b4a97be

voronkovich force-pushed the swiftmailer-example branch from f87b37f to b4a97be Compare January 8, 2017 16:35

javiereguiluz reviewed Jan 8, 2017

View reviewed changes

javiereguiluz approved these changes Jan 8, 2017

View reviewed changes

bocharsky-bw reviewed Jan 9, 2017

View reviewed changes

stof reviewed Jan 9, 2017

View reviewed changes

voronkovich added 2 commits January 9, 2017 19:14

Remove default value for sender property

f3b258a

Add note about event listener's method

4125af9

voronkovich commented Jan 9, 2017

View reviewed changes

javiereguiluz added 5 commits January 10, 2017 15:35

Minor tweaks in the translation strings

f37e552

Minor reword in the help note

ec601b6

Improved some help notes

a596761

Improved more help notes

bbc4077

Final round of help notes improvements

1e83a1d

javiereguiluz reviewed Jan 10, 2017

View reviewed changes

Use '_fragment' parameter

f918956

yceruto reviewed Jan 10, 2017

View reviewed changes

Change prefix for translation messages

7dc958c

javiereguiluz added the FINISHED label Jan 12, 2017

javiereguiluz merged commit 7dc958c into symfony:master Jan 12, 2017

voronkovich deleted the swiftmailer-example branch January 12, 2017 16:43

Uh oh!

Notify post author about new comments by sending email #421

Notify post author about new comments by sending email #421

Uh oh!

Conversation

voronkovich commented Jan 5, 2017

Uh oh!

javiereguiluz commented Jan 8, 2017

Uh oh!

voronkovich commented Jan 8, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

javiereguiluz left a comment

Choose a reason for hiding this comment

Uh oh!

bocharsky-bw Jan 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stof Jan 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

javiereguiluz commented Jan 12, 2017

Uh oh!

Uh oh!

bocharsky-bw Jan 9, 2017 •

edited

Loading

stof Jan 9, 2017 •

edited

Loading