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.

Sign up for GitHub

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

Jump to bottom

#170230517 Display all notification for a specific user #52

Merged
merged 1 commit into from
Dec 12, 2019
Merged

#170230517 Display all notification for a specific user #52

merged 1 commit into from
Dec 12, 2019

Conversation

NiyoEric
Copy link
Contributor

@NiyoEric NiyoEric commented Dec 11, 2019 •
edited
Loading

What does this PR do?

The PR displays all notifications for a specific user

Description of Task to be completed?

Acceptance Criteria
  • Notification for new comment
  • Notification for a trip
  • Notification for a booking
  • Notification for accommodation.

How should this be manually tested?

  • Open your terminal
  • git clone https://github.com/andela/team-odd-bn-backend.git
  • cd team-odd-bn-backend
  • git checkout ft-all-user-notification-170230517
  • Make sure your .env file is updated.
  • Run migrations npm run db-migrate
  • Run psql (Download if it's not installed)
  • Run \c barefoot_nomad_test_db to connect to the database
  • Run SELECT * FROM {tableName}
  • OR select * from INFORMATION_SCHEMA.TABLE_CONSTRAINTS; to see the schema

Any background context you want to provide?

No

What are the relevant pivotal tracker stories?

#170230517

Screenshots (if appropriate)

All notification
Screen Shot 2019-12-11 at 08 50 31

Documentation
Screen Shot 2019-12-11 at 16 14 30

Questions:

hound[bot]
hound bot reviewed Dec 11, 2019
View reviewed changes
src/helpers/notificationConfig.js Outdated
@@ -0,0 +1,18 @@
import eventEmitters from './eventEmitters';
Copy link

Choose a reason for hiding this comment

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

Parsing error: 'import' and 'export' may appear only with 'sourceType: module'

@NiyoEric NiyoEric added the Ready for review Task is done. It is ready to be reviewed label Dec 11, 2019
@NiyoEric NiyoEric force-pushed the ft-all-user-notification-170230517 branch from 2997793 to 812c13d Compare December 11, 2019 08:43
@NiyoEric NiyoEric added Done Feature/chore task is done and removed Ready for review Task is done. It is ready to be reviewed labels Dec 11, 2019
hezronkimutai
hezronkimutai reviewed Dec 11, 2019
View reviewed changes
Copy link
Collaborator

@hezronkimutai hezronkimutai left a comment

Choose a reason for hiding this comment

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

Nice work @NiyoEric I like the way you have re-used many functionalities and also refactored the existing notification code and this making my work easier, I have left one feedback, please have a look at it.

src/routes/api/userRoute.js Show resolved Hide resolved
@NiyoEric NiyoEric force-pushed the ft-all-user-notification-170230517 branch from 812c13d to dfadf5a Compare December 11, 2019 14:18
victkarangwa
victkarangwa approved these changes Dec 11, 2019
View reviewed changes
Copy link
Collaborator

@victkarangwa victkarangwa left a comment

Choose a reason for hiding this comment

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

Good job @NiyoEric
I like your modular coding techniques. I need to learn from you :)

william000000
william000000 reviewed Dec 12, 2019
View reviewed changes
src/services/NotificationService.js Outdated
} else {
receiverId = userId; // is a user
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

@NiyoEric How do you know who you are going to send the notification?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If user id saved in trips table equal to user id from token, that means a user adds a comment. The notification should be sent to the manager. Vice versa.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Okay, It makes sense.

william000000
william000000 reviewed Dec 12, 2019
View reviewed changes
Copy link
Collaborator

@william000000 william000000 left a comment

Choose a reason for hiding this comment

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

Good Job @NiyoEric, Kindly attend to the feedback I left.

src/tests/090-notification.js Show resolved Hide resolved
src/routes/api/userRoute.js Outdated Show resolved Hide resolved
@NiyoEric NiyoEric force-pushed the ft-all-user-notification-170230517 branch from dfadf5a to eb3aeef Compare December 12, 2019 08:45
@NiyoEric NiyoEric force-pushed the ft-all-user-notification-170230517 branch from eb3aeef to b1f1e1e Compare December 12, 2019 11:19
@NiyoEric
ft(notification): display all notification for a user
face623 
- add new route
- [Finish #170230517]
@NiyoEric NiyoEric force-pushed the ft-all-user-notification-170230517 branch from b1f1e1e to face623 Compare December 12, 2019 11:45
@julietezekwe julietezekwe merged commit 726e833 into develop Dec 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hound hound[bot] hound[bot] left review comments

@hezronkimutai hezronkimutai hezronkimutai approved these changes

@william000000 william000000 william000000 approved these changes

@victkarangwa victkarangwa victkarangwa approved these changes

@julietezekwe julietezekwe julietezekwe approved these changes

@ivymwende ivymwende Awaiting requested review from ivymwende

Assignees
No one assigned
Labels
Done Feature/chore task is done
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@NiyoEric @julietezekwe @victkarangwa @william000000 @hezronkimutai