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

Feat/345 load activity data from database #2446

Merged
merged 12 commits into from
Nov 7, 2024

Conversation

pbastia
Copy link
Contributor

@pbastia pbastia commented Nov 5, 2024

  • report-activity GET endpoint serializes the ReportActivity, ReportUnit, ReportFuel, ReportEmission and ReportMethodology in a format that the activity form can display and understand
  • report-activity POST endpoint makes use of the serialization endpoint to return saved data with ids and computed fields
  • Updates the ActivityForm to update the form data with the saved IDs and computed data

@shon-button shon-button force-pushed the feat/345-load-activity-data-from-database branch from 33470c9 to 6b3b0b9 Compare November 6, 2024 13:02
@pbastia pbastia force-pushed the feat/345-load-activity-data-from-database branch 3 times, most recently from ddf1497 to ea8e9f6 Compare November 6, 2024 22:12
@pbastia pbastia marked this pull request as ready for review November 6, 2024 22:12
@pbastia pbastia force-pushed the feat/345-load-activity-data-from-database branch from ea8e9f6 to c508d59 Compare November 6, 2024 22:14
import { withTheme } from "@rjsf/core";
import formTheme from "@bciers/components/form/theme/defaultTheme";

const Form = withTheme(formTheme);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

FormBase was introducing some unintended behaviours by also holding a copy of the formData in the state, so this uses the RJSF component directly

@pbastia pbastia force-pushed the feat/345-load-activity-data-from-database branch from 622aa16 to 773ed47 Compare November 6, 2024 23:41
Copy link
Contributor

@dleard dleard left a comment

Choose a reason for hiding this comment

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

Looks great! Love how those serializers work. Just one note on a potential performance optimization.
I've made a bug card for the issue this PR unearthed around the persistence of formState that should be handled shortly after this goes in

def load(cls, report_version_id: int, facility_id: UUID, activity_id: int) -> dict:

try:
r = ReportActivity.objects.get(
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this query benefit from prefetch_related?

@pbastia pbastia force-pushed the feat/345-load-activity-data-from-database branch from 2351deb to 1676986 Compare November 7, 2024 17:18
@pbastia pbastia merged commit 26686ab into develop Nov 7, 2024
40 checks passed
@pbastia pbastia deleted the feat/345-load-activity-data-from-database branch November 7, 2024 21:10
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.

2 participants