#167727329 feat(user receives notification): user should be able edit his trip request #54

solomonfrank · 2019-09-14T13:07:14Z

What does this PR do?

endpoint that allow a logged in user to edit his trip request

Description of Task to be completed?

ensure that the endpoint is working
run a test on the endpoint on the endpoint
PATCH /trips/{tripId}/edit

How should this be manually tested?

after cloning the repo, cd into it directory and Run

npm run migration
npm db:seed to insert dummy data into the database
npm run start:dev to start the server
use postman to test the endpoint by:
POST /auth/siginin to login
copy return token and paste into the header in this format Bearer Token
test PATCH /trips/{tripId}/edit

What are the relevant pivotal tracker stories?

#167727329

joaquinto

Apart from all those changes i pointed out, These look good to me. Thanks.

joaquinto · 2019-09-14T13:42:02Z

src/controllers/chatController.js

@@ -1,4 +1,4 @@
-import { chatEmitter, getChatsEmitter } from '../helpers/notificationHandler';
+import { actionEmitter } from '../helpers/notificationHandler';


Thank you very much for the effort in trying to refactor the code. but i think you should revert all these changes on the notifications the reason is because we don't have a frontend to test them with now and I'm sure you didn't test them after you change them. I can see some of the changes that will result in a bug that would need to be fixed later. Another reason is because other team members that were given this same task can understand the workflow between event emitter and socket.io easily. So please leave them the way they are now when we have an actual frontend platform then we can begin to start refactoring were needed to meet best practice but for now, leave them the way they are. I tested them and they are working perfectly with a frontend I created myself. It'll be a good idea to create your own notification and test if it is working as expected. Thanks

joaquinto · 2019-09-14T13:49:11Z

src/helpers/notificationHandler.js

-/**
- * @exports approvedEmitter
- * @exports approveTripNotification
- */



Kindly revert these changes. Thanks.

joaquinto · 2019-09-14T13:51:39Z

src/helpers/notificationHandler.js

-export const approvedEmitter = (data) => {
-  tripEmitter.emit('approved trip', data);
-};
+


Kindly revert these changes. Thanks.

joaquinto · 2019-09-14T13:53:16Z

src/helpers/notificationHandler.js

@@ -33,21 +30,26 @@ export const approvedTripNotification = (io, staffs) => {
  });
 };



Kindly revert these changes.

joaquinto · 2019-09-14T13:57:21Z

src/helpers/notificationHandler.js

- */
-export const chatEmitter = (data) => {
-  tripEmitter.emit('chat', data);
-};



Kindly revert these changes. Thanks

joaquinto · 2019-09-14T13:57:48Z

src/helpers/notificationHandler.js

-export const getChatsEmitter = (data) => {
-  tripEmitter.emit('chats', data);
-};
-
 /**


Kindly revert these changes. Thanks.

joaquinto · 2019-09-14T14:01:36Z

src/middlewares/tripMiddleware.js

@@ -3,7 +3,14 @@ import { respondWithWarning } from '../helpers/responseHandler';
 import statusCode from '../helpers/statusCode';

 export const verifyTrip = async (req, res, next) => {
-  const trip = await findTripById(req.params.tripId);
+


Kindly stick to tripId as the parameter for trip id and use Number(req.params.tripId) instead of parseInt(req.params.tripId, 10).

joaquinto · 2019-09-14T14:04:39Z

src/middlewares/tripMiddleware.js

+
+  if(!Number.isInteger(id)) {
+    return respondWithWarning(res, statusCode.resourceNotFound, 'Trip not found');
+  }


please, these changes are not needed, your validation should take care of that. Thank you.

joaquinto · 2019-09-14T14:05:33Z

src/routes/api/trip.routes.js

@@ -25,6 +26,7 @@ trip.get('/:tripId', authenticateUserToken, validateTripId, verifyTrip, checkPer
 trip.get('/', authenticateUserToken, checkPermission('VIEW_USERS_TRIP_REQUESTS'), getAllTripRequests);
 trip.patch('/:tripId/reject', authenticateUserToken, validateTripId, verifyTrip, checkPermission('APPROVE_TRIP_REQUEST'), rejectTripRequest);

+trip.patch('/:id/edit', authenticateUserToken, validateEditRequest, verifyTrip, verifyTripDestination, updateTripRequest);


kindly use :tripId as the parameter. Thanks.

joaquinto

Awesome job. LGTM.

Hector101 · 2019-09-16T10:07:10Z

src/controllers/tripController.js

+  try { 
+    const [, tripRequest] = await editTripRequest(body, params.tripId, auth.id);
+    const payload = {
+    type: 'Trip Request edited',


why are we making the type to be a sentence as opposed to word. This is ambiguous and it'll make sense to use updateTrip or tripUpdated, depending on which you find more appropriate.

Hector101 · 2019-09-16T10:08:59Z

src/helpers/notificationHandler.js

+*/
+
+export const editRequestEmitter = data => {
+  tripEmitter.emit('edit Request', data)


Likewise this, let's use editRequest as opposed to edit Request

Hector101 · 2019-09-16T10:14:14Z

src/helpers/notificationHandler.js

+    const [ Roles ] = await getAllManager();
+    const managers = Roles.toJSON();
+    const { users } = managers;
+    const managersEmail = users.map(items => items.email);


this iteration is not necessary. Doing this introduces an avoidable iteration (O(N)2) which reduces performance.

Hector101 · 2019-09-16T10:18:00Z

src/helpers/notificationHandler.js

+    managersEmail.forEach(email => {
+      io.to(staffs[email]).emit('edit Request', notification);
+    });
+  })


Rather than this

managersEmail.forEach(email => { io.to(staffs[email]).emit('edit Request', notification); });

,we can do this

users.forEach(({ email }) => { io.to(staffs[email]).emit('editRequest', notification); });

instead to reduce the number of overall iteration we have above const managersEmail = users.map(items => items.email);

Hector101 · 2019-09-16T10:20:06Z

src/helpers/notificationHandler.js

+
+    const [ Roles ] = await getAllManager();
+    const managers = Roles.toJSON();
+    const { users } = managers;


const managers = Roles.toJSON(); const { users } = managers;

Can be refactored to

const { user } = Roles.toJSON();

…cation when they edit their trip request [Delivers #167727329]

solomonfrank added the review needed Review this pull request label Sep 14, 2019

solomonfrank requested review from stepheng323, orley12, Hector101, joaquinto, steelze and shonubijerry September 14, 2019 13:07

joaquinto reviewed Sep 14, 2019

View reviewed changes

solomonfrank force-pushed the ft-notification-on-edit-167727329 branch 3 times, most recently from 5c4b084 to b28bb7c Compare September 15, 2019 15:57

joaquinto reviewed Sep 15, 2019

View reviewed changes

Hector101 reviewed Sep 16, 2019

View reviewed changes

feat(user receives notification): user should be able receive a notfi…

177fa69

…cation when they edit their trip request [Delivers #167727329]

solomonfrank force-pushed the ft-notification-on-edit-167727329 branch from b28bb7c to 177fa69 Compare September 16, 2019 13:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#167727329 feat(user receives notification): user should be able edit his trip request #54

#167727329 feat(user receives notification): user should be able edit his trip request #54

solomonfrank commented Sep 14, 2019

joaquinto left a comment

joaquinto Sep 14, 2019 •

edited

Loading

joaquinto Sep 14, 2019

joaquinto Sep 14, 2019

joaquinto Sep 14, 2019

joaquinto Sep 14, 2019

joaquinto Sep 14, 2019

joaquinto Sep 14, 2019 •

edited

Loading

joaquinto Sep 14, 2019

joaquinto Sep 14, 2019

joaquinto left a comment

Hector101 Sep 16, 2019

Hector101 Sep 16, 2019

Hector101 Sep 16, 2019

Hector101 Sep 16, 2019

Hector101 Sep 16, 2019

		@@ -1,4 +1,4 @@
		import { chatEmitter, getChatsEmitter } from '../helpers/notificationHandler';
		import { actionEmitter } from '../helpers/notificationHandler';

		@@ -33,21 +30,26 @@ export const approvedTripNotification = (io, staffs) => {
		});
		};

#167727329 feat(user receives notification): user should be able edit his trip request #54

Are you sure you want to change the base?

#167727329 feat(user receives notification): user should be able edit his trip request #54

Conversation

solomonfrank commented Sep 14, 2019

What does this PR do?

Description of Task to be completed?

How should this be manually tested?

What are the relevant pivotal tracker stories?

joaquinto left a comment

Choose a reason for hiding this comment

joaquinto Sep 14, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joaquinto Sep 14, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joaquinto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joaquinto Sep 14, 2019 •

edited

Loading

joaquinto Sep 14, 2019 •

edited

Loading