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

Ensure that calculateChange() returns zero when both current and previous values are zero. #7172

Closed
techanvil opened this issue Jun 15, 2023 · 1 comment
Labels
P0 High priority Type: Enhancement Improvement of an existing feature

Comments

@techanvil
Copy link
Collaborator

techanvil commented Jun 15, 2023

Feature Description

At the moment, the calculateChange() function returns null when the previous value is zero, as seen here:

export function calculateChange( previous, current ) {
// Prevent divide by zero errors.
if ( '0' === previous || 0 === previous || Number.isNaN( previous ) ) {
return null;
}

However, this is incorrect in the scenario where current is also zero, in which case the change should be returned as 0 rather than null.

This was identified as a fix for a point raised in QA for #6244 (see "Question 2" here), but appears to be a valid fix to make across the board for calculateChange(). However, with this function being used widely in the codebase, due diligence must be applied to confirm this is the case and validate with a good round of testing.

Note: There is a closed PR which can be recreated as the fix for this assuming it's confirmed as above.


Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

  • The ChangeBadge rendered in Key Metric Widgets should show 0% where both, the previous and current values are 0.
  • "Change" badges on other existing components such as the Admin Bar, WPDashboard Widget and DataBlocks on the Main and Entity dashboards should NOT be affected by this change.

Implementation Brief

  • There is a closed PR which can be recreated as the fix for this assuming it's confirmed as above. The calculateChange() function should return 0 when both the previous and current values are zero.

Test Coverage

  • No new tests required.

QA Brief

  • Verify the ACs using a site with Zero data, since such a site will have "zero" values for the previous and current periods.

Changelog entry

  • Fix bug that could cause zero percent in key metric widgets not to appear.
@techanvil techanvil added P0 High priority Type: Enhancement Improvement of an existing feature labels Jun 15, 2023
@jimmymadon jimmymadon self-assigned this Jun 21, 2023
@bethanylang bethanylang added P1 Medium priority and removed P0 High priority labels Jun 22, 2023
@jimmymadon jimmymadon removed their assignment Jun 28, 2023
@tofumatt tofumatt assigned tofumatt and unassigned tofumatt Jun 28, 2023
@bethanylang bethanylang added P0 High priority and removed P1 Medium priority labels Jun 29, 2023
@mohitwp mohitwp assigned mohitwp and unassigned mohitwp Jul 3, 2023
@wpdarren wpdarren self-assigned this Jul 3, 2023
@wpdarren
Copy link
Collaborator

wpdarren commented Jul 5, 2023

QA Update: ✅

Verified:

  • The ChangeBadge rendered in Key Metric Widgets shows 0% where both, the previous and current values are 0.
  • The ChangeBadge on other existing components such as the Admin Bar, WPDashboard Widget and DataBlocks on the Main and Entity dashboards are not affected by this change.
  • Checked the ChangeBadge on a site that is not zero data to make sure that they continue to appear on the Key metrics widget and other areas of the plugin.

image
image

@wpdarren wpdarren removed their assignment Jul 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P0 High priority Type: Enhancement Improvement of an existing feature
Projects
None yet
Development

No branches or pull requests

7 participants