[ENG-6165] monthly institutional-users metrics report #10722
Conversation
aaxelb
changed the title
aaxelb
force-pushed
the
eng-6165
branch
3 times, most recently
from
fd9be78
to
7fdc6ea
Compare
There was a problem hiding this comment.
Some mild observations, but a really solid piece of engineering. I'm ready to merge it. But @mfraezz should probably look at it and get the benchmarking confirmed.
| _CHUNK_SIZE = 500 | ||
|
|
||
|
|
||
| class InstitutionalUsersReporter(MonthlyReporter): |
There was a problem hiding this comment.
nit: docstring here could how this report can be used, maybe warn people changes could effect dashboard page.
| department_name=(_affiliation.sso_department or None), | ||
| month_last_login=YearMonth.from_date(self.user.date_last_login), | ||
| account_creation_date=YearMonth.from_date(self.user.created), | ||
| orcid_id=self.user.get_verified_external_id('ORCID', verified_only=True), |
There was a problem hiding this comment.
nit: No unverified ORCID's ? I think that's a correct choice. Good to remember in case individual users see they aren't counted and complain, more of a Product concern, but good to agree on it with Product.
|
|
||
| def _public_project_queryset(self): | ||
| return self._node_queryset().filter( | ||
| type='osf.node', # `type` field from TypedModel |
There was a problem hiding this comment.
nit: Good comments for each function, but it would be nice to have a docstring on the class listing these out.
|
|
||
| def _published_preprint_queryset(self): | ||
| if not hasattr(osfdb.Preprint, 'affiliated_institutions'): | ||
| return osfdb.Preprint.objects.none() # HACK: preprints affiliation project still in-progress |
There was a problem hiding this comment.
nit: if I was going to be a real stickler I would say this should match the creator affiliation numbers to match current SHARE, but that's really minor and probably won't confuse anyone. NBD
| cls._user_setup_with_nothing = _InstiUserSetup(0, 0, 0, 0, 0, cls._institution, cls._now) | ||
| cls._user_setup_with_ones = _InstiUserSetup(1, 1, 1, 1, 1, cls._institution, cls._now) | ||
| cls._user_setup_with_stuff = _InstiUserSetup( | ||
| 2, 3, 5, 3, 2, cls._institution, cls._now, |
There was a problem hiding this comment.
nit: I feel like it's more readable with keyword argument for these fiddly test numbers
| self.assertEqual(report.private_project_count, setup.private_project_count) | ||
| self.assertEqual(report.public_registration_count, setup.public_registration_count) | ||
| self.assertEqual(report.embargoed_registration_count, setup.embargoed_registration_count) | ||
| # NOTE: currently untested due to the annoyance involved: |
There was a problem hiding this comment.
nit: I don't fully understand this comment, because there's other tests that test for these. Be clear it's the annoyance is just in the test set-up.
| # helper class for test-case setup | ||
| @dataclasses.dataclass | ||
| class _InstiUserSetup: | ||
| '''oof, so many things to set up, gross''' |
There was a problem hiding this comment.
Nit: I'd reiterate any business logic here, like I'd say preprints should be based on can_view, or any opinionated stuff, but NBD.
aaxelb
merged commit 4552e4f
into
CenterForOpenScience:feature/insti-dash-improv
Purpose
compute counts on a monthly basis for each user with an institutional affiliation
Changes
osf.metrics.reports.InstitutionalUserReportosf.metrics.reporters.InstitutionalUsersReporterQA 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