[TAN-7380] Formsync better progress indicator#13606
Conversation
AchrafGoVocal
changed the title
|
… tracker-based progress polling
…com:CitizenLabDotCo/citizenlab into TAN-7380-formsync-better-progress-indicator
| @@ -0,0 +1,45 @@ | |||
| import React from 'react'; | |||
There was a problem hiding this comment.
In our components/UI folder we also have a ProgressBar component - not sure how that looks/if it would work for FormSync as well instead? :
/front/app/components/UI/ProgressBar/index.tsx
If it doesn't, I'd just check with Rob first before adding anything new in our component library :)
adessy
left a comment
adessy
left a comment
There was a problem hiding this comment.
Sorry for the wait! Good job, it's looking good 💯 I'm just challenging a few decisions, but nothing blocking on my end. It might be worth adding a few more tests to the back end though.
I didn't check the front-end part since Amanda already reviewed it.
|
|
||
| file = bulk_create_params[:file] | ||
| job_ids = file_parser_service.parse_file_async file | ||
| files = file_parser_service.create_files(file) |
There was a problem hiding this comment.
We should make sure it's okay to do this synchronously for reasonably large files.
There was a problem hiding this comment.
I guess we're doing this sync to make error reporting easier, but would it make sense to split the file in IdeaImportJob at the same time as batching?
There was a problem hiding this comment.
We should make sure it's okay to do this synchronously for reasonably large files.
I double checked and the file splitting has always been done synchronously before launching the jobs. Since we haven't had any issue with it up until now, it should be fine.
I guess we're doing this sync to make error reporting easier, but would it make sense to split the file in IdeaImportJob at the same time as batching?
Hmm I'm not sure. Since the file splitting doesn't take that long, it feels better to have it be done synchronously and get immediate feedback on what went wrong (for example the user might enter a number_of_pages_per_form that cannot divide the actual number of pages of the pdf evenly)
Another solution would be to split the validation from the file splitting itself but that would require more changes in the code and I am not sure it's worth it for now since it doesn't take long to do the splitting anyway.
| SideFxBulkImportService.new.after_failure(import_user, phase, 'idea', 'pdf', e.to_s) | ||
| remaining = idea_import_files.count - files_processed | ||
| track_progress_and_complete!(remaining, remaining) | ||
| raise e |
There was a problem hiding this comment.
Do we want the job to fail if a BulkImportIdeas::Error is raised? I mean, you could argue that aborting on an ill-formed file is just normal, expected behavior. It really depends on when and why those errors are used though.
| SideFxBulkImportService.new.after_success(import_user, phase, 'idea', 'pdf', ideas, users) | ||
| SideFxBulkImportService.new.after_success(import_user, phase, 'idea', 'pdf', all_ideas, import_service.imported_users) | ||
| complete_if_done! | ||
| rescue StandardError => e |
There was a problem hiding this comment.
Do we want the rescue at the batch-level or file-level?
There was a problem hiding this comment.
I'm wondering if we really need a parent job here, or if IdeaPdfImportJob and IdeaXlsxImportJob could just handle batching themselves (using a recursive job, for instance). That would eliminate the need for the FORMAT_CONFIG hash, but it might not be any better than what you have. Anyway, just food for thought, not blocking at all.
AchrafGoVocal
merged commit 33215cf
into
TAN-7201-add-new-output-schema-and-prompt
4 participants
Description
Added a better progress indicator while importing so that users can see the progression of their import and already see ideas as they get processed.
Note: There are some improvements to error handling that I kept in a seperate PR TAN-7519 for clarity.
with_tracking+enqueue_child_jobpattern.IdeaImportJobthat orchestrates the child jobs and sets the tracker total.ProgressBarcomponent to the component library.ImportStatuscomponent showing a live progress bar ("Importing 3/6") during import.api/background_jobs/that was no longer used after switching to tracker-based progress polling.Changelog
Changed
For translators