Feature/one time user colors#735
Conversation
added code to add color property to exisiting users
… added migration files to required folders
|
Other than the 2 comments above, everything looks good to me. We can proceed with merging after those are resolved. Thanks for the PR. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| const { SUPERUSER } = require("../constants/roles"); | ||
| const migrations = require("../controllers/userMigrations"); | ||
|
|
||
| router.patch("/user-default-color", authenticate, authorizeRoles([SUPERUSER]), migrations.addDefaultColors); |
Check failure
Code scanning / CodeQL
Missing rate limiting
so do you mean that for new users this random color constant are being added. only for the old users this is a 1 time fix ? |
heyrandhir
left a comment
heyrandhir
left a comment
There was a problem hiding this comment.
Please add testing stats and follow the standard template for description.
this will only be for existing users, I am not sure if a new user signs up we are adding them a color property, we can also have a follow up |
In this scenario, it seems more logical to write that part initially otherwise, the current script will function for all the current users, but the color property will be missing for new users and the super user will need to run this every time new users are added in the DB. Alternatively, if we add it for new users first, then we can ensure that everyone is covered. Once the script is executed, we can then remove the script for once and for all. What do you think @vivek-geekyants @isVivek99 |
| expect(res).to.have.status(200); | ||
| expect(res.body.usersDetails.totalUsersFetched).to.be.equal(colorBearingUsernames.length); | ||
| expect(res.body.usersDetails.totalUsersUpdated).to.be.equal(colorBearingUsernames.length); | ||
| expect(res.body.usersDetails.totalUsersUnaffected).to.be.equal(0); |
There was a problem hiding this comment.
can we also write a test in which we test users who have the color property pre-existing are not affected.
There was a problem hiding this comment.
sure , i had that check, but after removing usernames i removed it, ill add it back in some other. way
There was a problem hiding this comment.
@isVivek99 still can't see the assertion for users who have the color property pre-existing and are not affected.
There was a problem hiding this comment.
i have added a similar test in the model test.
test/unit/models/userMigrations.test.js
does this suffice?
There was a problem hiding this comment.
i have added a similar test in the model test. test/unit/models/userMigrations.test.js does this suffice?
it would be great if we could add in integration test. This expansion in coverage will enhance our confidence in the system and would assure everything works end to end. 😊
will be updating |
|
yes will raise a seperate |
models/userMigrations.js
Outdated
| const userColorIndex = getRandomIndex(USER_COLORS); | ||
| colors.color_id = userColorIndex; | ||
| const docId = userModel.doc(user.id); | ||
| batchArray[parseInt(batchIndex)].set(docId, { ...user, colors }); |
There was a problem hiding this comment.
Nit: Instead of resetting the entire object, we can simply update the colors field. This would be more efficient and would avoid unnecessary changes to the object.
There was a problem hiding this comment.
agree, will update
models/userMigrations.js
Outdated
| const usersSnapshot = await userModel.get(); | ||
| const usersArr = []; |
There was a problem hiding this comment.
can this be done using data access layer as we are trying to avoid accessing the user model directly !
There was a problem hiding this comment.
i thought we can only access user through id or usernames, ill check it
There was a problem hiding this comment.
I have used the dataAccessLayer for this, but the layer has only provision to fetch un-archived users, but we want to get all users, what should we do in this case?
There was a problem hiding this comment.
Your current code should suffice for now. We can proceed without blocking your pull request. Let's create a ticket to keep track of this task, and when you find the time and bandwidth, we can address it after this current work.
CC @mailmesriza98 @prakashchoudhary07 @iamitprakash @ankushdharkar
| expect(res).to.have.status(200); | ||
| expect(res.body.usersDetails.totalUsersFetched).to.be.equal(colorBearingUsernames.length); | ||
| expect(res.body.usersDetails.totalUsersUpdated).to.be.equal(colorBearingUsernames.length); | ||
| expect(res.body.usersDetails.totalUsersUnaffected).to.be.equal(0); |
There was a problem hiding this comment.
@isVivek99 still can't see the assertion for users who have the color property pre-existing and are not affected.
| app.use("/users", require("./users.js")); | ||
| app.use("/profileDiffs", require("./profileDiffs.js")); | ||
| app.use("/wallet", require("./wallets.js")); | ||
| app.use("/migrations", require("./userMigrations.js")); |
There was a problem hiding this comment.
if you are aware @ankushdharkar has strongly advised against approving pull requests that don't adhere to the correct naming convention. As migrations isn't a resource, could you kindly consider relocating this route under \users ?
https://discord.com/channels/673083527624916993/729399523268624405/1141334143867822081
12 participants
This is a one-time PR
Issue Ticket Number:- #712
API Contract: - API contract RealDevSquad/website-api-contracts#147
Frontend PR: RealDevSquad/website-members#396
Backend changes
Frontend Changes
Database changes
Breaking changes (If your feature is breaking/missing something please mention pending tickets)
Deployment notes
None
Description
PRwill add a colors id for each user which will then be used on frontend for users to see their profile is a specific color.Testing Stats:
file:
controllers/userMigrations.jsfile:
models/userMigrations.jsfile
utils/helpers.js