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

[WIP] Notifications for new matches #1657

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

[WIP] Notifications for new matches #1657

wants to merge 6 commits into from

Conversation

albertcui
Copy link
Member

No description provided.

}
return cb(err, match.match_id);
});
}

function sendNotifications(match) {
Copy link
Member

Choose a reason for hiding this comment

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

you probably have to check the origin of the parse request and do this for scanner only (similar to what was done for scenarios), otherwise, people requesting parses might cause multiple notifications?

Copy link
Member Author

Choose a reason for hiding this comment

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

code pointer?

Copy link
Member

Choose a reason for hiding this comment

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

https://github.com/odota/core/blob/master/svc/parser.js#L39

you could add something like doNotifications as a property on the match, or just check directly

return next();
});

notifications.route('/')
Copy link
Member

Choose a reason for hiding this comment

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

what is this route used for

Copy link
Member Author

Choose a reason for hiding this comment

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

It's used to store the notification token on the server so that we know who to send notifications to.

@@ -61,9 +61,34 @@ function parseProcessor(job, cb) {
console.error(err.stack || err);
} else {
console.log('completed parse of match %s', match.match_id);
sendNotifications(match);
Copy link
Member

Choose a reason for hiding this comment

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

I think we should make this only enabled if a config option is set. probably there are people using this that don't need this feature

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

Successfully merging this pull request may close these issues.

2 participants