#167727330 Post comment notification #39
Merged
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
joaquinto
force-pushed
the
ft-create-comment-notification-167727330
branch
5 times, most recently
from
3b5beac
to
ddff467
Compare
joaquinto
added
review needed
joaquinto
force-pushed
the
ft-create-comment-notification-167727330
branch
3 times, most recently
from
b591e47
to
fc746f2
Compare
joaquinto
force-pushed
the
ft-create-comment-notification-167727330
branch
from
fc746f2
to
7f2c69f
Compare
joaquinto
force-pushed
the
ft-create-comment-notification-167727330
branch
3 times, most recently
from
8e4bf02
to
23378c8
Compare
Hector101
reviewed
Sep 11, 2019
src/controllers/commentController.js
Outdated
| const comment = await createComment(data); | ||
|
|
||
| // get the reason of the trip as title for the notification | ||
| const reason = await findTripById(Number(req.params.tripId)); |
There was a problem hiding this comment.
the variable naming reason lacks context, tripReason seems more appropriate.
src/controllers/commentController.js
Outdated
| const reason = await findTripById(Number(req.params.tripId)); | ||
|
|
||
| // check if the comment was sent from the manager or the staff | ||
| const senderRole = (Number(reason.toJSON().userId) !== req.auth.id) ? 'Manager' : null; |
There was a problem hiding this comment.
we can destructure userId and use it in multiple places where we have reason.toJSON().userId
const { userId } = reason.toJSON();
Then Number(userId)
src/controllers/commentController.js
Outdated
| * @param {Object} res | ||
| * @returns {Object} response object | ||
| */ | ||
| export const getAllComment = async (req, res) => { |
There was a problem hiding this comment.
getComments seems more appropriate to getAllComment
src/controllers/commentController.js
Outdated
| */ | ||
| export const getAllComment = async (req, res) => { | ||
| try { | ||
| const comment = await getAllComments(Number(req.params.tripId)); |
There was a problem hiding this comment.
If comment is an array of comments then it should be renamed comments.
src/controllers/commentController.js
Outdated
| export const getAllComment = async (req, res) => { | ||
| try { | ||
| const comment = await getAllComments(Number(req.params.tripId)); | ||
| return !comment ? respondWithWarning(res, statusCode.resourceNotFound, 'Comments not found') |
There was a problem hiding this comment.
This !comment is buggy. If no record is found, comment should be an empty array which is a truthy value, hence respondWithWarning(res, statusCode.resourceNotFound, 'Comments not found') will never be called.
Instead, check for !comment.length
- Add Comment model - Add comment migration - Add comment seeder - Add comment controller - Add comment route - Add comment middleware - Add comment service - Add comment notification [Finishes #167727330]
joaquinto
force-pushed
the
ft-create-comment-notification-167727330
branch
from
23378c8
to
cb2aa68
Compare
Hector101
approved these changes
Sep 11, 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?
Description of Task to be completed?
Any background context you want to provide?
How should this be manually tested?
npm installnpm run start:dev/api/v1/trips/<:tripId>/comment/api/v1/trips/<:tripId>/comments/api/v1/trips/<:tripId>/comments/<:commentId>What are the relevant PT stories?
(#167727330)[https://www.pivotaltracker.com/story/show/167727330]