-
Notifications
You must be signed in to change notification settings - Fork 330
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
[ENG-6366] Add Monthly Activity Stat to Dashboard Reporter #10780
[ENG-6366] Add Monthly Activity Stat to Dashboard Reporter #10780
Conversation
a83fea3
to
1c24a5c
Compare
1c24a5c
to
1cdb1d7
Compare
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.
a couple suggestions to avoid unnecessary queries and reader confusion, but overall seems good
latest_node_log_date = node_logs.first().created if node_logs.exists() else None | ||
latest_preprint_log_date = preprint_logs.first().created if preprint_logs.exists() else None | ||
dates = [date for date in [latest_node_log_date, latest_preprint_log_date] if date is not None] |
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.
could avoid the two exists
queries and simplify a bit by using values_list
:
dates = filter(bool, [
node_logs.values_list('created', flat=True).first(),
preprint_logs.values_list('created', flat=True).first(),
])
self.user.month_last_active = max(dates) | ||
else: | ||
self.user.month_last_active = None |
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 is pretty convoluted, setting a non-existent attr on the user instance for use elsewhere
could make it more explicit by adding a month_last_active
field to _InstiUserSetup
(with init=False
because it's computed in __post_init__
, like user
)
month_last_active: YearMonth | None = dataclasses.field(init=False)
then set self.month_last_active
here and access setup.month_last_active
in the test assertion
28fce27
to
e3669b7
Compare
preprint_logs.values_list('created', flat=True).first(), | ||
]) | ||
|
||
self.month_last_active = max(dates, default=None) |
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.
oh, small point of "names shouldn't lie" (not to mention that type annotations shouldn't lie):
if leaving names (and annotations) as they are, should cast this to YearMonth
here instead of in the test (which would save you the if
/else
)
or if leaving logic as it is, should rename the field something like date_last_active
and change its type annotation to datetime.date | None
either way, as long as words and logic agree
6437270
into
CenterForOpenScience:feature/insti-dash-improv
Purpose
Add Monthly Activity state for users report in institutional dashboard.
Changes
QA Notes
Please make verification statements inspired by your code and what your code touches.
What are the areas of risk?
Any concerns/considerations/questions that development raised?
Documentation
Side Effects
Ticket
https://openscience.atlassian.net/browse/ENG-6366