GitHub duplication Fixed and Migration #1044

iamYashSinha · 2023-04-24T15:30:18Z

Close: [RFC] GitHub Duplication #957

Added a new field (github_user_id) to the Firestore database.

2. Refer to the video

2023-04-24.21-31-14.mp4

Tests Added:

UNIT TESTS
✔ should add the github_user_id to the user collection
✔ should update the github_user_id in the user collection (40ms)
✔ should be stored correctly in the database
INTEGRATION TESTS
✔ Should update the user
✔ Should return an unauthorized error when not logged in

Test Coverage Stats

Controllers

RFC DOCUMENT
MIGRATION DOCUMENT
Ticket for cleanup strategy Remove temporary/one-time-use migration endpoint for GitHub duplication fix #1331

…ackend into develop

prakashchoudhary07

Nicely done 🚀

prakashchoudhary07

Tests are failing, please check

prakashchoudhary07 · 2023-04-24T20:03:46Z

Can you please add tests for the case where the GitHub id is updated?

vikhyat187 · 2023-04-25T00:39:47Z

The test are failing rest things, look good.

iamYashSinha · 2023-04-25T01:18:36Z

Yes working on the test cases.

prakashchoudhary07 · 2023-04-25T03:23:46Z

@iamYashSinha Please share the migration plan for existing uses, how will we add github_user_id to them?

iamYashSinha · 2023-04-26T00:48:12Z

Migration Plan

initially, the plan was to revoke all the users & tell them to reauthorize by visiting the Real Dev Squad website. But this method ended up creating another document of that user and then adding the github_user_id field to it. As of now, I've written the logic for the new users, but in order to do migration I've to find a way where can add the new github_user_id field to the existing user without creating any other document
@prakashchoudhary07

iamYashSinha · 2023-04-26T00:51:43Z

Can you please add tests for the case where the GitHub id is updated?

Tests Added:

iamYashSinha · 2023-04-26T15:55:57Z

@iamYashSinha Please share the migration plan for existing uses, how will we add github_user_id to them?
Ans - will be matching the github_id (which is storing the GitHub username) of two documents and if a match is found, will be inserting the github_user_id field. Refer to the attached video @prakashchoudhary07

2023-04-26.21-20-29.mp4

…into bugfix/issue957

…ackend into bugfix/issue957

heyrandhir

One concern that comes to mind is the possibility of the request timing out. Since we are performing multiple operations for all users, there is a risk that it could impact the production data or what if some docs are changed and some fail before the request is timed out 👀

heyrandhir · 2023-07-03T15:52:34Z

controllers/users.js

+                const invalidUsers = {
+                  userId: `${userId}`,
+                  username: `${username}`,
+                  githubUsername: `${githubUsername}`,
+                };


Nit : we could have just used the shorter format of creating objects instead of template literals

const invalidUsers = {userId, username, githubUsername };

heyrandhir · 2023-07-03T15:57:13Z

controllers/users.js

+            })
+            .catch((error) => {
+              if (error.response && error.response.status === 404) {
+                countUserNotFound++;


shouldn't count not found ++ and usersNotFound.push happen before the if block as if it doesnt go inside the if block then there will be a mismatch

Thanks for pointing it out. Updated the code

iamitprakash

please add test coverage stats

controllers/users.js

RitikJaiswal75 · 2023-07-26T10:03:01Z

controllers/users.js

 };

+// one time script function to perform the migration - adding github_user_id field to the document
+const migrate = async (req, res) => {


could we use a better name for this please

Updated the name to addGtihubId

RitikJaiswal75 · 2023-07-26T10:04:40Z

controllers/users.js

+    const batchCount = Math.ceil(totalUsers / 500);
+    // Create batch write operations for each batch of documents
+    for (let i = 0; i < batchCount; i++) {
+      const batchDocs = usersSnapshot.docs.slice(i * 500, (i + 1) * 500);


rather than doing this why not directly fetch just 500 from db itself? something like paginated ones.

It will lead to extra reads on our database as in firestore it reads all the documents up from the startAt document id and also this is simpler in terms of code readability

RitikJaiswal75 · 2023-07-26T10:05:51Z

controllers/users.js

+      data: {
+        totalUsers: totalUsers,
+        usersUpdated: countUserFound,
+        usersNotUpdated: countUserNotFound,


if we are sending this i think we should add the reason as well for not updating.

There can be different reasons. So IMO, in such a case we can check our logs for the reason

RitikJaiswal75 · 2023-07-26T10:10:57Z

test/integration/users.test.js

+      Sinon.restore();
+    });
+
+    it("Should update the user", async function () {


i think the title must be should add github_id to the user

Updated the title

RitikJaiswal75 · 2023-07-26T10:11:34Z

test/integration/users.test.js

+        .post(`/users/migrate`)
+        .set("Cookie", `${cookieName}=${superUserAuthToken}`);
+      expect(usersMigrateResponse).to.have.status(200);
+      expect(usersMigrateResponse.body).to.eql({


please use to deep equal or strict equal

Updated the code

test/integration/users.test.js

…ackend into bugfix/issue957

routes/users.js

 router.get("/suggestedUsers/:skillId", authenticate, authorizeRoles([SUPERUSER]), users.getSuggestedUsers);

+// WARNING!! - One time Script/Route to do migration.
+router.post("/migrate", authenticate, authorizeRoles([SUPERUSER]), users.addGithubId);


RitikJaiswal75

Please make changes of removing the github_user_id from a few users and then testing if your script adds the field to the users.

RitikJaiswal75 · 2023-07-26T17:51:49Z

routes/users.js

 router.get("/suggestedUsers/:skillId", authenticate, authorizeRoles([SUPERUSER]), users.getSuggestedUsers);

+// WARNING!! - One time Script/Route to do migration.
+router.post("/migrate", authenticate, authorizeRoles([SUPERUSER]), users.addGithubId);


migrate represents an action/verb verbs should not be a part of the API name

github-id-migrator or user-data-migrator
Is this fine?

shubham-y · 2023-07-26T20:37:44Z

Please make changes of removing the github_user_id from a few users and then testing if your script adds the field to the users.

Updated the test to add a user without github_user_id field and testing if the script added the field or not

website-backend/test/integration/users.test.js

Lines 1259 to 1262 in 7f79193

    
           beforeEach(async function () { 
        
             await addUser(userWithoutGithubUserId); 
        
             fetchStub = Sinon.stub(axios, "get"); 
        
           });

website-backend/test/integration/users.test.js

Lines 1286 to 1290 in 7f79193

    
           const usersResponse = await chai.request(app).get(`/users`).set("cookie", `${cookieName}=${superUserAuthToken}`); 
        
           expect(usersResponse).to.have.status(200); 
        
           usersResponse.body.users.forEach((document) => { 
        
             expect(document).to.have.property(`github_user_id`); 
        
           });

shubham-y added 13 commits February 26, 2022 15:17

Merge branch 'develop' into main-resolve-conflict

5941bc5

Merge branch 'develop' of https://github.com/Real-Dev-Squad/website-b…

5aad16d

…ackend into develop

Merge branch 'develop' of https://github.com/Real-Dev-Squad/website-b…

7358662

…ackend into develop

Merge branch 'develop' of https://github.com/Real-Dev-Squad/website-b…

0c2e8ce

…ackend into develop

Merge branch 'develop' of https://github.com/Real-Dev-Squad/website-b…

0ee25fd

…ackend into develop

Merge branch 'develop' of https://github.com/Real-Dev-Squad/website-b…

d54f16a

…ackend into develop

Merge branch 'develop' of https://github.com/Real-Dev-Squad/website-b…

4e5aff3

…ackend into develop

Merge branch 'develop' of https://github.com/Real-Dev-Squad/website-b…

9afcbd0

…ackend into develop

Merge branch 'develop' of https://github.com/Real-Dev-Squad/website-b…

0fbfc9a

…ackend into develop

Merge branch 'develop' of https://github.com/Real-Dev-Squad/website-b…

cc487d9

…ackend into develop

Merge branch 'develop' of https://github.com/Real-Dev-Squad/website-b…

a5ca925

…ackend into develop

Merge branch 'develop' of https://github.com/Real-Dev-Squad/website-b…

f01f146

…ackend into develop

Merge branch 'develop' of https://github.com/Real-Dev-Squad/website-b…

558dd2d

…ackend into develop

iamYashSinha marked this pull request as draft April 24, 2023 15:31

iamYashSinha changed the title ~~fixing_doc_duplication~~ GitHub duplication Fixed Apr 24, 2023

iamYashSinha marked this pull request as ready for review April 24, 2023 16:37

iamYashSinha requested review from prakashchoudhary07, heyrandhir and vinit717 April 24, 2023 17:15

prakashchoudhary07 approved these changes Apr 24, 2023

View reviewed changes

prakashchoudhary07 previously requested changes Apr 24, 2023

View reviewed changes

iamYashSinha added 2 commits April 26, 2023 22:02

fixingdoc_duplication

adaf3cb

fixed linting error

8d8232c

iamYashSinha added 5 commits May 20, 2023 09:41

fixed integration test

ae3bfd8

fixed sinon.stub and resolves method

9569688

fixed sinon.stub timeout

5484294

fixed test cases

d20932d

auth token yet to be generated

6d309e3

iamYashSinha force-pushed the bugfix/issue957 branch from cae90c7 to 6d309e3 Compare June 11, 2023 14:41

iamYashSinha and others added 8 commits June 11, 2023 20:14

Merge branch 'develop' into bugfix/issue957

240ccb9

add missing closing bracket

d0ed63c

fix conflict in users integration test file

c3bd1db

Merge branch 'develop' of https://github.com/shubham-y/website-backend …

2dc68a5

…into bugfix/issue957

Merge branch 'develop' of https://github.com/Real-Dev-Squad/website-b…

e9a0819

…ackend into bugfix/issue957

add github_user_id in user fixtures

ed2dfc3

update response data for migrate function

7b57924

update tests for migrate route

97f0959

shubham-y requested review from prakashchoudhary07, RitikJaiswal75 and heyrandhir July 3, 2023 05:59

heyrandhir suggested changes Jul 3, 2023

View reviewed changes

iamitprakash requested changes Jul 3, 2023

View reviewed changes

shubham-y added 2 commits July 5, 2023 23:38

update tests for migrate route

0dfda95

update response data for migrate function

b2c933e

RitikJaiswal75 reviewed Jul 26, 2023

View reviewed changes

shubham-y added 3 commits July 26, 2023 22:17

Merge branch 'develop' of https://github.com/Real-Dev-Squad/website-b…

8158773

…ackend into bugfix/issue957

update controller function

31ad08a

refactor tests

708d57e

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

View reviewed changes

RitikJaiswal75 reviewed Jul 26, 2023

View reviewed changes

update integration test

7f79193

ravikumar1002 mentioned this pull request Aug 3, 2023

Feat/GitHub created at #1362

Merged

GitHub duplication Fixed and Migration #1044

Are you sure you want to change the base?

GitHub duplication Fixed and Migration #1044

Uh oh!

Conversation

iamYashSinha commented Apr 24, 2023 • edited by shubham-y Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Close: [RFC] GitHub Duplication #957

Uh oh!

prakashchoudhary07 left a comment

Choose a reason for hiding this comment

Uh oh!

prakashchoudhary07 left a comment

Choose a reason for hiding this comment

Uh oh!

prakashchoudhary07 commented Apr 24, 2023

Uh oh!

vikhyat187 commented Apr 25, 2023

Uh oh!

iamYashSinha commented Apr 25, 2023

Uh oh!

prakashchoudhary07 commented Apr 25, 2023

Uh oh!

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

Uh oh!

Migration Plan

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

heyrandhir 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iamitprakash left a comment

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Check failure

RitikJaiswal75 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shubham-y Jul 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!

shubham-y commented Jul 26, 2023

iamYashSinha commented Apr 24, 2023 •

edited by shubham-y

Loading

iamYashSinha commented Apr 26, 2023 •

edited

Loading

iamYashSinha commented Apr 26, 2023 •

edited

Loading

iamYashSinha commented Apr 26, 2023 •

edited

Loading

shubham-y Jul 26, 2023 •

edited

Loading