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

Feature/one time user colors #735

Open
wants to merge 37 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 35 commits
Commits
Show all changes
37 commits
Select commit Hold shift + click to select a range
2fed9ad
✨feat(addDefaultColor):
isVivek99 Sep 23, 2022
0f505b7
Merge branch 'develop' into feature/one-time-user-colors
vivek-geekyants Oct 4, 2022
cd2c96d
feat(user-colors):added code to add user colors
vivek-geekyants Oct 6, 2022
5bd0f9a
Merge branch 'develop' into feature/one-time-user-colors
vivek-geekyants Oct 6, 2022
5e4f75a
🐞 fix():removed console logs
vivek-geekyants Oct 6, 2022
04c43e7
fixes+chores():fixed PR review changes and changed the filenames plus…
isVivek99 Oct 13, 2022
964dd56
chore(): dele commented code
isVivek99 Oct 13, 2022
0f65a11
chore(one-time-user-color): chores
isVivek99 Oct 17, 2022
146304a
fix(fixtures): changed color property
isVivek99 Oct 17, 2022
01c8138
fix():changed file structure suggested in the PR
isVivek99 Oct 18, 2022
7a75444
fix: helpers.js
isVivek99 Oct 26, 2022
54c94ce
chore():fixed migration filenames
isVivek99 Nov 5, 2022
f4d60d3
fix(userMigrations):
isVivek99 Nov 5, 2022
64f56cc
chore():removed unwanted comment
isVivek99 Nov 5, 2022
b210076
fix(usercolorArray): added constant
isVivek99 Nov 10, 2022
804c64c
code fix
isVivek99 Jun 9, 2023
b2c9c3c
Merge branch 'develop' into feature/one-time-user-colors
isVivek99 Jul 17, 2023
b495022
added firestore batch changes and updated tests
isVivek99 Jul 24, 2023
807df54
Update controllers/userMigrations.js
isVivek99 Aug 5, 2023
2ecedb5
review changes
isVivek99 Aug 5, 2023
4b0df82
change route name to noun
isVivek99 Aug 5, 2023
cc10b82
(#712) change route name to noun
isVivek99 Aug 5, 2023
b8c7521
update response based on review
isVivek99 Aug 6, 2023
231cbef
update test based on review
isVivek99 Aug 6, 2023
48f6ef3
(#712) moved firestore interaction inside controller and updated name…
isVivek99 Aug 8, 2023
f322598
Merge branch 'develop' of https://github.com/Real-Dev-Squad/website-b…
isVivek99 Aug 14, 2023
018f829
(#712) added test for helpers
isVivek99 Aug 14, 2023
62fb581
updated tests for the model
isVivek99 Aug 14, 2023
542d504
updated tests for the model
isVivek99 Aug 14, 2023
7e0d00a
Update users.test.js
isVivek99 Aug 16, 2023
a522f0f
adress PR comments
isVivek99 Aug 16, 2023
c723578
remove unrequired check
isVivek99 Aug 16, 2023
d76b090
Merge branch 'feature/one-time-user-colors' of https://github.com/vic…
isVivek99 Aug 16, 2023
c87fe9c
added dataAccessLayer to fetch users
isVivek99 Aug 17, 2023
6aaafd7
address failing test
isVivek99 Aug 20, 2023
c2151cd
updated helper file
isVivek99 Aug 21, 2023
0852243
update helper
isVivek99 Aug 21, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions controllers/userMigrations.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
const userQuery = require("../models/userMigrations");
const { SOMETHING_WENT_WRONG } = require("../constants/errorMessages");

/**
* Returns the lists of usernames where default colors were added
*
* @param req {Object} - Express request object
* @param res {Object} - Express response object
*/

const addDefaultColors = async (req, res) => {
try {
const usersDetails = await userQuery.addDefaultColors();

return res.json({
message: "User colors updated successfully!",
usersDetails,
});
} catch (error) {
logger.error(`Error adding default colors to users: ${error}`);
return res.boom.badImplementation(SOMETHING_WENT_WRONG);
}
};

module.exports = {
addDefaultColors,
};
63 changes: 63 additions & 0 deletions models/userMigrations.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
const firestore = require("../utils/firestore");
const userModel = firestore.collection("users");
const { getRandomIndex } = require("../utils/helpers");
const dataAccess = require("../services/dataAccessLayer");
const MAX_TRANSACTION_WRITES = 500;
const MAX_USERS_SIZE = 10_000;
const USER_COLORS = 10;

/**
* Returns the object with details about users to whom user color was added
*
* @param req {Object} - Express request object
* @param res {Object} - Express response object
*/

const addDefaultColors = async (batchSize = MAX_TRANSACTION_WRITES) => {
try {
const usersSnapshotArr = await dataAccess.retrieveUsers({ query: { size: MAX_USERS_SIZE } });
const usersArr = usersSnapshotArr.users;
// usersSnapshot.users.forEach((doc) => usersArr.push({ id: doc.id, ...doc }));
isVivek99 marked this conversation as resolved.
Show resolved Hide resolved

const batchArray = [];
const users = [];
batchArray.push(firestore.batch());
let operationCounter = 0;
let batchIndex = 0;
let totalCount = 0;

for (const user of usersArr) {
const colors = user.colors ?? {};

if (!user.colors) {
const userColorIndex = getRandomIndex(USER_COLORS);
colors.color_id = userColorIndex;
const docId = userModel.doc(user.id);
user.colors = colors;
batchArray[parseInt(batchIndex)].set(docId, user);
operationCounter++;
totalCount++;
users.push(user.username);
if (operationCounter === batchSize) {
batchArray.push(firestore.batch());
batchIndex++;
operationCounter = 0;
}
}
}
batchArray.forEach(async (batch) => await batch.commit());

return {
totalUsersFetched: usersArr.length,
totalUsersUpdated: totalCount,
totalUsersUnaffected: usersArr.length - totalCount,
};
} catch (err) {
logger.error("Error adding default colors to users", err);
throw err;
}
};

module.exports = {
addDefaultColors,
};
1 change: 1 addition & 0 deletions routes/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ app.use("/users/status", require("./userStatus.js"));
app.use("/users", require("./users.js"));
app.use("/profileDiffs", require("./profileDiffs.js"));
app.use("/wallet", require("./wallets.js"));
app.use("/migrations", require("./userMigrations.js"));
Copy link
Contributor

@heyrandhir heyrandhir Aug 20, 2023

Choose a reason for hiding this comment

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

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

app.use("/extension-requests", require("./extensionRequests"));
app.use("/tags", require("./tags.js"));
app.use("/levels", require("./levels.js"));
Expand Down
10 changes: 10 additions & 0 deletions routes/userMigrations.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
const express = require("express");
const router = express.Router();
const authenticate = require("../middlewares/authenticate");
const authorizeRoles = require("../middlewares/authorizeRoles");
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

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.

module.exports = router;
3 changes: 3 additions & 0 deletions test/fixtures/user/user.js
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,9 @@ module.exports = () => {
archived: false,
in_discord: true,
},
colors: {
color_id: 2,
},
picture: {
publicId: "profile/mtS4DhUvNYsKqI7oCWVB/aenklfhtjldc5ytei3ar",
url: "https://res.cloudinary.com/realdevsquad/image/upload/v1667685133/profile/mtS4DhUvNYsKqI7oCWVB/aenklfhtjldc5ytei3ar.jpg",
Expand Down
71 changes: 71 additions & 0 deletions test/integration/userMigrations.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
const chai = require("chai");
const { expect } = chai;
const chaiHttp = require("chai-http");

const app = require("../../server");
const authService = require("../../services/authService");
const addUser = require("../utils/addUser");
const cleanDb = require("../utils/cleanDb");
// Import fixtures
const userData = require("../fixtures/user/user")();
const superUser = userData[4];
const nonSuperUser = userData[0];
const colorBearingUsernames = [superUser.username, nonSuperUser.username];
isVivek99 marked this conversation as resolved.
Show resolved Hide resolved

const config = require("config");
const cookieName = config.get("userToken.cookieName");

chai.use(chaiHttp);

describe("userColorMigrations", function () {
let superUserId;
let superUserAuthToken;
let userId = "";
let nonSuperUserId = "";
beforeEach(async function () {
userId = await addUser(nonSuperUser);
superUserId = await addUser(superUser);
nonSuperUserId = userId;
superUserAuthToken = authService.generateAuthToken({ userId: superUserId });
});

afterEach(async function () {
await cleanDb();
});

describe("PATCH /migrations/user-default-color", function () {
it("Should return 401 if user is not a super user", function (done) {
const nonSuperUserJwt = authService.generateAuthToken({ userId: nonSuperUserId });
chai
.request(app)
.patch(`/migrations/user-default-color`)
.set("cookie", `${cookieName}=${nonSuperUserJwt}`)
.end((err, res) => {
if (err) {
return done(err);
}
expect(res).to.have.status(401);
expect(res.body).to.be.a("object");
expect(res.body.message).to.equal("You are not authorized for this action.");
return done();
});
});
it("Should add default color property to all users,using authorized user (super_user)", function (done) {
chai
.request(app)
.patch(`/migrations/user-default-color`)
.set("cookie", `${cookieName}=${superUserAuthToken}`)
.end((err, res) => {
if (err) {
return done(err);
}

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);
Copy link
Contributor

@heyrandhir heyrandhir Aug 9, 2023

Choose a reason for hiding this comment

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

can we also write a test in which we test users who have the color property pre-existing are not affected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure , i had that check, but after removing usernames i removed it, ill add it back in some other. way

Copy link
Contributor

Choose a reason for hiding this comment

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

@isVivek99 still can't see the assertion for users who have the color property pre-existing and are not affected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i have added a similar test in the model test.
test/unit/models/userMigrations.test.js
does this suffice?

Copy link
Contributor

@heyrandhir heyrandhir Aug 20, 2023

Choose a reason for hiding this comment

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

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. 😊

return done();
});
});
});
});
52 changes: 52 additions & 0 deletions test/unit/models/userMigrations.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
const chai = require("chai");
const { expect } = chai;
const firestore = require("../../../utils/firestore");
const userModel = firestore.collection("users");
const cleanDb = require("../../utils/cleanDb");
const userMigrationModel = require("../../../models/userMigrations");
const userData = require("../../fixtures/user/user")();
const addUser = require("../../utils/addUser");

describe("userColorMigrations", function () {
const MAX_TRANSACTION_WRITES = 500;

beforeEach(async function () {
await addUser(userData[0]);
await addUser(userData[1]);
await addUser(userData[2]);
await addUser(userData[3]);
await addUser(userData[4]);
await addUser(userData[6]);
});
afterEach(async function () {
await cleanDb();
});

it("should add color property to added users which dont have a color property", async function () {
const response = await userMigrationModel.addDefaultColors();

expect(response.totalUsersFetched).to.equal(6);
expect(response.totalUsersUpdated).to.equal(5);
expect(response.totalUsersUnaffected).to.equal(1);
});
it("should make sure that batch updates are working properly by passing smaller batch size", async function () {
const SMALL_BATCH_SIZE = 2;
const response = await userMigrationModel.addDefaultColors(SMALL_BATCH_SIZE);
expect(response.totalUsersFetched).to.equal(6);
expect(response.totalUsersUpdated).to.equal(5);
expect(response.totalUsersUnaffected).to.equal(1);
});
it("should not affect users already having color property", async function () {
// Manually add a color property to a user
const userId = await addUser(userData[0]);
await userModel.doc(userId).update({ colors: { color_id: 3 } });
const response = await userMigrationModel.addDefaultColors(MAX_TRANSACTION_WRITES);
expect(response.totalUsersFetched).to.equal(6);
expect(response.totalUsersUpdated).to.equal(4);
expect(response.totalUsersUnaffected).to.equal(2);
heyrandhir marked this conversation as resolved.
Show resolved Hide resolved

// Check that the user with a color property was unaffected
const updatedUser = await userModel.doc(userId).get();
expect(updatedUser.data().colors.color_id).to.equal(3);
});
});
20 changes: 20 additions & 0 deletions test/unit/utils/helpers.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
const chai = require("chai");
const { getRandomIndex } = require("../../../utils/helpers");
const { expect } = chai;

describe("helpers", function () {
describe("getRandom Index from function", function () {
it("should return a random number between 0 and 10 excluding 10 if no index is passed", function () {
const result = getRandomIndex();
expect(result).to.be.at.least(0);
expect(result).to.be.below(10);
});

it("expect a number between 0 and passed number", function () {
const delimiter = 100;
const result = getRandomIndex(delimiter);
expect(result).to.be.at.least(0);
expect(result).to.be.below(delimiter);
});
});
});
12 changes: 12 additions & 0 deletions utils/helpers.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
/**
* Returns a random object from the array of colors to user
* @param array {array} : array containing objects
* @returns random Index number : index between the range 0 to array.length
*/
const getRandomIndex = (arrayLength = 10) => {
heyrandhir marked this conversation as resolved.
Show resolved Hide resolved
heyrandhir marked this conversation as resolved.
Show resolved Hide resolved
return Math.floor(Math.random() * arrayLength);
};
heyrandhir marked this conversation as resolved.
Show resolved Hide resolved

module.exports = {
getRandomIndex,
};