search

openownership / cove-bods

Check that your data complies with the Beneficial Ownership Data Standard (BODS) using our open source data review tool
https://datareview.openownership.org/
Other
4 stars 2 forks source link

tests/fixtures/0.2/bad_statement_id_type.json crashes cove but does not get an error message #100

Closed odscjames closed 1 year ago

odscjames commented 1 year ago

https://github.com/openownership/lib-cove-bods/blob/master/tests/fixtures/0.2/bad_statement_id_type.json

odscjames commented 1 year ago

The problem is in ConvertJSONIntoSpreadsheets in cove_bods/process.py. If flattentool crashes, the xlsx is not written to disk. Then, the task running code asks is_processing_needed() and is always told yes, so it always trys to reprocess the data.

odscjames commented 1 year ago

Fast solution:

Remove the try/except from ConvertJSONIntoSpreadsheets.process

Then when this test file is uploaded the user will see a generic error message and the whole data check will stop. User will not see any results at all. Because of that, this is not ideal and we need a .....

Better solution:

We want to store the fact an error occurred, show a nice message to the user, show the user the rest of the check data and not falsely trigger is_processing_needed to break the processing loop.

In ConvertJSONIntoSpreadsheets.__init__ create another variable, self.error_filename. it's a path that ends "ConvertJSONIntoSpreadsheets.error.json" or something.

In the try except, if an error is raised, write a small bit of JSON to that filename with info on the error.

In ConvertJSONIntoSpreadsheets.is_processing_needed check whether the XLSX OR the error file exists.

In ConvertJSONIntoSpreadsheets.get_context look for the error JSON and if it exists, put some info in the context.

In https://github.com/openownership/cove-bods/blob/master/cove_bods/templates/cove_bods/explore.html put a nice human error message "We could not create you a spreadsheet cos ..."

(Ideally, use the Django file storage API for all the above.)

radix0000 commented 1 year ago

Fixed by merged PR. Closing.