Fix inddex reports #35605
Fix inddex reports #35605
Conversation
orangejenny
added
the
product/custom
| @@ -15,12 +15,22 @@ class MultiTabularReport(DatespanMixin, CustomProjectReport, GenericTabularRepor | |||
| exportable = True | |||
| exportable_all = True | |||
| export_only = False | |||
| fix_left_col = True | |||
There was a problem hiding this comment.
This and fixed_cols_spec below are to support the fixColumns, fixColsNumLeft, and fixColsWidth values that were previously specified directly in the inddex js.
| @@ -34,10 +44,11 @@ def _to_context_dict(data_provider): | |||
| 'rows': list(data_provider.rows), | |||
| } | |||
|
|
|||
| context = { | |||
| context = super().report_context | |||
There was a problem hiding this comment.
This pulls in the report_table_js_options specified here. This is where the assumption is made that all of the tables in a single inddex report will have the same js options - I'm not pulling them separately for each data_provider.
| @@ -68,6 +68,9 @@ hqDefine("reports/js/bootstrap5/tabular", [ | |||
|
|
|||
| // Handle async reports | |||
| $(document).on('ajaxSuccess', function (e, xhr, ajaxOptions, data) { | |||
| if (!data || !data.slug) { | |||
There was a problem hiding this comment.
Because inddex/main is now importing this file, it gets this event handler, which it doesn't need, and which errors sometimes (presumably because there are ajax requests happening that aren't the same as what this handler expects). But checking for data.slug is pretty innocuous and fixes the issue.
There was a problem hiding this comment.
I think it would be cleaner to put the event handlers in an init function, and make code that imports this module explicitly call it, but I wanted to limit changes to core reports as much as possible.
There was a problem hiding this comment.
What do you think of putting this comment in the code? This seems far away from inddex/main, and it might be nice to know why this check was added.
| @@ -68,6 +68,9 @@ hqDefine("reports/js/bootstrap5/tabular", [ | |||
|
|
|||
| // Handle async reports | |||
| $(document).on('ajaxSuccess', function (e, xhr, ajaxOptions, data) { | |||
| if (!data || !data.slug) { | |||
There was a problem hiding this comment.
What do you think of putting this comment in the code? This seems far away from inddex/main, and it might be nice to know why this check was added.
Product Description
Followup for #35586, which caused these reports to 500.
Technical Summary
The 500 resulted from the data discussed here not being JSON serializable.
The bulk of the inddex js is the same logic that's in tabular.js, with the major difference being that some of the inddex reports have multiple datatables. The general change here is to update the inddex code to rely more on
tabular.js.This PR assumes that all of the inddex tables in a single report have the same datatables options, which from looking at the reports in python I believe is true. The different reports have different slugs & titles, but I didn't see any different display options.
Feature Flag
inddex reports
Safety Assurance
I've tested on staging that the reports now load, and it appears to behave the same as a browser window of the reports on prod that I've had open since before the deploy of #35586.
While working on this I realized that I could have tested the previous PR on staging. I had initially hit errors there, eventually rebuilt the underlying table, and was still running into errors. But I think I just didn't wait long enough after rebuilding.
Safety story
There are minor changes to
tabular.js, but most of the work is in custom code.As in #35586, the impact of problems here is limited. The only live projects using them are on a third party env, so the changes won't be "live" for real users until that env deploys.
Automated test coverage
Not at the UI level.
QA Plan
Not requesting QA
Rollback instructions
Labels & Review