-
Notifications
You must be signed in to change notification settings - Fork 2
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
Add User Lifetime Statistics #309
Conversation
backend god @rachllee |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great first pass! Didn't look into the test cases, I'll trust you on that. A few comments on most files but most important comments in user_stat.py
. Let's talk more about that file at the GBM.
@@ -411,3 +411,35 @@ class Announcement(models.Model): | |||
author = models.ForeignKey(User, related_name="announcements", on_delete=models.CASCADE) | |||
time_updated = models.DateTimeField(auto_now=True) | |||
course = models.ForeignKey(Course, related_name="announcements", on_delete=models.CASCADE) | |||
|
|||
|
|||
class UserStatistic(models.Model): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good
backend/package-lock.json
Outdated
@@ -0,0 +1,27 @@ | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure what package-lock is here? should remove
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm actually also unsure- it appeared during my very convoluted env setup process but I will manually delete
backend/package.json
Outdated
@@ -0,0 +1,5 @@ | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
backend/ohq/statistics.py
Outdated
|
||
|
||
def user_calculate_questions_answered(user): | ||
num_questions = Decimal(Question.objects.filter(responded_to_by=user).count()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: should add status=Question.STATUS_ANSWERED
to make the query more accurate
backend/ohq/statistics.py
Outdated
|
||
def user_calculate_time_helped(user): | ||
user_time_helped = ( | ||
Question.objects.filter(asked_by=user) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this query work if the question was rejected or withdrawn? In that case the time_response_started
field would be null. If it doesn't, perhaps filter for where the question status is answered?
backend/ohq/statistics.py
Outdated
UserStatistic.objects.update_or_create( | ||
user=user, | ||
metric=UserStatistic.METRIC_TOTAL_TIME_BEING_HELPED, | ||
defaults={"value": time.seconds if time else Decimal('0.00000000')}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am more inclined to not call update_or_create
at all if time.seconds
is None
instead of updating the value to be 0.
backend/ohq/statistics.py
Outdated
|
||
|
||
def user_calculate_time_helping(user): | ||
user_time_helping = ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto for comments regarding user_calculate_time_helped
else yesterday | ||
) | ||
|
||
course_questions = Question.objects.filter(queue__course=course, time_asked__gte=date) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from django.db.models import Q
questions_queryset = Question.objects.filter(queue__course=course, time_asked__gte=date)
users_union = (
User.objects.filter(
Q(id__in=questions_queryset.values_list("asked_by", flat=True)) |
Q(id__in=questions_queryset.values_list("responded_to_by", flat=True))
)
)
def add_arguments(self, parser): | ||
parser.add_argument("--hist", action="store_true", help="Calculate all historic statistics") | ||
|
||
def calculate_statistics(self, profiles, courses, earliest_date): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The earliest_date
parameter here is used to compute a date
field, which I don't think is quite necessary. I think we can directly just use the earliest_date
field wherever date
is used.
active_users_today.add(q.asked_by) | ||
active_users_today.add(q.responded_to_by) | ||
|
||
for profile in profiles: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might be slow because we are iterating over all profiles, it is probably best to do this in the DB somehow (like the code snippet from above)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost there! Few nits
backend/Pipfile
Outdated
@@ -45,4 +45,4 @@ typing-extensions = "*" | |||
drf-excel = "*" | |||
|
|||
[requires] | |||
python_version = "3" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not quite sure why the Pipfile
and Pipfile.lock
are changed. In general, if there are no dependency changes, we shouldn't touch these cause they're read directly from our production machines to install. Would suggest following this article to remove these changes from the PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to fix this with the stackoverflow article but it kind of does not look like it did anything per the last commit- I think I could use some help with this at GBM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds good
backend/ohq/statistics.py
Outdated
def user_calculate_questions_asked(user): | ||
num_questions = Question.objects.filter(asked_by=user).count() | ||
|
||
if num_questions and not num_questions == 0: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I think you can just write if num_questions:
. If num_questions
is ever 0, the if statement would fail (0 is considered False
)
) | ||
time = user_time_helped["time_helped"] | ||
|
||
if time and not math.isclose(time.total_seconds(), 0, abs_tol=0.001): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: is there a reason you use time.total_seconds()
here, but a few lines down you use time.seconds
? Is there a difference?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lol nope I don't think there's a difference I was just copying off of the documentation- I'll change it to .seconds for consistency
|
||
|
||
|
||
class Command(BaseCommand): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's talk about this file on Friday!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost there! Few nits. Please also run all the necessary linting for backend as well to format your code. I'm not sure how OHQ does it so please ask @trangiabach for that. One more detail as well that I'll message you about on slack
@@ -1,5 +1,6 @@ | |||
from django.core.management.base import BaseCommand | |||
from django.utils import timezone | |||
import datetime |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: think we can remove this line
user_calculate_students_helped(user) | ||
|
||
|
||
def handle(self, *args, **kwargs): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks a lot cleaner, good work!
Nevermind, false alarm. Just that 1 nit and linting then we are good! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!! Congrats on your first finished PR :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
Added a UserStatistics model & relevant methods to track a user's lifetime statistics across all classes
Currently includes:
and I tested them too :)