Skip to content

Email notifications#801

Merged
lpatmo merged 13 commits into
stagingfrom
email-notifications-1
Apr 9, 2018
Merged

Email notifications#801
lpatmo merged 13 commits into
stagingfrom
email-notifications-1

Conversation

@distalx

@distalx distalx commented Mar 29, 2018

Copy link
Copy Markdown
Member

This PR is a part of an issue #626 and follows #776

Subscribers will be notified on following events

  • new hangouts
    when user creates a hangout in a study group.
  • new member
    when new members joins the group.
  • new discussion
    when user creates a discussion in a study group.

User can set notification preference from their account settings.

A cron job riggers these notifications.
Right now cron triggers at every 40 minute but for production we might want to tune it up to 20!.

email_notifications: {
initial: false,
reminder: false,
follow_up: false

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Does this mean users of a group won't receive email alerts when a hangout in that group is created?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

No, These flags are there so cron job can keep track for which notification has been sent.
we makes it true once notification is sent. here

// set intial flag for email_notifications true
listOfNewHangouts.forEach(function(hangout){
Hangouts.update({_id:hangout._id}, {$set:{
'email_notifications.initial': true

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

.... oh wait, it's set to true here!

@lpatmo

lpatmo commented Mar 30, 2018

Copy link
Copy Markdown
Member

Looks beautiful.

One bug I noticed: if you start a new discussion from the nav bar (instead of from inside a study group), the attempt fails because the discussion needs to be linked with a groupId.

Maybe the form for a new discussion started on the nav header needs to be different -- or include the list of groups?

Or perhaps we can remove the "Start a new discussion" button for now, and tell users to start new discussions from inside their groups.

Comment thread imports/libs/server/cb_mailer/index.js Outdated
@@ -0,0 +1,33 @@
import { Meteor } from 'meteor/meteor';
import {SSR} from 'meteor/meteorhacks:ssr';

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

whitespace

Comment thread imports/libs/server/cb_mailer/index.js Outdated
Email.send(mail);
} catch (e) {
console.log(e);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

whitespace

* @return { Array } members - list of group members
*/
function getGroupMembers(study_group_id) {
if (study_group_id === 'CB') {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

?

* @name initialEmailNotificationsForHangouts
*/
async function initialEmailNotificationsForHangouts() {
let hangouts = await getNewHangouts();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

maybe this is worth wrapping in try catch.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

getNewHangouts doesn't throw any err, So I'm not sure what purpose would it serve.

@distalx

distalx commented Apr 8, 2018

Copy link
Copy Markdown
Member Author

@lpatmo

One bug I noticed: if you start a new discussion from the nav bar (instead of from inside a study group), the attempt fails because the discussion needs to be linked with a groupId.

In that case Discussion is linked to the CB(global) group.

Maybe the form for a new discussion started on the nav header needs to be different -- or include the list of groups? Or perhaps we can remove the "Start a new discussion" button for now, and tell users to start new discussions from inside their groups.

The idea behind this behaviour is that user can start a discussion without being a part of any group. same as a hangout. and yes we should include the list of groups.

@lpatmo , @nalbina Thank you for reviewing this.

@lpatmo lpatmo merged commit accd2e3 into staging Apr 9, 2018
@lpatmo lpatmo added [state] closed the issue is now closed, see comments for more information and removed Waiting For Review labels Apr 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[state] closed the issue is now closed, see comments for more information

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants