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

[ENG-6366] Add Monthly Activity Stat to Dashboard Reporter #10780

Conversation

Johnetordoff
Copy link
Contributor

@Johnetordoff Johnetordoff commented Oct 16, 2024

Purpose

Add Monthly Activity state for users report in institutional dashboard.

Changes

  • add query to reporter for most recent user log
  • add case to test
  • add to filtering on view

QA Notes

Please make verification statements inspired by your code and what your code touches.

  • Verify
  • Verify

What are the areas of risk?

Any concerns/considerations/questions that development raised?

Documentation

Side Effects

Ticket

https://openscience.atlassian.net/browse/ENG-6366

@Johnetordoff Johnetordoff changed the title add monthly active stat to users table [ENG-6366][WIP] Add Monthly Activity Stat to Dashboard Reporter Oct 16, 2024
@Johnetordoff Johnetordoff force-pushed the feature/insti-dash-improv-active-month-user branch 3 times, most recently from a83fea3 to 1c24a5c Compare October 17, 2024 15:03
@Johnetordoff Johnetordoff force-pushed the feature/insti-dash-improv-active-month-user branch from 1c24a5c to 1cdb1d7 Compare October 17, 2024 16:49
@Johnetordoff Johnetordoff marked this pull request as ready for review October 17, 2024 18:26
@Johnetordoff Johnetordoff changed the title [ENG-6366][WIP] Add Monthly Activity Stat to Dashboard Reporter [ENG-6366] Add Monthly Activity Stat to Dashboard Reporter Oct 17, 2024
Copy link
Contributor

@aaxelb aaxelb left a 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

Comment on lines 151 to 153
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]
Copy link
Contributor

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(),
])

Comment on lines 177 to 179
self.user.month_last_active = max(dates)
else:
self.user.month_last_active = None
Copy link
Contributor

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

@Johnetordoff Johnetordoff force-pushed the feature/insti-dash-improv-active-month-user branch from 28fce27 to e3669b7 Compare October 18, 2024 15:20
preprint_logs.values_list('created', flat=True).first(),
])

self.month_last_active = max(dates, default=None)
Copy link
Contributor

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

@Johnetordoff Johnetordoff merged commit 6437270 into CenterForOpenScience:feature/insti-dash-improv Oct 18, 2024
6 checks passed
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.

2 participants