Import file without show screen to confirm #1319
Conversation
…he confirmation form before file importing, that file will be imported directly and the screen to confirm data inside of that won't be shown.
…-to-confirm Import file without screen to confirm
|
Hi Flávio Thanks for submitting this PR. I like the idea that there can be an option to skip the confirmation page. I've made some comments with respect to the change. Incidentally, did you look into using a different BaseStorage implementation? You can get round your issue by storing the data to import in a cache or by using MediaStorage such as s3. |
| dataset, confirm_form, request, *args, original_file_name=import_file.name | ||
| ) | ||
|
|
||
| tmp_storage.remove() |
There was a problem hiding this comment.
It might be better not to write to temp storage in the first place if the IMPORT_EXPORT_SKIP_ADMIN_CONFIRM_FORM is set, rather than remove here. I think that would be cleaner.
There was a problem hiding this comment.
I've done this way because I would like to keep that screen where it showed what's wrong
and I wouldn't change the main import workflow. I understand your concern about keeping the code cleaner but I can't see any solution but to separate that feature of the main import workflow. Is That approach sounds good to you?
There was a problem hiding this comment.
I think it would be ideal to have a means of disabling the write to storage entirely. Your issue has highlighted one reason why this might be required, but there could be others, for example if users were concerned about sensitive data leaks, or if they never needed to confirm the import, and just wanted to skip a step.
So I think this PR would be improved if you could make it so tmp_storage write was skipped. This would be a bit more logic just before the write_to_tmp_storage() call so that the data is kept in memory.
I also think that it would be cleaner not to use a new settings variable for this, it could be achieved simply by setting IMPORT_EXPORT_TMP_STORAGE_CLASS to None. This would be my preferred way of doing it because it would remove the need for an additional setting. Obviously we would need to document this behaviour.
I think this would be a useful addition. What are your thoughts on the above?
There was a problem hiding this comment.
Excellent points, I'll work to improve that PR.
…9#issuecomment-906236059 Reworded env var from PROCESS_WITHOUT_SHOW_CONFIRM_FORM to IMPORT_EXPORT_SKIP_ADMIN_CONFIRM_FORM
…9#issuecomment-906236059 Removed comment about new flag IMPORT_EXPORT_SKIP_ADMIN_CONFIRM_FORM
…9#issuecomment-906236059 Removed trailing space
…9#issuecomment-906240911 Fixed .gitignore to ignore all content inside of .vscode folder
Use another MediaStorage is also a great solution to fix that problem. I've done that and that pull request is possibly improving. |
|
It would be great to get this change into our next release - would you have any availability to complete the changes? |
Sorry about my delay with that PR. I haven't had time to finish that PR in the last few days. I'll have to rework that solution then I don't know when I'll refresh that PR. Maybe it's good to finish this PR, and I can open a new PR in the future. |
|
Linking to issue #251 |
|
Closing. Replaced by #1540 |
Problem
Currently, when we want to import a file, in the first screen django-import-export send that file to save it in the server's temp directory and return a form where there is the file's data to user confirm if they want to import it. After the user's confirmation, the library uses the file saved on the temp directory and import its data.
When an app has two or more containers provisioned, that approach doesn't work because it's impossible to ensure the same container where the file has been created will be the same container that will receive the request to import that file.
Solution
Implemented a new feature that if the environment variable "PROCESS_WITHOUT_SHOW_CONFIRM_FORM" is true, the file will be imported directly without showing the screen to confirm data inside of that. This way, it's possible to ensure the same file sent will be imported into the database.
Acceptance Criteria
If the environment variable "PROCESS_WITHOUT_SHOW_CONFIRM_FORM" is false, the library must work as it works nowadays. If true, the confirmation form mustn't show and the file has to be imported.
Have you written tests? Yes, I have (tests/core/tests/test_admin_integration.py, test_import_without_show_confirm_form)
Have you included screenshots of your changes if applicable? No, It's not applicable.
Did you document your changes? Yes,