Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix standard_hq_report #35753

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Fix standard_hq_report #35753

wants to merge 4 commits into from

Conversation

orangejenny
Copy link
Contributor

@orangejenny orangejenny commented Feb 10, 2025

Product Description

https://dimagi.atlassian.net/browse/SAAS-16512

Fixes a bug where UCRs could not be saved as saved reports.

Technical Summary

Introduced in #35599, specifically the removal of the block here that handled UCRs by calling out to the UCR version of getStandardReport.

getStandardReport was split into a UCR version and a standard reports version many years ago, because UCRs were migrated to requirejs and standard reports were not. Now that all reports are on webpack, this PR merges the two sets of logic back into one. The UCR version included some UCR-specific analytics code that's staying in the UCR module.

This means updating UCRs to use one big js_options object for report options, the way standard reports do, instead of passing the report options as individual pieces of data. Because js_options is built in python, this highlighted that several of these options have never done anything in UCRs. Additional context on this...

Standard reports all derive from GenericReportView, which sets up a report object in the context here.

In UCRs, the report in the context is the view itself (see here), which has some inconsistencies with GenericReportView.

Expressions like report.filter_set, which don't exist in the UCR view class, were previously failing silently when they got called during template rendering and are now removed. This also made it apparent that although UCRs have emailable set to True, the initial page data incorrectly referenced report.is_emailable and therefore the functionality wasn't working. I started to implement emailability for UCRs, since clearly that was the original intention, but I backed out after realizing that it would be more involved than expected.

This PR does, however, turn on the export all functionality, which was similarly previously broken but now should work because isExportAll is now based on the real exportable_all view attribute, instead of is_export_all which exists in the context of standard reports (see here) but doesn't exist in UCRs.

This PR has made me much less fond of renames like this where the name changes but the meaning does not. Not that I was fond of them in the first place.

Feature Flag

UCRs

Safety Assurance

Safety story

I'm testing on staging. I'm leaning towards requesting QA to make sure I didn't break any other basic reports interaction in either standard reports or UCRs.

I've tested the inddex reports on staging.

Automated test coverage

No

QA Plan

https://dimagi.atlassian.net/browse/QA-7456

Rollback instructions

  • This PR can be reverted after deploy with no further considerations

Labels & Review

  • Risk label is set correctly
  • The set of people pinged as reviewers is appropriate for the level of risk of the change

This combines the UCR version of getStandardHQReport, which the breaking change caused to no longer get called,
into the logic in standard_hq_report.js. It sets up a js_options dictionary to use in UCR, which was previously
using initial page data.

It also eliminates the old block for handling custom reports, which should be irrelevant now that all custom
reports use external js files and hqDefine, instead of ineline js and global variables.
This isn't necessary for UCRs. It's only used in getReportBaseUrl, which is used in getReportRenderUrl
(which UCR overrides) and sendEmail (which doesn't currently matter because UCR emailing is broken for other
reasons).
@orangejenny orangejenny added Open for review: do not merge A work in progress product/feature-flag Change will only affect users who have a specific feature flag enabled labels Feb 10, 2025
@orangejenny orangejenny added awaiting QA QA in progress. Do not merge and removed Open for review: do not merge A work in progress labels Feb 11, 2025
@jingcheng16
Copy link
Contributor

@orangejenny Hi Jenny, do you mind showing me the code that prove this? I trust you, I'm just curious. I searched StandardReportView (my guess of the view) and didn't find anything 😂

Standard reports all derive from GenericReportView.

Also, I saw you're adding another js_options to ConfigurableReportView. This make sense to me. So both standard report and ucr will have different js_options. What confused me is, when I tried to fix it, I simply add initialPageData.get('js_options') in ucr's report options, without adding js_options in ConfigurableReportView. It works. But after seeing your fix, I'm surprised that m fix works locally, though the yarn build failed...

@jingcheng16
Copy link
Contributor

'require' is not defined

Want to ask about this lint error too. It is unrelated to your change. Just curious what is it. I noticed we have migrated hqRequire to be something else (I forgot). I wonder if this require is a similar thing that need migration.

@orangejenny
Copy link
Contributor Author

@jingcheng16

The reports class hierarchy is extensive. There isn't a single line that proves that all standard reports inherit from GenericReportView - each standard report is its own class, so you need to look at each of them.

But to take one example, the Worker Activity report, you can trace the class hierarchy: GenericReportView > GenericTabularReport > WorkerMonitoringReportTableBase > WorkerMonitoringCaseReportTableBase > WorkerActivityReport.

For others the hierarchy is shallower, like for the Project Performance report it's just: GenericReportView > ProjectReport > ProjectHealthDashboard

Plus there are pages that aren't exactly reports but still use the same basic UI, which also typically inherit from GenericReport, like the Reassign Cases page (GenericReportView > DataInterface > BulkDataInterface > CaseReassignmentInterface).

If you added initialPageData.get('js_options') without adding js_options to the python view, I'm skeptical things were fully working, as I don't see what other code would have provided those options to inital page data, and I'd think the page would fail pretty hard with an empty options object, but maybe I'm not imagining the same code that you wrote. Without seeing the actual code, I can't really say why it did or didn't work.

About require, I just noticed in this PR that it's flagged by the linter. require is what I've been using to replace hqRequire, but I forget how I landed on it, and looking around, it's not an ES6 construct. So I'll revisit how we should be doing dynamic imports, but not in this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting QA QA in progress. Do not merge product/feature-flag Change will only affect users who have a specific feature flag enabled
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants