[ENG-6165] bugfixing followup #10746
Conversation
aaxelb
force-pushed
the
eng-6165-followup
branch
from
71a84f9
to
0f53a27
Compare
aaxelb
force-pushed
the
eng-6165-followup
branch
from
0f53a27
to
8e5a21f
Compare
osf/metrics/reporters/_base.py
Outdated
| @@ -20,7 +20,7 @@ def report( | |||
| def run_and_record_for_month(self, report_yearmonth: YearMonth) -> None: | |||
| reports = self.report(report_yearmonth) | |||
| for report in reports: | |||
| assert report.report_yearmonth == str(report_yearmonth) | |||
| assert str(report.report_yearmonth) == str(report_yearmonth) | |||
There was a problem hiding this comment.
Minor: The other MonthlyReporter casts report_yearmonth to str on report initialization -- should this one as well, instead of changing this assertion? It may not matter.
There was a problem hiding this comment.
hmm agree this is odd -- now looking at it again, seems better to set report.report_yearmonth here, instead of making the reporter do it (have updated)
| @@ -37,7 +37,7 @@ class Meta: | |||
|
|
|||
| class YearmonthField(metrics.Date): | |||
| def __init__(self, *args, **kwargs): | |||
| super().__init__(*args, **kwargs, format='strict_year_month', required=True) | |||
| super().__init__(*args, **kwargs, format='strict_year_month') | |||
There was a problem hiding this comment.
Minor: This makes a lot of YearMonth fields no longer implicitly (sort of) required. Would it be preferable to default to required=True but allow required=False to be passed in? e.g. with required = kwargs.pop('required', True)
There was a problem hiding this comment.
the default for elasticsearch_dsl.Field is required=False, and consistency with that seems like a good idea -- there was only one yearmonth field prior to this project (on the MonthlyReport base class), which now has explicit required=True, and the ones added for this project seem fine to make optional
|
(fyi, changed some field mappings after api work; cherry-picked to here trying to avoid changing mappings again after this pr if we can avoid it) |
aaxelb
merged commit 4a2e997
into
CenterForOpenScience:feature/insti-dash-improv
Purpose
fix bugs found running monthly institutional-user report on actual data
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