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.

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

feat: API to sync in_discord role with discord members and add joined_discord date #1097

Merged
merged 26 commits into from
Jun 7, 2023

Conversation

bhtibrewal
Copy link
Contributor

@bhtibrewal bhtibrewal commented May 17, 2023

Create an endpoint to look into the users who have not linked their discord or are not in discord

Coverage

models:
image

controllers:
image
THe lines wrote by me are covered

@bhtibrewal bhtibrewal marked this pull request as draft May 17, 2023 13:46
routes/users.js Fixed Show resolved Hide resolved
@bhtibrewal bhtibrewal marked this pull request as ready for review May 21, 2023 19:21
@bhtibrewal bhtibrewal self-assigned this May 22, 2023
@bhtibrewal bhtibrewal added the feature task Feature that has to be built label May 22, 2023
@heyrandhir
Copy link
Contributor

yarn test is not passing. can you please check.

Copy link
Contributor

@heyrandhir heyrandhir left a comment

Choose a reason for hiding this comment

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

Request you to share the updated data model and write tests as well.

controllers/users.js Outdated Show resolved Hide resolved
controllers/users.js Outdated Show resolved Hide resolved
controllers/users.js Outdated Show resolved Hide resolved
controllers/users.js Outdated Show resolved Hide resolved
controllers/users.js Outdated Show resolved Hide resolved
controllers/users.js Outdated Show resolved Hide resolved
models/users.js Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
redisCredentials.js Outdated Show resolved Hide resolved
routes/users.js Fixed Show resolved Hide resolved
@bhtibrewal
Copy link
Contributor Author

bhtibrewal commented May 24, 2023

Request you to share the updated data model and write tests as well.

tracking the test task in a separate issue: #1112
updated data model: Real-Dev-Squad/website-data-models#32

@WYGIN
Copy link

WYGIN commented May 30, 2023

Please don't mind, but in users.js file line no: 504 why fetching all user data at once ? Why don't we fetch it as paginated or or executing all the paginated users in parallel with Promise.all() ?

routes/users.js Fixed Show fixed Hide fixed
routes/users.js Fixed Show fixed Hide fixed
routes/users.js Fixed Show fixed Hide fixed
routes/users.js Fixed Show fixed Hide fixed
models/users.js Outdated Show resolved Hide resolved
heyrandhir
heyrandhir previously approved these changes Jun 7, 2023
@prakashchoudhary07
Copy link
Contributor

Tests are failing?

controllers/users.js Outdated Show resolved Hide resolved
@@ -21,6 +21,7 @@
router.put("/self/intro", authenticate, userValidator.validateJoinData, users.addUserIntro);
router.get("/:id/skills", users.getUserSkills);
router.get("/:id/badges", getUserBadges);
router.patch("/", authenticate, authorizeRoles([SUPERUSER]), users.nonVerifiedDiscordUsers);

Check failure

Code scanning / CodeQL

Missing rate limiting

This route handler performs [authorization](1), but is not rate-limited. This route handler performs [authorization](2), but is not rate-limited. This route handler performs [authorization](3), but is not rate-limited.
Copy link
Member

@iamitprakash iamitprakash left a comment

Choose a reason for hiding this comment

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

please add test coverage stats

models/users.js Show resolved Hide resolved
test/integration/users.test.js Show resolved Hide resolved
models/users.js Show resolved Hide resolved
test/integration/users.test.js Show resolved Hide resolved
@RitikJaiswal75 RitikJaiswal75 merged commit 5cd5815 into develop Jun 7, 2023
@RitikJaiswal75 RitikJaiswal75 deleted the feat/sync-indiscord-role branch June 7, 2023 18:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature task Feature that has to be built
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants