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

#167164993 add user notification #29

Merged
merged 1 commit into from
Aug 22, 2019

Conversation

benisi
Copy link
Contributor

@benisi benisi commented Aug 7, 2019

Screenshots

response
new

flow chart

Blank Diagram (1)

Pivotal tracker story

#167164993

What does this PR do?

Add functions to create user notification

Summary of Task

This PR adds the following to the project

  • add notification config / template file
  • add notifications route
  • add add notification function to add notification
  • add get notification function to get notification
  • add email notification functionality

How can this be tested?

# Clone this repository
$ git clone https://github.com/andela/dahlia-ah-backend.git

# Navigate to the application folder
$ cd dahlia-ah-backend

# Install dependencies
$ npm install

# start the server by typing
$ npm run dev:start

# send a get request to http://localhost:3000/api/v1/notifications 
make sure you enter you authorization header token

you will get the notification of the user that are unread if there is none you will receive an empty array.






@Nerocodes Nerocodes temporarily deployed to ah-dahlia-staging-pr-29 August 7, 2019 04:04 Inactive
@benisi benisi force-pushed the feature/167164993/create-notification branch from 8ae5054 to c0346ed Compare August 7, 2019 10:56
@Nerocodes Nerocodes temporarily deployed to ah-dahlia-staging-pr-29 August 7, 2019 10:57 Inactive
@benisi benisi force-pushed the feature/167164993/create-notification branch from c0346ed to e0c96f9 Compare August 14, 2019 07:53
@Nerocodes Nerocodes had a problem deploying to ah-dahlia-staging-pr-29 August 14, 2019 07:53 Failure
@benisi benisi changed the title feature(notification) add user notification #167164993 add user notification Aug 14, 2019
@benisi benisi force-pushed the feature/167164993/create-notification branch from e0c96f9 to 73ca266 Compare August 14, 2019 12:21
@Nerocodes Nerocodes had a problem deploying to ah-dahlia-staging-pr-29 August 14, 2019 12:21 Failure
Copy link
Contributor

@allebd allebd left a comment

Choose a reason for hiding this comment

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

I observed no screenshot attached

@benisi benisi force-pushed the feature/167164993/create-notification branch from 73ca266 to 06b50a2 Compare August 16, 2019 21:33
@Nerocodes Nerocodes had a problem deploying to ah-dahlia-staging-pr-29 August 16, 2019 21:34 Failure
@benisi benisi force-pushed the feature/167164993/create-notification branch from 06b50a2 to debf3a4 Compare August 16, 2019 22:30
@Nerocodes Nerocodes had a problem deploying to ah-dahlia-staging-pr-29 August 16, 2019 22:31 Failure
@benisi benisi force-pushed the feature/167164993/create-notification branch from debf3a4 to 8e9fc70 Compare August 17, 2019 02:29
@Nerocodes Nerocodes had a problem deploying to ah-dahlia-staging-pr-29 August 17, 2019 02:29 Failure
@benisi benisi force-pushed the feature/167164993/create-notification branch from 8e9fc70 to 02cfb9b Compare August 17, 2019 02:36
@Nerocodes Nerocodes had a problem deploying to ah-dahlia-staging-pr-29 August 17, 2019 02:37 Failure
@benisi benisi force-pushed the feature/167164993/create-notification branch from 02cfb9b to c7c7483 Compare August 17, 2019 03:22
@Nerocodes Nerocodes had a problem deploying to ah-dahlia-staging-pr-29 August 17, 2019 03:22 Failure
@Ukhu
Copy link
Contributor

Ukhu commented Aug 17, 2019

Kindly place your screenshot as the last section of the PR description as stated in the template, and add a screenshot of your flowchart for this task.

@benisi
Copy link
Contributor Author

benisi commented Aug 18, 2019

Kindly place your screenshot as the last section of the PR description as stated in the template, and add a screenshot of your flowchart for this task.

i am having a problem loading screenshot on the bottom, it seems to only show at the top

@@ -14,7 +14,7 @@ externalDocs:
url: https://github.com/andela/dahlia-ah-backend

servers:
- url: /api/v1
- url: http://localhost:3000
Copy link
Contributor

Choose a reason for hiding this comment

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

I observed you specified port 3000 above if another person is not using the same port number do you think it will still work.

@benisi benisi force-pushed the feature/167164993/create-notification branch from c7c7483 to 23a1ec2 Compare August 19, 2019 07:37
@Nerocodes Nerocodes had a problem deploying to ah-dahlia-staging-pr-29 August 19, 2019 07:37 Failure
Copy link
Contributor

@OvieMudi OvieMudi left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@Nerocodes Nerocodes left a comment

Choose a reason for hiding this comment

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

LGTM


const getNotification = async (req, res) => {
const { id } = req.user;
const userNotificationObject = await getUserNotification(id);
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel you could use a try/catch block in this controller, in case an error occurs while awaiting getUserNotification(id)

Copy link
Contributor

Choose a reason for hiding this comment

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

Pls attend to this review

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor

@profmcdan profmcdan left a comment

Choose a reason for hiding this comment

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

Attend to the feedback, as well as handle necessary errors in your code


const getNotification = async (req, res) => {
const { id } = req.user;
const userNotificationObject = await getUserNotification(id);
Copy link
Contributor

Choose a reason for hiding this comment

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

Pls attend to this review

subject: 'Authors Haven - Notification',
html: `${messageHeader}
<p>${actor} ${message}
<a href='${process.env.SERVER_URL}${novelUrl || ''}'> ${novelTitle || ''}</a></p>
Copy link
Contributor

Choose a reason for hiding this comment

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

Please consider a better implementation here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@benisi benisi force-pushed the feature/167164993/create-notification branch from 23a1ec2 to fd2badc Compare August 22, 2019 14:41
@Nerocodes Nerocodes had a problem deploying to ah-dahlia-staging-pr-29 August 22, 2019 14:41 Failure
@Nerocodes Nerocodes temporarily deployed to ah-dahlia-staging-pr-29 August 22, 2019 15:22 Inactive
- add in app notification using socket.io
- add email notification
- add get all notification

[Finishes #167164993]
@profmcdan profmcdan merged commit 5e834fe into staging Aug 22, 2019
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.

None yet

7 participants