feat: Add task request flow by TanishqSingla · Pull Request #931 · RealDevSquad/website-backend

TanishqSingla · 2023-02-27T11:58:57Z

related to closes #904

Task Request Flow

This merge introduces api for task request feature.
The features include

API to fetch task requests to SUPERUSER
API to approve task request by SUPERUSER
API to request for a task for user.

routes/taskRequest.js

Earlier in the task flow the user would automatically be assigned the task, in this change the flow is changed to create a task request first which will be approved by superuser

TanishqSingla · 2023-03-04T13:28:36Z

Waiting on: Task Request API Contract

controllers/tasks.js

Update create task request model function Add approve task controller and model fucntion Add fetch tasks for user controller and model function

toFirestoreData for task request has been added This function validates the payload and returns a valid firestore object.

… user

routes/taskRequests.js

+const taskRequests = require("../controllers/tasksRequests");
+const cache = require("../utils/cache");
+
+router.get("/", authenticate, authorizeRoles([SUPERUSER]), cache(), taskRequests.fetchTaskRequests);


vinayak-trivedi · 2023-03-16T04:05:15Z

related to #904

try using closes #904, this will automatically close the issue ones this PR is merged

iamitprakash · 2023-05-24T19:41:04Z

models/taskRequests.js

+    const users = await Promise.all(
+      taskRequest.requestors.map((requestor) => {
+        if (visitedUsers.includes(requestor)) {
+          return visitedUsersId.find((visitedUserId) => (visitedUserId.id = requestor));


The bug is that the visitedUsersId variable is not being initialized. This means that the find() method will always return undefined. To fix this bug, you need to initialize the visitedUsersId variable to an empty array. You can do this by adding the following line to the top of the code:

visitedUsersId is already defined as a Set above, thanks for pointing this out though as I need to use .has method instead of .includes.

models/taskRequests.js

DashDeipayan · 2023-05-25T15:27:42Z

controllers/tasksRequests.js

+  try {
+    const data = await taskRequestsModel.fetchTaskRequests();
+
+    return res.status(200).json({


In case there are no tasks found we should be doing a 404 Not found status right??

DashDeipayan · 2023-05-25T15:30:37Z

controllers/tasksRequests.js

+
+    const { userStatusExists, data: userStatus } = await userStatusModel.getUserStatus(userId);
+    if (!userStatusExists) {
+      return res.boom.conflict("User status does not exist");


instead of doing 3 such rigorous if checks, how about we create a separate validator function?

DashDeipayan · 2023-05-25T15:33:33Z

controllers/tasksRequests.js

+    }
+
+    const { userStatusExists, data: userStatus } = await userStatusModel.getUserStatus(userId);
+    if (!userStatusExists) {


Same validator function suggestion as above

DashDeipayan · 2023-05-25T15:35:13Z

controllers/tasksRequests.js

+    if (!userStatusExists) {
+      return res.boom.conflict("User status does not exists");
+    }
+    if (userStatus.currentStatus.state === userState.OOO) {


capital OOO seems like a bad choice of the property name. We can go with OUT_OF_OFFICE. In this case maybe is a constant but OOO seems smelly).

This constant has been there for a while, I believe its best to deal with this issue separately.

I guess this is the time to fix this @RitikJaiswal75

DashDeipayan · 2023-05-25T15:41:39Z

models/taskRequests.js

+        continue;
+      }
+
+      const { user } = await userModel.fetchUser({ userId: requestor });


instead of doing two await calls inside a for-of loop, why not we keep them as Promises and do a Promise.all outside the loop. That will drastically improve the performance of this code. We can sit and discuss in a call if you have any doubts.

models/taskRequests.js

DashDeipayan

Good job with the PR. Lets get it done

routes/taskRequests.js

+const { validateUser } = require("../middlewares/taskRequests");
+
+router.get("/", authenticate, authorizeRoles([SUPERUSER]), cache(), taskRequests.fetchTaskRequests);
+router.post("/addOrUpdate", authenticate, validateUser, taskRequests.addOrUpdate);


routes/taskRequests.js

+
+router.get("/", authenticate, authorizeRoles([SUPERUSER]), cache(), taskRequests.fetchTaskRequests);
+router.post("/addOrUpdate", authenticate, validateUser, taskRequests.addOrUpdate);
+router.patch("/approve", authenticate, authorizeRoles([SUPERUSER]), validateUser, taskRequests.approveTaskRequest);


DashDeipayan

There are few comments that still needs to be addressed(not every comment is a change request, some are just queries 🙂)

DashDeipayan · 2023-05-27T13:40:03Z

models/taskRequests.js

+    const taskRequestData = taskRequestsSnapshot.data();
+    const { requestors } = taskRequestData;
+
+    const { taskData } = await tasksModel.fetchTask(taskRequestData.taskId);


You can move this line out of the for-loop by creating an array of promises(not writing await statements inside a for loop) and outside the for-loop this will be resolved with a promise.all()

DashDeipayan · 2023-05-27T13:40:38Z

models/taskRequests.js

+    const { requestors } = taskRequestData;
+
+    const { taskData } = await tasksModel.fetchTask(taskRequestData.taskId);
+    const users = await Promise.all(requestors.map((requestor) => userModel.fetchUser({ userId: requestor })));


Same suggestion as above for the users

DashDeipayan

LGTM

middlewares/taskRequests.js

models/taskRequests.js

iamitprakash

please add test coverage stats

Covering all test cases for controllers, improving it from 86.95 to 89.18

TanishqSingla · 2023-06-06T19:00:53Z

Coverage stats are as follows

file	%stmts	%Branch	%Funcs	%Lines	Uncovered Lines
constants/taskRequests.js	100	100	100	100
controllers/tasksRequests.js	89.18	85.71	100	89.18	56-57,75-76
middlewares/taskRequests.js	86.95	90	100	86.95	21,39-40
models/taskRequests.js	92.45	100	100	92.15	83-84,113-114

TanishqSingla · 2023-06-06T19:01:09Z

@iamitprakash Have commented the test coverage please check

iamitprakash

Done

middlewares/taskRequests.js

models/taskRequests.js

DashDeipayan

LGTM

* add in_discord false role for every new user (#1167) * feat: Add task request flow (#931) * feat: Add get route for tasks requests for Super User * feat: Add createTaskRequest model function * feat: Add controller for adding task request * feat: Edit assign task flow to create task reques instead Earlier in the task flow the user would automatically be assigned the task, in this change the flow is changed to create a task request first which will be approved by superuser * feat: Add controllers and model functions for task tasksRequests Update create task request model function Add approve task controller and model fucntion Add fetch tasks for user controller and model function * fix: Call createTaskRequest model function in task controller * feat: Add function to validate task request payload toFirestoreData for task request has been added This function validates the payload and returns a valid firestore object. * feat: Make create task request a working implementation from stub * feat: Add endpoint for task requests * feat: Make fetch requests working from stub * feat: Make approve task route working from stub * test: Add test for put request in task request * fix: Task request when a already existed task is requested by another user * test: Add test for already existing tasks requests * fix: task requests cached response is fetched to unauthorized users * tests: add tests for fetch request * test: Add test for approve task request * feat: Add more test for approve tasks * fix: Return response instead of throwing error in controllers * feat: update task request controller added * refactor: Add different controller for updating task request requestor Earlier createTaskRequest handled updation and creation of task request, this controller had been deconstructed into two controllers i.e createTaskRequest and addRequestor * fix: Make one function for updating and creating task request * fix: change create controller name to addOrUpdate add sinon stubs in test * fix: remove mock verify function that wraps sinon stub * Delete empty file * fix: Change createTaskRequest to addOrUpdate * fix: Task request error message for task does not exist * refactor(tasksRequests): Review comments Use shorthand firestore methode `docs` for creating array of tasksRequests Fix server errors * feat: send user to task request fetch api * fix: Check user status before creating task request Check if the user status exists before creating task request * fix: Review changes - Use array.some for requestors - Store taskId in the taskrequest obj - Fix test description for approve route on 400 request status * fix: Review comments - Fixed variable name from requestorExists to already requesting - Restrict OOO and active users to request for tasks * fix: Review comments - Remove redundant `.limit()` - Remove redundant if statement * fix: match task request data with the data-model * refactor: code restructure for taskrequests controller and models * feat: Fix visited users bug mentioned in review comments * fix: fetchTaskRequest task request requestors data * fix: Review comments * fix: Use Promise.all to fetch task requests * fix: Generic Object Sink * test: Improve test coverage Covering all test cases for controllers, improving it from 86.95 to 89.18 * [Live-Site] Feat/event room apis (#994) * docs: made the prerequisites setup more clear * docs: fixed an unwanted change * feat: initial setup for 100ms * fix: private variable issue * feat: rooms apis * feat: added jsdoc comments to the controller and model * fix: added object destructuring to payloads * test: added unit tests for models * test: added integration tests for room APIs * fix: removed logs * fix: added constant and refresh token function. * fix: replaced uuid with crypto package to generate the unique ids. * fix: unit test for event update API * fix: removed crypto module * fix: rooms to events * fix: fallback for env variables in prod * fix: changed the endActiveRoom from delete to patch * fix: env variables in prod * feat: added removeUnwantedProperties utility function * fix: added env in test config * fix: getAllRooms controller * chore: added env variables to the config * fix: sinon variable naming * fix: added www-authenticate header check and removed payload check in post method * fix: added config default value and removed extra expiration check in isTokenExpired method * fix: changed naming from rooms to events everywhere * fix: integration and unit tests according to the changes * add: added validators for response * fix: added a constant and updated the imports * fix: joinEvent API * fix: integration test for event apis * fix: utility function * fix: integration tests for events * [Fix #1173]: Add proper room_id and user_id (#1174) Co-authored-by: sanketumesh.dhabarde <sanketumesh.dhabarde@clarivate.com> * add tests for arts middleware (#1182) * add tests for join section models (#1177) * Script to Add Unverified Role to Users Without Discord Link (#1181) * add unverified role to all users who have not linked their discord id * add unit tests * add integration tests * move generate auth token for cloudflare to a util * beautify addRoleToUser function in services * add role id to test config * add unverified role id to configs * change the url from discord actions to post users --------- Co-authored-by: Tanishq Singla <tanishqsingla00@gmail.com> Co-authored-by: Prerana Nawar <61601706+prerana1821@users.noreply.github.com> Co-authored-by: Sanket Dhabarde <40385118+SanketDhabarde@users.noreply.github.com> Co-authored-by: sanketumesh.dhabarde <sanketumesh.dhabarde@clarivate.com>

feat: Add get route for tasks requests for Super User

d061557

github-advanced-security bot found potential problems Feb 28, 2023

View reviewed changes

routes/taskRequest.js Fixed Show fixed Hide fixed

TanishqSingla added 3 commits March 1, 2023 01:43

feat: Add createTaskRequest model function

a81481f

feat: Add controller for adding task request

1a74010

feat: Edit assign task flow to create task reques instead

09a021c

Earlier in the task flow the user would automatically be assigned the task, in this change the flow is changed to create a task request first which will be approved by superuser

ankushdharkar assigned TanishqSingla Mar 3, 2023

ankushdharkar added the feature task Feature that has to be built label Mar 3, 2023

ankushdharkar linked an issue Mar 3, 2023 that may be closed by this pull request

[RFC] Task Levelling new flow #904

Closed

TanishqSingla mentioned this pull request Mar 4, 2023

API contract for Task Requests RealDevSquad/website-api-contracts#111

Open

vinayak-trivedi reviewed Mar 6, 2023

View reviewed changes

controllers/tasks.js Outdated Show resolved Hide resolved

TanishqSingla added 10 commits March 10, 2023 02:35

feat: Add controllers and model functions for task tasksRequests

bea40d7

Update create task request model function Add approve task controller and model fucntion Add fetch tasks for user controller and model function

fix: Call createTaskRequest model function in task controller

a1f0eb5

feat: Add function to validate task request payload

181989b

toFirestoreData for task request has been added This function validates the payload and returns a valid firestore object.

feat: Make create task request a working implementation from stub

ec12e67

feat: Add endpoint for task requests

26f80c2

feat: Make fetch requests working from stub

ff408b2

feat: Make approve task route working from stub

eaa1b9e

test: Add test for put request in task request

c09341d

fix: Task request when a already existed task is requested by another…

283a09c

… user

test: Add test for already existing tasks requests

c6d0c0f

github-advanced-security bot found potential problems Mar 14, 2023

View reviewed changes

routes/taskRequests.js Fixed Show fixed Hide fixed

routes/taskRequests.js Fixed Show fixed Hide fixed

routes/taskRequests.js Fixed Show fixed Hide fixed

TanishqSingla added 2 commits March 15, 2023 18:49

fix: task requests cached response is fetched to unauthorized users

a6c7115

tests: add tests for fetch request

65d0f9c

github-advanced-security bot found potential problems Mar 15, 2023

View reviewed changes

TanishqSingla added 2 commits March 15, 2023 19:59

test: Add test for approve task request

27438fe

feat: Add more test for approve tasks

ceaed1b

TanishqSingla changed the title ~~feat: Add get route for tasks requests for Super User~~ feat: Add task request flow Mar 15, 2023

TanishqSingla marked this pull request as ready for review March 15, 2023 14:58

iamitprakash requested changes May 24, 2023

View reviewed changes

TanishqSingla added 2 commits May 25, 2023 01:38

feat: Fix visited users bug mentioned in review comments

8a25590

fix: fetchTaskRequest task request requestors data

406d4a3

DashDeipayan reviewed May 25, 2023

View reviewed changes

models/taskRequests.js Show resolved Hide resolved

DashDeipayan reviewed May 25, 2023

View reviewed changes

models/taskRequests.js Show resolved Hide resolved

DashDeipayan suggested changes May 25, 2023

View reviewed changes

fix: Review comments

216dca2

github-advanced-security bot found potential problems May 26, 2023

View reviewed changes

DashDeipayan suggested changes May 27, 2023

View reviewed changes

fix: Use Promise.all to fetch task requests

e350ca9

DashDeipayan previously approved these changes Jun 4, 2023

View reviewed changes

iamitprakash requested changes Jun 4, 2023

View reviewed changes

middlewares/taskRequests.js Show resolved Hide resolved

models/taskRequests.js Show resolved Hide resolved

fix: Generic Object Sink

d3ceeed

TanishqSingla dismissed DashDeipayan’s stale review via d3ceeed June 4, 2023 23:12

iamitprakash requested changes Jun 6, 2023

View reviewed changes

test: Improve test coverage

1e0a5f8

Covering all test cases for controllers, improving it from 86.95 to 89.18

TanishqSingla requested review from DashDeipayan and iamitprakash June 6, 2023 19:05

iamitprakash approved these changes Jun 9, 2023

View reviewed changes

middlewares/taskRequests.js Show resolved Hide resolved

models/taskRequests.js Show resolved Hide resolved

DashDeipayan approved these changes Jun 11, 2023

View reviewed changes

prakashchoudhary07 merged commit e5a38c4 into RealDevSquad:develop Jun 16, 2023

RitikJaiswal75 mentioned this pull request Jun 21, 2023

Sync dev to main #1186

Merged

9 tasks

Conversation

TanishqSingla commented Feb 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Task Request Flow

Uh oh!

Uh oh!

TanishqSingla commented Mar 4, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Check failure

vinayak-trivedi commented Mar 16, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TanishqSingla May 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DashDeipayan May 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iamitprakash May 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

DashDeipayan left a comment

Choose a reason for hiding this comment

Uh oh!

Check failure

Check failure

DashDeipayan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DashDeipayan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

iamitprakash left a comment

Choose a reason for hiding this comment

Uh oh!

TanishqSingla commented Jun 6, 2023

Uh oh!

TanishqSingla commented Jun 6, 2023

Uh oh!

iamitprakash left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

DashDeipayan left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

TanishqSingla commented Feb 27, 2023 •

edited

Loading

TanishqSingla May 24, 2023 •

edited

Loading

DashDeipayan May 25, 2023 •

edited

Loading

iamitprakash May 26, 2023 •

edited

Loading