Skip to content

Conversation

VinuB-Dev
Copy link
Contributor

@VinuB-Dev VinuB-Dev commented Nov 15, 2024

Date: 15/11/2024
Developer Name: Vignesh B S


Issue Ticket Number

Closes Real-Dev-Squad/website-dashboard#888

Description

Implemented changes in users API for Tracking Departed Users.

  • users?departed=true - Retrieves users who had a task assigned and have departed the Discord server.

Added test cases for the above api.

Documentation Updated?

  • Yes
  • No

Under Feature Flag

  • Yes
  • No

Database Changes

  • Yes
  • No

Breaking Changes

  • Yes
  • No

Development Tested?

  • Yes
  • No

Screenshots

Screenshot 1

Screenshot 2024-11-15 at 3 34 37 PM

Test Coverage

Screenshot 1

Screenshot 2024-11-15 at 3 33 11 PM

Additional Notes

PRD
Design Doc
Have moved the archieveUsers function to models as it was directly interacts with the database and performing operations such as updating task documents. This was also causing circular dependency issues.

- users/departed-users retrieves users with assigned tasks who have left the Discord server

Added test cases for the API and fixed test case issues arising from mock data additions used in current API testing.
@VinuB-Dev VinuB-Dev changed the title Created users/departed-users API Feat: Implement APIs for Tracking Departed Users with Assigned Tasks Nov 15, 2024
@vinit717
Copy link
Member

Are pr 2251 and 2252 stack PR? and In what order I need to review them?

… and the corresponding queries to model.

- removed test cases from integration testing and moved it to unit testing/
- moved archieveUsers to users model as it was causing circular dependency.
- created a separate fixture file for mock data required to test the feature.
- reverted the test case changes done for other test cases due to mock data changes.
@VinuB-Dev
Copy link
Contributor Author

Are pr 2251 and 2252 stack PR? and In what order I need to review them?

There were 2 api's in the original PR, I have separated both of them into separate PR's.
Each one can be reviewed independently.

Copy link
Contributor

@pankajjs pankajjs left a comment

Choose a reason for hiding this comment

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

Left few comments @VinuB-Dev

- Added function documentation for all the modified and new model functions.
- Implemented a negative unit test case for the `fetchUsersNotInDiscordServer` function.
- Updated `fetchUsersNotInDiscordServer` to use `userId` as a parameter instead of the entire `user` object.
- Added test cases for recent model changes.
@pankajjs
Copy link
Contributor

Not under feature flag? @VinuB-Dev i

@VinuB-Dev
Copy link
Contributor Author

@vinit717 Should we put these APIs under a feature flag?

The APIs are read-only, fetching data from the database without performing any write operations. They are completely independent, involve straightforward database queries, and are not dependent on external factors. Considering their simplicity, I feel we don’t need to place them under a feature flag.

@pankajjs
Copy link
Contributor

@VinuB-Dev You have moved archivedUser function to different file. Have you fixed all the imports statement?

@VinuB-Dev
Copy link
Contributor Author

@VinuB-Dev You have moved archivedUser function to different file. Have you fixed all the imports statement?

Yes I have and all test cases are working fine.

VinuB-Dev and others added 3 commits November 19, 2024 09:14
…the /users/departed-users API.

- Implemented negative test cases for the /users/departed-users endpoint:
  - Tested the scenario where the dev flag is not provided, expecting a 404 status code with a "Route not found" message.
  - Tested the scenario where the database query fails, ensuring the API responds with a 500 status code and an appropriate error message.
@tejaskh3
Copy link
Contributor

as this PR is outdated compared to develop, Could you please pull the latest changes from develop into your branch and resolve any conflicts if they arise?

@VinuB-Dev
Copy link
Contributor Author

as this PR is outdated compared to develop, Could you please pull the latest changes from develop into your branch and resolve any conflicts if they arise?

Done

…nality under /users route with departed parameter. Added test cases for the same.
});
} catch (error) {
logger.error("Error when fetching users who abandoned tasks:", error);
return res.boom.badImplementation(INTERNAL_SERVER_ERROR);
Copy link
Contributor

Choose a reason for hiding this comment

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

What will be the status code?

Copy link
Contributor Author

@VinuB-Dev VinuB-Dev Nov 24, 2024

Choose a reason for hiding this comment

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

It'll be 500
You can see ti at the end here - https://www.npmjs.com/package/boom/v/2.5.0?activeTab=readme

Copy link
Contributor

@pankajjs pankajjs left a comment

Choose a reason for hiding this comment

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

Left few comments

Comment on lines 200 to 201
try {
result = await dataAccess.retrieveUsers({ query: req.query });
Copy link
Contributor

Choose a reason for hiding this comment

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

what will be the query 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.

It'll be { departed: "true" }

return res.boom.notFound("Route not found");
}
try {
const result = await dataAccess.retrieveUsers({ query: req.query });
Copy link
Contributor

Choose a reason for hiding this comment

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

What is it returning?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

An object
{ users: [], nextId: '' , prevId: ''}

Copy link
Member

@Achintya-Chatterjee Achintya-Chatterjee left a comment

Choose a reason for hiding this comment

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

please write API contract for this API

@VinuB-Dev
Copy link
Contributor Author

please write API contract for this API

@Achintya-Chatterjee Where should I add it? In the description or somewhere else?

@Achintya-Chatterjee
Copy link
Member

Achintya-Chatterjee commented Nov 25, 2024

please write API contract for this API

@Achintya-Chatterjee Where should I add it? In the description or somewhere else?

you have to add it here

@VinuB-Dev
Copy link
Contributor Author

VinuB-Dev commented Nov 26, 2024

Splitting this PR into smaller parts for easier review. This PR will serve as the main split: #2268. Closing this PR as part of the process.

@VinuB-Dev VinuB-Dev closed this Nov 26, 2024
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.

Implement APIs for Tracking Orphaned Tasks and Departed Users.
5 participants