-
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: facility review fixes #2223
base: develop
Are you sure you want to change the base?
Conversation
1aa8b9d
to
f6237f4
Compare
|
||
|
||
@router.get( | ||
"/report-version/{version_id}/facility-report/{facility_id}", |
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.
👌 👌
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! 🥇 🥇 Just a couple of nitpicks
request: HttpRequest, version_id: int, facility_id: UUID | ||
) -> Tuple[Literal[200], Optional[FacilityReportOut]]: | ||
facility_report = FacilityReportService.get_facility_report_by_version_and_id(version_id, facility_id) | ||
activity_ids = FacilityReportService.get_activity_ids_for_facility(version_id, facility_id) or [] |
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.
The service already returns an empty list if none are found
activity_ids = FacilityReportService.get_activity_ids_for_facility(version_id, facility_id) or [] | |
activity_ids = FacilityReportService.get_activity_ids_for_facility(version_id, facility_id) |
facility_type=facility_report.facility_type, | ||
facility_bcghgid=facility_report.facility_bcghgid, | ||
activities=list(facility_report.activities.values_list('id', flat=True)), | ||
products=list(facility_report.products.values_list('id', flat=True)) or [], |
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.
products=list(facility_report.products.values_list('id', flat=True)) or [], | |
products=list(facility_report.products.values_list('id', flat=True)), |
def facility_report_baker(**props) -> FacilityReport: | ||
if "report_version" not in props and "report_version_id" not in props: | ||
props["report_version"] = report_version_baker() | ||
|
||
if "facility" not in props and "facility_id" not in props: | ||
props["facility"] = facility_baker() | ||
|
||
fr = baker.make(FacilityReport, **props) | ||
|
||
return fr | ||
|
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 should be a recipe now :D
return list(facility_report.activities.values_list('id', flat=True)) | ||
|
||
@classmethod | ||
def get_facility_report_form_data( |
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.
I think this method is not a service method, it's just something to build API response data within the API layer. I'd put it in the api/facility_report.py
file directly
There were some errors in how facility_id was being applied.
It was being treated as a numeric where it is a UUID value, which had some cascading effects.
This PR should fix that up.
Also cleaned up the API a little bit by splitting out
facility-report
API/service stuff into its own directory & added some tests.Easiest way to test this: