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 error trying to round unsupported operands in custom reports #16543

Merged
merged 4 commits into from Oct 8, 2020

Conversation

tsteur
Copy link
Member

@tsteur tsteur commented Oct 7, 2020

Fixes eg such an error
image

Since merging #16520 the custom reports UI tests fail. I have been able to reproduce this and it seems to be eg caused by this PR https://github.com/matomo-org/matomo/pull/15304/files

To be specific it is happening here where eg reportTotal=494,836.4. Basically, the total values are formatted and then it throws an error.

image

totals are formatted:
image

Not sure how to best fix it. Was first thinking maybe we need to store also totalsUnformatted as a metadata in ReportTotalsCalculator class and then use it there. This be possible to do but not sure if it has any downside when calculating the percentages. On the other side I think it should not calculate percentage values where one part is formatted and the other part is unformatted so this might be the solution.

We also can't simply do if (is_numeric($value) && is_numeric($reportTotal)) { because this will prevent calculating the percentage for any metric that has values > 1000 (as it would be 1,000 and therefore not a number). Maybe the problem is that it partially needs to be smarter and only calculate percentages for certain metrics that are actually needing percentages but this alone won't fix it either because it also needs to use additionally the unformatted total metrics. Otherwise the same issue persists.

It seems to me using the unformatted total metric values might be the best solution so it actually always compares unformatted values.

I was reproducing it with an "avg" metric (what I did was actually creating a report that adds all metrics like below but note some metrics might not be available... below metrics I added to a "table" custom reports in the custom_reports table directly in the metrics column):

{code:java}
["avg_sum_actions_per_visits","avg_sum_corehome_visitordayssincefirst_per_visits","avg_sum_corehome_visitordayssinceorder_per_visits","avg_sum_visitorinterest_visitordayssincelast_per_visits","avg_sum_events_totalevents_per_visits","avg_sum_actions_visittotalinteractions_per_visits","avg_sum_actions_visittotalsearches_per_visits","avg_sum_corehome_profilable_per_visits","avg_sum_corehome_visitorsecondssincefirst_per_visits","avg_sum_corehome_visitorsecondssinceorder_per_visits","avg_sum_visitorinterest_visitorsecondssincelast_per_visits","avg_sum_corehome_visittotaltime_per_visits","bounce_rate","bounce_count","visits_conversion_rate","max_actions","max_corehome_visitordayssincefirst","max_corehome_visitordayssinceorder","max_visitorinterest_visitordayssincelast","max_events_totalevents","max_actions_visittotalinteractions","max_actions_visittotalsearches","max_corehome_visitorsecondssincefirst","max_corehome_visitorsecondssinceorder","max_visitorinterest_visitorsecondssincelast","max_corehome_visittotaltime","sum_actions","sum_corehome_visitordayssincefirst","sum_corehome_visitordayssinceorder","sum_visitorinterest_visitordayssincelast","sum_events_totalevents","sum_actions_visittotalinteractions","sum_actions_visittotalsearches","sum_corehome_profilable","sum_corehome_visitorsecondssincefirst","sum_corehome_visitorsecondssinceorder","sum_visitorinterest_visitorsecondssincelast","sum_corehome_visittotaltime","nb_uniq_devicesdetection_browserengine","nb_uniq_devicesdetection_browserversion","nb_uniq_devicesdetection_browsername","nb_uniq_devicesdetection_clienttype","nb_uniq_devicesdetection_devicebrand","nb_uniq_devicesdetection_devicemodel","nb_uniq_devicesdetection_devicetype","nb_uniq_customdimension_customdimension3","nb_uniq_corehome_idsite","nb_uniq_devicesdetection_osversion","nb_uniq_devicesdetection_os","nb_uniq_resolution_resolution","nb_uniq_customdimension_customdimension2","nb_uniq_corehome_userid","nb_uniq_corehome_visitgoalbuyer","nb_uniq_corehome_visitorreturning","nb_uniq_corehome_visitip","nb_uniq_visitors","nb_uniq_visitors_fingerprints","nb_visits","nb_visits_converted","nb_uniq_usercountry_city","nb_uniq_usercountry_continent","nb_uniq_usercountry_country","nb_uniq_userlanguage_language","nb_uniq_usercountry_latitude","nb_uniq_usercountry_longitude","nb_uniq_usercountry_region","avg_time_dom_completion","avg_time_dom_processing","avg_page_generation_time","avg_time_network","avg_time_on_load","avg_time_server","avg_time_transfer","hits","max_time_dom_completion","max_time_dom_processing","max_time_network","max_time_on_load","max_actions_pagegenerationtime","max_actions_pageviewposition","max_time_server","max_time_transfer","min_time_dom_completion","min_time_dom_processing","min_time_network","min_time_on_load","min_time_server","min_time_transfer","pageviews","pageviews_with_time_dom_completion","pageviews_with_time_dom_processing","pageviews_with_generation_time","pageviews_with_time_network","pageviews_with_time_on_load","pageviews_with_time_server","pageviews_with_time_transfer","sum_time_dom_completion","sum_time_dom_processing","sum_time_network","sum_time_on_load","sum_actions_pagegenerationtime","sum_actions_pageviewposition","sum_time_server","sum_time_transfer","nb_uniq_actions_actionurl","nb_uniq_actions_searchcategory","nb_uniq_actions_clickedurl","nb_uniq_contents_contentinteraction","nb_uniq_contents_contentname","nb_uniq_contents_contentpiece","nb_uniq_contents_contenttarget","nb_uniq_actions_downloadurl","nb_uniq_actions_entrypagetitle","nb_uniq_actions_entrypageurl","nb_uniq_actions_exitpagetitle","nb_uniq_actions_exitpageurl","nb_uniq_actions_searchcount","nb_uniq_actions_searchkeyword","nb_uniq_actions_pagetitle","nb_uniq_actions_pageurl","nb_uniq_customdimension_customdimension1","avg_event_value","events_with_event_value","max_events_eventvalue","min_events_eventvalue","sum_events_eventvalue","nb_uniq_events_eventaction","nb_uniq_events_eventcategory","nb_uniq_events_eventname","nb_uniq_events_eventurl","nb_uniq_marketingcampaignsreporting_campaigncontent","nb_uniq_marketingcampaignsreporting_campaignid","nb_uniq_marketingcampaignsreporting_campaignkeyword","nb_uniq_marketingcampaignsreporting_campaignmedium","nb_uniq_marketingcampaignsreporting_campaignname","nb_uniq_marketingcampaignsreporting_campaignsource","nb_uniq_referrers_referrertype","nb_uniq_referrers_keyword","nb_uniq_referrers_referrername","nb_uniq_referrers_referrerurl","avg_sum_ecommerce_revenuediscount_per_uniq_orders","avg_sum_ecommerce_items_per_uniq_orders","avg_sum_ecommerce_revenue_per_uniq_orders","avg_sum_ecommerce_revenueshipping_per_uniq_orders","avg_sum_ecommerce_revenuesubtotal_per_uniq_orders","avg_sum_ecommerce_revenuetax_per_uniq_orders","max_ecommerce_revenuediscount","max_ecommerce_items","max_ecommerce_revenue","max_ecommerce_productprice","max_ecommerce_productquantity","max_ecommerce_revenueshipping","max_ecommerce_revenuesubtotal","max_ecommerce_revenuetax","max_ecommerce_productviewprice","nb_uniq_orders","sum_ecommerce_revenuediscount","sum_ecommerce_items","sum_ecommerce_revenue","sum_ecommerce_productprice","sum_ecommerce_productquantity","sum_ecommerce_revenueshipping","sum_ecommerce_revenuesubtotal","sum_ecommerce_revenuetax","sum_ecommerce_productviewprice","nb_uniq_ecommerce_productname","nb_uniq_ecommerce_productsku","nb_uniq_ecommerce_productviewcategory","nb_uniq_ecommerce_productviewcategory2","nb_uniq_ecommerce_productviewcategory3","nb_uniq_ecommerce_productviewcategory4","nb_uniq_ecommerce_productviewcategory5","nb_uniq_ecommerce_productviewname","nb_uniq_ecommerce_productviewsku","goal_4_conversion_uniq_visitors_rate","goal_5_conversion_uniq_visitors_rate","goal_2_conversion_uniq_visitors_rate","goal_15_conversion_uniq_visitors_rate","goal_11_conversion_uniq_visitors_rate","goal_29_conversion_uniq_visitors_rate","goal_26_conversion_uniq_visitors_rate","goal_22_conversion_uniq_visitors_rate","goal_23_conversion_uniq_visitors_rate","goal_21_conversion_uniq_visitors_rate","goal_6_conversion_uniq_visitors_rate","goal_8_conversion_uniq_visitors_rate","goal_7_conversion_uniq_visitors_rate","goal_20_conversion_uniq_visitors_rate","goal_19_conversion_uniq_visitors_rate","goal_18_conversion_uniq_visitors_rate","goal_4_conversion","goal_5_conversion","goal_2_conversion","goal_15_conversion","goal_11_conversion","goal_29_conversion","goal_26_conversion","goal_22_conversion","goal_23_conversion","goal_21_conversion","goal_6_conversion","goal_8_conversion","goal_7_conversion","goal_20_conversion","goal_19_conversion","goal_18_conversion","goal_4_daystoconversion","goal_5_daystoconversion","goal_2_daystoconversion","goal_15_daystoconversion","goal_11_daystoconversion","goal_29_daystoconversion","goal_26_daystoconversion","goal_22_daystoconversion","goal_23_daystoconversion","goal_21_daystoconversion","goal_6_daystoconversion","goal_8_daystoconversion","goal_7_daystoconversion","goal_20_daystoconversion","goal_19_daystoconversion","goal_18_daystoconversion","max_goals_revenue","goal_4_revenue","goal_5_revenue","goal_2_revenue","goal_15_revenue","goal_11_revenue","goal_29_revenue","goal_26_revenue","goal_22_revenue","goal_23_revenue","goal_21_revenue","goal_6_revenue","goal_8_revenue","goal_7_revenue","goal_20_revenue","goal_19_revenue","goal_18_revenue","sum_goals_revenue","goal_4_visitstoconversion","goal_5_visitstoconversion","goal_2_visitstoconversion","goal_15_visitstoconversion","goal_11_visitstoconversion","goal_29_visitstoconversion","goal_26_visitstoconversion","goal_22_visitstoconversion","goal_23_visitstoconversion","goal_21_visitstoconversion","goal_6_visitstoconversion","goal_8_visitstoconversion","goal_7_visitstoconversion","goal_20_visitstoconversion","goal_19_visitstoconversion","goal_18_visitstoconversion","avg_form_time_hesitation","avg_form_time_spent","avg_form_time_to_first_submission","form_conversion_rate","nb_form_conversions","form_resubmitters_rate","nb_form_resubmitters","nb_form_starters","form_starters_rate","nb_form_starts","nb_form_submissions","nb_form_submitters","nb_form_viewers","nb_form_views","nb_form_time_hesitation","max_form_views","max_form_time_hesitation","max_form_time_spent","max_form_time_to_first_submission","nb_form_time_spent","nb_form_time_to_first_submission","nb_uniq_paidadvertisingperformance_campaignname","nb_uniq_paidadvertisingperformance_adclickid","nb_uniq_paidadvertisingperformance_adprovider"]
{code}

This patch seems to work for me.

@tsteur tsteur added not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. Needs Review PRs that need a code review labels Oct 7, 2020
@tsteur tsteur added this to the 4.0.0-RC milestone Oct 7, 2020
@tsteur
Copy link
Member Author

tsteur commented Oct 7, 2020

I think the test failures are unrelated.

@diosmosis
Copy link
Member

relevant test failure here: https://travis-ci.org/github/matomo-org/matomo/jobs/733830035#L809

Copy link
Member

@diosmosis diosmosis left a comment

Choose a reason for hiding this comment

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

haven't tested, but code lgtm

@tsteur
Copy link
Member Author

tsteur commented Oct 8, 2020

tests should be fixed now.

@tsteur tsteur merged commit 22809f1 into 4.x-dev Oct 8, 2020
@tsteur tsteur deleted the reporttotalspercentage branch October 8, 2020 04:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review PRs that need a code review not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants