[ENG-6124] Create Monthly Reporter for Institution Summary #10756
Conversation
Johnetordoff
force-pushed
the
summary-reporter-for-insti-dashboard
branch
11 times, most recently
from
505b185
to
cb8ac19
Compare
Johnetordoff
changed the title
| self.assertEqual(report.institution_id, self._institution._id) | ||
| self.assertEqual(report.user_count, 2) # _logged_in_user and _active_user | ||
| self.assertEqual(report.public_project_count, 1) | ||
| self.assertEqual(report.private_project_count, 1) | ||
| self.assertEqual(report.public_registration_count, 1) | ||
| self.assertEqual(report.embargoed_registration_count, 1) | ||
| self.assertEqual(report.published_preprint_count, 1) | ||
| self.assertEqual(report.storage_byte_count, 1337) # test value for one file | ||
| self.assertEqual(report.public_file_count, 1) | ||
| self.assertEqual(report.monthly_logged_in_user_count, 1) | ||
| self.assertEqual(report.monthly_active_user_count, 1) |
There was a problem hiding this comment.
would ideally have more than one test case
| def get_monthly_active_user_count(self, institution, yearmonth): | ||
| start_date, end_date = self.get_month_start_end(yearmonth) | ||
| return institution.get_institution_users().filter( | ||
| date_disabled__isnull=True, |
There was a problem hiding this comment.
is "active" supposed to mean only that the account has not been deactivated? not seeing much point to this measure, except to indirectly show how many of this institution's users have been banned this month (by subtracting from the "logged-in" count)
There was a problem hiding this comment.
Yeah I see what you mean, but I think it should just be a running total of non-deleted users, that makes more sense then user logins, together the numbers give a lot of context.
Johnetordoff
force-pushed
the
summary-reporter-for-insti-dashboard
branch
from
cb8ac19
to
c590f07
Compare
Johnetordoff
force-pushed
the
summary-reporter-for-insti-dashboard
branch
from
c590f07
to
79b2d23
Compare
Johnetordoff
force-pushed
the
summary-reporter-for-insti-dashboard
branch
from
a4e84d3
to
e339efa
Compare
|
|
||
| def test_report_generation_multiple_institutions(self): | ||
| institution2 = InstitutionFactory() | ||
| institution3 = InstitutionFactory() |
There was a problem hiding this comment.
nit: why create institution3 and then not assert anything on its report?
There was a problem hiding this comment.
Just making sure it can handle an empty one,
| self.assertEqual(report_institution2.public_project_count, 1) | ||
| self.assertEqual(report_institution2.private_project_count, 0) | ||
| self.assertEqual(report_institution2.user_count, 1) | ||
| self.assertEqual(report_institution2.monthly_active_user_count, 1) | ||
| self.assertEqual(report_institution2.monthly_logged_in_user_count, 0) # No logged-in users |
There was a problem hiding this comment.
having no counts greater than 1 doesn't test counting logic very well -- when i suggested "more than one test case" i meant to imply a small variety of situations that yield different results, not roughly the same situation a second time
(tho don't get me wrong, "multiple institutions" is a good step in that direction)
| institution3 = InstitutionFactory() | ||
|
|
||
| # Set up dates for different months | ||
| now = datetime.datetime(2018, 2, 4, tzinfo=datetime.UTC) |
There was a problem hiding this comment.
nit: this is already at self._now? why this test method so inconsistent from the last one? when setting up a different scenario with different data, might be cleaner in a separate TestCase
…ForOpenScience/osf.io into summary-reporter-for-insti-dashboard * 'feature/insti-dash-improv' of https://github.com/CenterForOpenScience/osf.io: refactor documentation and names add another more complex test case speed up tests update institution summary information with new view for monthly report
Johnetordoff
force-pushed
the
summary-reporter-for-insti-dashboard
branch
from
88e1593
to
f0f6e69
Compare
Johnetordoff
force-pushed
the
summary-reporter-for-insti-dashboard
branch
from
f0f6e69
to
edeb0ac
Compare
Johnetordoff
merged commit 58c60a5
into
CenterForOpenScience:feature/insti-dash-improv
Purpose
Adds monthly reporter for institution summary.
Changes
InstitutionalSummaryMonthlyReporterobject with institutions KPIsInstitutionMonthlySummaryReportto reports fileQA 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-6124