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: Integrate userData into Progresses API to reduce redundant calls #2311

Open
wants to merge 8 commits into
base: develop
Choose a base branch
from

Conversation

AnujChhikara
Copy link

@AnujChhikara AnujChhikara commented Dec 21, 2024

Date: 21 Dec 2024

Developer Name: Anuj Chhikara


Issue Ticket Number

Description

This PR introduces a change to include the full userData (e.g., username, profile picture) alongside the progress data when fetching progress updates. Currently, the backend only returns the userId, requiring the frontend to make an additional API call to fetch user details. By including userData directly with the progress object, we aim to reduce the number of API calls and improve frontend performance and simplicity.

Documentation Updated?

  • Yes
  • No

Under Feature Flag

  • Yes
  • No

Database Changes

  • Yes
  • No

Breaking Changes

  • Yes
  • No

Development Tested?

  • Yes
  • No

Screenshots

Screenshot 1
  • Without dev flag
    ShareX_9fydyPImyR

  • With dev flag
    Postman_CKua0H9mYP

Test Coverage

Screenshot 1

image
image
image

Additional Notes

@Achintya-Chatterjee Achintya-Chatterjee changed the title Include userData in Progresses API responses feat: Include userData in Progresses API responses Dec 23, 2024
Comment on lines +55 to +72

if (dev) {
try {
const uniqueUserIds = [...new Set(progressDocs.map((doc) => doc.userId))];
const batchSize = 500;

const batches = Array.from({ length: Math.ceil(uniqueUserIds.length / batchSize) }, (_, index) =>
uniqueUserIds.slice(index * batchSize, index * batchSize + batchSize)
);

const batchPromises = batches.map((batch) => Promise.all(batch.map((userId) => fetchUser({ userId }))));

const usersDataBatches = await Promise.all(batchPromises);

const usersData = usersDataBatches.flat();

const userLookupMap = usersData.reduce((lookup, { user }) => {
if (user) lookup[user.id] = user;

Choose a reason for hiding this comment

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

write tests for this.

Copy link
Author

Choose a reason for hiding this comment

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

Hello @Achintya-Chatterjee I just need some clarification regarding the tests. I can add a test to check that userData returns null if the operation fails, but I'm also wondering how to test for scenarios with a large number of unique users. With our current test setup, I would need to add 1000+ users to Firestore to test it.

If you want me to proceed with that approach, please let me know!

@vinit717
Copy link
Member

Please fix the title as commit message is not the title

Comment on lines +55 to +65

if (dev) {
try {
const uniqueUserIds = [...new Set(progressDocs.map((doc) => doc.userId))];
const batchSize = 500;

const batches = Array.from({ length: Math.ceil(uniqueUserIds.length / batchSize) }, (_, index) =>
uniqueUserIds.slice(index * batchSize, index * batchSize + batchSize)
);

const batchPromises = batches.map((batch) => Promise.all(batch.map((userId) => fetchUser({ userId }))));
Copy link
Member

Choose a reason for hiding this comment

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

Please refactor this and why we need this?

Copy link
Author

Choose a reason for hiding this comment

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

Achintya asked me to use the batches approach as Promise.all could cause performance issues for large datasets. You can check the prevoius resolved messages

@AnujChhikara AnujChhikara changed the title feat: Include userData in Progresses API responses feat: Integrate userData into Progresses API to reduce redundant calls Dec 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.

Include Full User Details in Task Progress Updates
4 participants