Skip to content
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

248 start report #1876

Merged
merged 19 commits into from
Jun 28, 2024
Merged

248 start report #1876

merged 19 commits into from
Jun 28, 2024

Conversation

pbastia
Copy link
Contributor

@pbastia pbastia commented Jun 21, 2024

An API endpoint on POST /api/reporting/reports

Card: bcgov/cas-reporting#248

Models created:

  • Report
  • ReportOperation (for admin operation/operator data for a report)
  • ReportFacility (for individual admin facility data for a report)
  • ReportingYear (To enforce FK validation for each reporting year, and store metadata associated like reporting windows and program deadlines)

Services

  • ReportingService with a method to create a report object and pre-populate operation and facility data
  • ReportingDataAccessService to handle database queries about reports

API routes

  • POST: /api/reporting/reports creates a report with the above mentioned service and return its ID

@pbastia pbastia force-pushed the 248-start-report branch 4 times, most recently from c18031b to 55fe0b4 Compare June 27, 2024 18:14
@pbastia pbastia marked this pull request as ready for review June 27, 2024 18:16
@pbastia pbastia force-pushed the 248-start-report branch from 3daae01 to 1917b44 Compare June 28, 2024 18:15
Copy link
Contributor

@joshgamache joshgamache left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests pass and the reports endpoint shows up in Django Ninja with everything as expected.

Everything looks solid! 🌮

@pbastia pbastia merged commit 260a2d5 into develop Jun 28, 2024
23 checks passed
@pbastia pbastia deleted the 248-start-report branch June 28, 2024 21:43
Copy link
Contributor

@Sepehr-Sobhani Sepehr-Sobhani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤩

Comment on lines +83 to +85
def operation_baker(operator_id: uuid.UUID = None, **properties) -> Operation:
if "type" not in properties:
properties["type"] = cycle(["sfo", "lfo"])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: we could still keep *args, **kwargs and have something like:

    if "type" not in kwargs:
        kwargs["type"] = cycle(["sfo", "lfo"])

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

*args makes little sense as the model baker doesn't take positional arguments

bc_obps/reporting/api/reports.py Show resolved Hide resolved
bc_obps/reporting/api/reports.py Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have a helper class named TestUtils in utils/helpers.py customized for testing endpoints; Could be useful.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This class is going to see heavy refactor when we add authentication and everything else, so we'll make sure to refactor at that time

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: To avoid having to refactor this later, as we did in the registration app, I recommend breaking this into separate functions now. like def _create_report_facilities and def _create_report_operation and ...

bc_obps/service/tests/test_report_service.py Show resolved Hide resolved
bc_obps/service/tests/test_report_service.py Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants