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

Data Submission Reports Feature #28

Merged
merged 11 commits into from
Feb 9, 2024
Merged

Data Submission Reports Feature #28

merged 11 commits into from
Feb 9, 2024

Conversation

akuny
Copy link
Collaborator

@akuny akuny commented Feb 7, 2024

Changes

  • Added a migration to update the data_submissions table that creates a new JSON field for storing report data.
  • Added a report property to the DataSubmission entity.
  • Added DTOs and related utilities for managing report data.
  • Updated the task queue to execute additional report logic and persist the results to disk.

Fixes

  • Addressed issues with the handling of SQLAlchemy sessions within ApplicationContext implementations and repository implementations.

Future Considerations

  • Task Queue Improvement: The current task queue execution blocks the invoking use case from completing. This must be updated so tasks run truly asynchronously in the background in the near future.
  • Dev Deployment: This needs some care and attention--presumably after the report frontend is built. In particular, the Celery setup that's currently in place won't work in dev.

@akuny akuny requested a review from danielnaab February 7, 2024 21:52
Copy link
Collaborator

@danielnaab danielnaab 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 - sorry for the delay

nad_ch/domain/entities.py Outdated Show resolved Hide resolved
):
return DataSubmissionReport(
overview=DataSubmissionReportOverview(feature_count=1)
)

app_context._task_queue = CustomMockTestTaskQueue()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Future thought - it might be helpful to have a way to provide a mock implementation to create_app_context (or maybe create_test_app_context - if this was done often.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point, we will almost certainly need to do this sort of mocking a number of times.

@akuny akuny merged commit bba1eb3 into main Feb 9, 2024
1 check passed
@akuny akuny deleted the reports branch February 9, 2024 18:39
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