#167891584 Notifications for new travel request(email notification) #46
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Pomile
force-pushed
the
ft-emailNotification-forNewTravelRequest-167891584
branch
3 times, most recently
from
96c1aa0
to
b795db5
Compare
Pomile
requested review from
tvpeter,
femitj,
max-wel and
daniellamarr
and removed request for
tvpeter
Pomile
force-pushed
the
ft-emailNotification-forNewTravelRequest-167891584
branch
2 times, most recently
from
dc9635b
to
4ff8048
Compare
max-wel
reviewed
Sep 12, 2019
max-wel
reviewed
Sep 12, 2019
| module.exports = { | ||
| up: (queryInterface, Sequelize) => { | ||
| return queryInterface.createTable('Notifications', { | ||
| id: { |
There was a problem hiding this comment.
Kindly add a field that contains the notification recipient like recipientId
There was a problem hiding this comment.
Thank you, Maxwell.
Who should be the recipient? I understand that at the present we do not have a department manager. Why do you think need we recipientId in the Notification table? What if I want to send a notification to a group of users?
There was a problem hiding this comment.
The user the notification is meant for. I think it's needed so we can fetch all notifications belonging to a particular user
Pomile
force-pushed
the
ft-emailNotification-forNewTravelRequest-167891584
branch
11 times, most recently
from
049f8f9
to
579dffd
Compare
…ure for new travel request - [x] Create Notifications model and migration - [x] Create Reads model and migration - [x] Create an HTML template for travel request email notification - [x] Create a function that finds all admins - [x] Build template with data - [x] Send email message to administrators - [x] Add notification data to the database [Finishes #167891584]
Pomile
force-pushed
the
ft-emailNotification-forNewTravelRequest-167891584
branch
from
579dffd
to
362e094
Compare
daniellamarr
approved these changes
Sep 16, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Have an email notification sent to admin users when new travel request is initiated
Description of Task to be completed?
How should this be manually tested?
Unit test
git pullgit checkout ft-emailNotification-forNewTravelRequest-167891584npm installdatabase_nameUnit test
git pullgit checkout ft-emailNotification-configOpt-endpoint-167891584npm installdatabase_nameDevelopment test with Postman
npm run devstart/api/v1/trips{ "type": "oneway", "from": "xxxxxxxxxxxxxxxxx", "departureDate": "2000-09-09", "reason": "Just wanna travel", "destination": { "to": "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx", "accomodation": "xxxxxxxxxxxxxxxxxxxxxxxxxxxx" } }What are the relevant pivotal tracker stories?
#167891584
Any background context you want to add?
Nil
Screenshots
Nil