Record import export #503
Record import export #503
Conversation
georgehelman
force-pushed
the
record_import_export
branch
from
56cc4a4
to
5e52f46
Compare
georgehelman
force-pushed
the
record_import_export
branch
from
5e52f46
to
c42211b
Compare
src/features/ExistingPetitions.jsx
Outdated
| disabled={!!batch?.generate_summary_errors?.batch} | ||
| title={batch?.generate_summary_errors?.batch?.join(' ') ?? ''} |
There was a problem hiding this comment.
I think we should only disable if it's impossible to generate the record summary spreadsheet. As far as I'm aware, that's not the case so I think you can remove this.
There was a problem hiding this comment.
Okay, I got rid of it.
| 'application/vnd.openxmlformats-officedocument.spreadsheetml.sheet', | ||
| ]; | ||
| const MAX_FILES = 1; | ||
| const MAX_FILE_SIZE = 30000; |
There was a problem hiding this comment.
Do we really want this max file size? I can't tell if it's in bytes, KB or MB, so it may be pretty small.
There was a problem hiding this comment.
I got rid of it.
| ], | ||
| ) | ||
| def import_spreadsheet(self, request): | ||
| files = request.data.getlist("files") |
There was a problem hiding this comment.
Consider try/except block to gracefully handle errors. Also do you need to make the transaction atomic?
There was a problem hiding this comment.
I added an atomic block to the import_data method of the resource
There was a problem hiding this comment.
So does it handle it gracefully if user imports spreadsheet with unexpected format? Or is try/except needed?
dear_petition/petition/resources.py
Outdated
| self.change_worksheet(sheet_name=title) | ||
|
|
||
| def append(self, values, fields, is_header=False, num_indent=0, bold=False, color=None,): | ||
| assert len(values) == len(fields), "The values and fields passed to append should match." |
There was a problem hiding this comment.
Better to raise error. I believe asserts can be disabled through configuration.
dear_petition/petition/resources.py
Outdated
| def export(self, batch_object=None, *args, **kwargs): | ||
| # Initialize a new dataset with headers | ||
| assert isinstance(batch_object,models.Batch), "Queryset must be a Batch object" |
There was a problem hiding this comment.
Better to raise error. I believe asserts can be disabled through configuration.
| @@ -186,7 +186,6 @@ function GenerationPage() { | |||
| </Button> | |||
| <Button | |||
| onClick={() => generateRecordsSpreadsheet()} | |||
| disabled={!!data?.generate_summary_errors?.batch} | |||
| title={data?.generate_summary_errors?.batch?.join(' ') ?? ''} | |||
There was a problem hiding this comment.
This should be removed as well. Otherwise, it will show errors for the account summary generation while hovering over the button
No description provided.