Conversation
MartinVanIJcken
force-pushed
the
async-exports-models-and-jobs
branch
from
00ced81 to
d1d2992
Compare
MartinVanIJcken
force-pushed
the
async-exports-models-and-jobs
branch
3 times, most recently
from
d413eeb to
76dcf80
Compare
This model will be used to contain the shared information of any export. The to_create action on the model is overrridden in the factory to allow for specs to create a model of this class before any of the exportable types are introduced. Jira-Issue: MAV-1521
In order to prevent overloading our database with historic exports, we want exports to be destroyed after a while. Jira-Issue: MAV-1521
We want these to be deleted after some time to prevent the database from being filled with old exports. This is similar to Imports which are also expired after some time. Jira-Issue: MAV-1521
The current_user is only used as a fallback value for the team. In an async context, there is no current user so it is more appropriate to use team directly. Jira-Issue: MAV-1521
Jira-Issue: MAV-1521
Jira-Issue: MAV-1521
Jira-Issue: MAV-1521
Jira-Issue: MAV-1521
MartinVanIJcken
force-pushed
the
async-exports-models-and-jobs
branch
from
76dcf80 to
28b4897
Compare
thomasleese
left a comment
thomasleese
left a comment
There was a problem hiding this comment.
Generally looks good, just got a few of small comments.
|
|
||
| def generate_file | ||
| form = | ||
| PatientSearchForm.new( |
There was a problem hiding this comment.
I'm not sure about using a form directly in this model like this, particularly as we have to pass in a request_path and request_session which aren't relevant in this context.
I think it might be worth extracting the logic related to filtering the records in to a separate service class which can then be used here directly, and used by the PatientSearchForm.
| @@ -0,0 +1,14 @@ | |||
| # frozen_string_literal: true | |||
|
|
|||
| class CleanupExportsJob < ApplicationJob | |||
There was a problem hiding this comment.
I'm happy for this to be its own job, but it has parallels with the RemoveImportCSVJob job and I wonder if there's the potential for some consistency here.
I'm thinking, for example, we could maybe put both in a shared module, and set them up to run on the same queue.
Cleanup::RemoveImportCSVDataJob
Cleanup::RemoveExportFileDataJobI'm not sold on the name of the Cleanup module, but something like that.
| file_type { exportable&.file_type&.to_s || "csv" } | ||
| filename { "export.csv" } | ||
|
|
||
| to_create { |instance| instance.save!(validate: false) } |
There was a problem hiding this comment.
Why we do have to call this with validate: false?
There was a problem hiding this comment.
The tests mock the exportable, making it not match the type validation that would otherwise be ran. I took that approach to decouple the tests for Export from any particular type of export. I'm not sold on it though so I'd happily switch to not mocking the exportable and setting up a valid exportable for the Export tests.
There was a problem hiding this comment.
I think ideally the factory would create valid instances, so maybe we can have it randomly pick an exportable if that's not important for the particular test, or be explicit if it is.
| # | ||
| # fk_rails_... (location_id => locations.id) | ||
| # | ||
| class LocationExport < ApplicationRecord |
There was a problem hiding this comment.
I wonder if it's making this clearer that the export contains patients rather than locations.
| class LocationExport < ApplicationRecord | |
| class LocationPatientsExport < ApplicationRecord |
There was a problem hiding this comment.
And similar with SessionExport actually, it might be clearer as SessionPatientsExport.
There was a problem hiding this comment.
Good idea, I went though multiple names for these exports and none of them felt right
2 participants
Introduce Jobs and Models required for asynchronous exports
This is an under-the-hood change
This PR only introduces the jobs and the models. The purpose is to split the asynchronous exports work into two PRs one of which (this one) has no user facing consequences. This should allow the current PR to only be regression tested and reduce the amount of code that needs to go into the release at once.
Jira Issue - MAV-1521