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

Aggregate Wrapped Data - updated version #895

Open
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

NIDHI2023
Copy link
Contributor

Summary

Made lots of edits to base code.

  • Added new fields for favorite month, favorite day, and number of students helped for TAs
  • Changed to favorite courseId instead of sessionId and got rid of favorite title
  • Refactored code to make more readable
  • Added some optimizations when getting sessionDocs and userDocs by calling Promise.all first instead of calling await every time in a loop
  • Added progress debugging statements for when we run it on bigger data, so we know where the script is at

Test Plan

  • New fields have correct values in test data
  • user in wrapped who’s not found in userdocs does not get wrapped:true
  • user who isn’t in wrapped does not get wrapped:true

Breaking Changes

None

  • I have updated the documentation accordingly.
  • My PR adds a @ts-ignore

@NIDHI2023 NIDHI2023 requested a review from a team as a code owner November 14, 2024 22:56
@dti-github-bot
Copy link
Member

dti-github-bot commented Nov 14, 2024

[diff-counting] Significant lines: 858.

Copy link
Contributor

@rgu0114 rgu0114 left a comment

Choose a reason for hiding this comment

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

Everything seems logically correct to me! Great work with this over the past couple of months and handling complex cases with fav TA and stuff. I'm still a bit nervous about the scale of reads/writes we're running, so one option would be to subdivide the date ranges even more, but we can figure that out on Monday.

@@ -2,7 +2,7 @@ name: CD
on:
push:
branches:
- master
- main
Copy link
Contributor

Choose a reason for hiding this comment

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

oh also prob don't include this change and the one in the readme

@anchen9
Copy link
Collaborator

anchen9 commented Nov 17, 2024

Hi Nidhi! Thanks for adding the fields for favorite day, favorite month, and number of students helped! The logic for favMonth makes sense and I like that you returned them as numbers since it was very intuitive to incorporate! Your comments throughout are also very helpful when looking over your code! I did notice a few commented out console statements that maybe can be removed?

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.

5 participants