-
Notifications
You must be signed in to change notification settings - Fork 1
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
Chore/compliance summary tests #2597
Conversation
dleard
commented
Dec 16, 2024
•
edited
Loading
edited
- Unit tests for the static functions in the compliance summary service
- Multiple test cases for the class function in the compliance summary service
- Refactors the service to not depend on the ninja schemas for data shaping
- Fixes a bug in the compliance service where the Reduction Factor & Tightening Rate were being applied in the wrong place
4436252
to
1cab204
Compare
eb26a16
to
58d7d32
Compare
a713a6b
to
d2f628e
Compare
36fca1d
to
8f2985b
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.
Very nice! I left a few nitpicks but this is good to go
reporting_only_emissions: Decimal | int, | ||
emissions_attributable_for_compliance: Decimal | int, | ||
emissions_limit: Decimal | int, | ||
excess_emissions: Decimal | int, |
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.
is the int
necessary? Wouldn't that be covered under the Decimal
already?
@classmethod | ||
def pare_data_single_product_flaring(cls): | ||
# Pare down build data to data needed for this test | ||
ReportProductEmissionAllocation.objects.exclude(emission_category_id=1).delete() |
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 might have side-effects on other tests, or on other things the user of this infrastructure may do.
I'd suggest cls.allocation_1.delete()
instead
...same for the ones down below
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 definitely ended up being cleaner & removed potential side effects, but what I did was I moved the .build() call inside this function instead of inside the test, delete from that instantiation & returned the new object
reporting_window_end='2026-12-31 16:00:00-08', | ||
report_due_date='2025-05-31 16:59:59.999-07', | ||
) | ||
report = Report.objects.get(reporting_year_id=2024) |
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.
same, probably cls.report_1.reporting_year = ...
would work better
operation.naics_code_id = 22 | ||
operation.save() | ||
report.reporting_year_id = 2024 | ||
report.save() |
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.
For shorter syntax we can use the update
queryset method:
Report.objects.filter(pk=...).update(reporting_year_id=2024)
6eb3e1b
to
41486f9
Compare
41486f9
to
fdab5dd
Compare