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 tag cloud visualization failing on formatted values #21001

Merged
merged 3 commits into from
Jul 12, 2023

Conversation

bx80
Copy link
Contributor

@bx80 bx80 commented Jul 11, 2023

Description:

When using the tag cloud data visualization on currency values an error is shown.

"message":"Unsupported operand types: string \/ string","file":"\/var\/www\/html\/plugins\/CoreVisualizations\/Visualizations\/Cloud.php","line":197,

This is because the tag cloud is generated after the datatable filters are applied, so when the data includes currency values the visualization fails to compute tag sizes because it cannot parse formatted currency values.

This PR changes the point where the tag cloud data is generated to happen before the metric formatting filters are applied.

Ref PG-2896

Review

@bx80 bx80 added the Bug For errors / faults / flaws / inconsistencies etc. label Jul 11, 2023
@bx80 bx80 added this to the 5.0.0 milestone Jul 11, 2023
@bx80 bx80 self-assigned this Jul 11, 2023
Copy link
Contributor

@AltamashShaikh AltamashShaikh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked this in 4.x-dev and it does solve the isse
Screenshot from 2023-07-11 13-23-14

Screenshot from 2023-07-11 13-23-25

@bx80 bx80 added the Needs Review PRs that need a code review label Jul 11, 2023
@sgiehl
Copy link
Member

sgiehl commented Jul 11, 2023

Had another look at that one. Actually we are showing the value when hovering over a word from the tag cloud. So this change now actually causes the value to be displayed unformatted there. So solving this properly won't be possible unless we implement another hack that fetches the data twice, so we are able to calculate with it and display it formatted.

Overall this is another case why #20701 and/or #20702 would be such important to implement, as currently we need to produce more and more technical debts to fix such issues... (ping @Stan-vw)

@bx80
Copy link
Contributor Author

bx80 commented Jul 12, 2023

Huge upvote for #20701 / #20702 👍 Everytime we encounter this problem and implement a workaround we've creating more work for later.

I've implemented a hack which avoids reloading the datatable, it works like this:

  • beforeLoadDataTable() sets ['format_metrics'] = 0;
  • Datatable is loaded, no metric formatting is done
  • afterAllFiltersAreApplied() gets the unformatted values from the datatable and stores them in an array
  • beforeRender() sets ['format_metrics'] = 1; and calls a new method applyMetricsFormatting() on the Visualization class which applies just the metric formatting filter to the loaded dataset.
  • beforeRender() finally calls generateCloudData() which uses both the stored unformatted values and the now formatted values from the dataset to build the tag cloud data.

This allows the tag cloud to be built properly when using currency or other formatted values and still show the correctly formatted values when hovering.

I know the Visualization::applyMetricsFormatting() method is an ugly approach, but it's a simple and performant workaround for visualization that need to build things with both formatted and unformatted data. I've made a note on the method that it is a workaround and may be removed once we have a proper solution for this issue.

Copy link
Member

@sgiehl sgiehl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The workaround seems to work. At least for custom reports, as they are using mainly processed metrics, which are correctly formatted.
Metrics in other reports aren't formatted. But that hadn't been the case before already. That another problem of formatting some metrics in the post processor, but all other metrics in the view. But that's not something we need to solve here.

@sgiehl sgiehl merged commit 3930355 into 5.x-dev Jul 12, 2023
17 of 20 checks passed
@sgiehl sgiehl deleted the cloud-tag-currency-fix branch July 12, 2023 08:48
@AltamashShaikh
Copy link
Contributor

@sgiehl @bx80 Can we cherrypick this into 4.x-dev so thatw e can solve the issue for CLOUD ?

sgiehl pushed a commit that referenced this pull request Jul 13, 2023
* Change the cloud tag visualization to generate before metric formatting filters

* Use request parameter to avoid formatting metrics instead of changing visualization generation timing

* Reapply metric formatting after storing tag cloud size values
sgiehl added a commit that referenced this pull request Jul 13, 2023
)

* Change the cloud tag visualization to generate before metric formatting filters

* Use request parameter to avoid formatting metrics instead of changing visualization generation timing

* Reapply metric formatting after storing tag cloud size values

Co-authored-by: Ben Burgess <88810029+bx80@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug For errors / faults / flaws / inconsistencies etc. Needs Review PRs that need a code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants