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

Add sendNotificationMessage hook #72

Merged
merged 4 commits into from
Aug 18, 2015

Conversation

dmolineus
Copy link
Contributor

This pull request provides a hook before sending a message.

I'm using the notification center in an application which sends a notification when something happens. This notification creates different messages for different persons. For example a productWasOrdered notifcation would send different messages to the product owner, the user who ordered the product and the system administrator.

The product owner has the option to disable such email notifications in his profile. To respect his user settings the hook is required.

Alternatively I would have to create different notifications for each receiver and handle the settings when creating the notification. But this would limit the flexibility which the notification center claims to provide.

@dmolineus
Copy link
Contributor Author

Another advantage is that the tokens could get enriched using the hook. So when creating the notification I only have to pass the recipientId. A hook would add the member username, email and so on automatically.

@Toflar Toflar added this to the 1.3.0 milestone Aug 12, 2015
@Toflar Toflar self-assigned this Aug 12, 2015
@Toflar
Copy link
Member

Toflar commented Aug 12, 2015

I would add this to 1.3 even though already in RC phase. I don't expect to have 1.4 anytime soon so I think we should accept this. @aschempp, @qzminski: If you agree, I'll merge it.

@qzminski
Copy link
Member

I'm fine with adding it now.

@dmolineus dmolineus force-pushed the feature/send-notification-hook branch from 6170792 to 0bcea88 Compare August 12, 2015 11:11
@dmolineus
Copy link
Contributor Author

Would be great if it will be part of release 1.3.0. Be aware that I've already rebased the branch with the changes in release/1.3.0. as I needed the fixes made there to get it installable.

If you want to merge this PR onto another branch, feel free to inform me. I can reopen it.

## Hooks

If you want to enrich each message being send by some meta data or want to disable some messages being sent, you can
Copy link
Member

Choose a reason for hiding this comment

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

each message being sent

@Toflar
Copy link
Member

Toflar commented Aug 13, 2015

Looks good to me, one little spelling mistake but then I'll merge :)

@dmolineus
Copy link
Contributor Author

Typo got fixed.

@Toflar
Copy link
Member

Toflar commented Aug 18, 2015

Thanks for the hint, unfortunately one does not get any notification when a new commit is pushed to a PR :)

Toflar added a commit that referenced this pull request Aug 18, 2015
@Toflar Toflar merged commit c31602b into terminal42:develop Aug 18, 2015
@dmolineus
Copy link
Contributor Author

I know. That's why I also add the comment.

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

Successfully merging this pull request may close these issues.

3 participants