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

Added analytics to booking and laundry APIs and removed previous analytics tracking functionalities #341

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

aangelinali
Copy link

Added analytics to booking and laundry APIs to record GSR booking durations and locations, added analytics for laundry washing time minutes by dorm, removed previous analytics tracking functionalities

…ations and locations as well as laundry washing time minutes by dorm
Copy link

sentry-io bot commented Feb 27, 2025

🔍 Existing Issues For Review

Your pull request is modifying functions with the following pre-existing issues:

📄 File: backend/gsr_booking/views.py

Function Unhandled Issue
get JSONDecodeError: Expecting value: line 1 column 1 (char 0) /api/gsr/availability/{...
Event Count: 44
📄 File: backend/laundry/views.py (Click to Expand)
Function Unhandled Issue
get TypeError: 'NoneType' object is not subscriptable /api/laundry/rooms/{room_ids...
Event Count: 291
get TypeError: 'NoneType' object is not subscriptable /api/laundry/hall/{room_id}...
Event Count: 80
---

Did you find this useful? React with a 👍 or 👎

Copy link

codecov bot commented Feb 28, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.15%. Comparing base (2fe43f0) to head (9db5c4d).
Report is 3 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #341   +/-   ##
=======================================
  Coverage   88.15%   88.15%           
=======================================
  Files          67       66    -1     
  Lines        2777     2753   -24     
=======================================
- Hits         2448     2427   -21     
+ Misses        329      326    -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@dr-Jess dr-Jess left a comment

Choose a reason for hiding this comment

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

Also needs to be unit tested after changes are made. Good work so far!



# Creates a singleton of of the 'AnalyticsRecorder' class
LabsAnalytics = get_analytics_recorder(Product.MOBILE_BACKEND)
Copy link
Contributor

Choose a reason for hiding this comment

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

@vcai122 Looking at the code from DLA, it seems that this singleton creates a new one each time? I could be wrong but if so, it might be optimal to create one and pass it around as an import from settings or something.

/ 60
),
)
)
Copy link
Contributor

Choose a reason for hiding this comment

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

The logic here doesn't seem to be triggering for testing--needs to be fixed

@@ -83,6 +85,24 @@ def get_snapshot_info(room_id):
snapshots = LaundrySnapshot.objects.filter(filter).order_by("-date")
return (room, snapshots)

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's come up with a more consistent naming schema. Potentially subproject (so gsr_booking, laundry) as head, so like <subproject>.<details>.<type (ex. duration)>

Copy link
Contributor

Choose a reason for hiding this comment

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

No need for naming scheme. Library handles it intentionally so developers do not have to waste time thinking about scheme

@dr-Jess dr-Jess marked this pull request as draft February 28, 2025 23:11
@dr-Jess dr-Jess requested a review from vcai122 February 28, 2025 23:11
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.

3 participants