-
Notifications
You must be signed in to change notification settings - Fork 10.8k
Fix analytics crashes with gutenberg 21.2 #59846
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 analytics crashes with gutenberg 21.2 #59846
Conversation
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Testing GuidelinesHi @woocommerce/ventures, Apart from reviewing the code changes, please make sure to review the testing instructions (Guide) and verify that relevant tests (E2E, Unit, Integration, etc.) have been added or updated as needed. Reminder: PR reviewers are required to document testing performed. This includes:
|
Size Change: +60 B (0%) Total Size: 5.96 MB |
Test using WordPress PlaygroundThe changes in this pull request can be previewed and tested using a WordPress Playground instance. Test this pull request with WordPress Playground. Note that this URL is valid for 30 days from when this comment was last updated. You can update it by closing/reopening the PR or pushing a new commit. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🚀
378efa1
to
019aeb7
Compare
@@ -333,6 +333,11 @@ class Chart extends Component { | |||
const legendDirection = legendPosition === 'top' ? 'row' : 'column'; | |||
const chartDirection = legendPosition === 'side' ? 'row' : 'column'; | |||
|
|||
// Items label is not defined for all the reports. | |||
const totalLabel = itemsLabel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we check if it's a string?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something like...
const totalLabel = itemsLabel | |
const totalLabel = typeof itemsLabel === 'String' && itemsLabel?.length |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which case would this handle specifically? Just if itemsLabel
is an empty string, that we still call sprintf
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say any case other than a non-empty string. sprintf() indeed expects a string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems to be some structural issues with this component. Why was sprintf
being invoked on itemsLabel
here instead of within the function passing in the prop? itemsLabel
should be passed in as the final string?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The use for sprintf
here is to add the length of the ordered keys into the label, which appears to be only available within this component specifically
Looks like the change in Gutenberg might have been introduced with this (which you've already noticed was a switch to the tannin/sprintf pacakge). Probably worth checking to see there are no other undefined values passed to string and we should flag this more broadly. |
I pushed one change ( 6e2cf9d ) also for the woocommerce/plugins/woocommerce/client/blocks/assets/js/blocks/attribute-filter/edit.tsx Lines 129 to 152 in d03c691
The good thing is that in this instance, the component isn't being shown anyway if items is empty, so it wouldn't run into the above scenario.
|
* Fix issue with passing an undefined value to sprintf for item labels. * Correctly handle sprintf label when label is undefined * Add changelog * Make sure that if noResults is undefined we still pass an empty string. * Update changelog
IMPORTANT: Merging this PR to the appropriate branches is critical to the release process and ensures that the bug does not cause regressions in the future releases. Cherry picking was successful for |
* Fix issue with passing an undefined value to sprintf for item labels. * Correctly handle sprintf label when label is undefined * Add changelog * Make sure that if noResults is undefined we still pass an empty string. * Update changelog
IMPORTANT: Merging this PR to the appropriate branches is critical to the release process and ensures that the bug does not cause regressions in the future releases. Cherry picking was successful for frozen release ( |
Fix analytics crashes with gutenberg 21.2 (#59846) * Fix issue with passing an undefined value to sprintf for item labels. * Correctly handle sprintf label when label is undefined * Add changelog * Make sure that if noResults is undefined we still pass an empty string. * Update changelog Co-authored-by: louwie17 <lourensschep@gmail.com>
Fix analytics crashes with gutenberg 21.2 (#59846) * Fix issue with passing an undefined value to sprintf for item labels. * Correctly handle sprintf label when label is undefined * Add changelog * Make sure that if noResults is undefined we still pass an empty string. * Update changelog Co-authored-by: louwie17 <lourensschep@gmail.com>
Submission Review Guidelines:
Changes proposed in this Pull Request:
With the latest GB version
sprintf
does not allow you to pass inundefined
( expected ), but it looks like we were doing this for some of our Analytics reports.This change makes it so we only call
sprintf
when the items label is defined.Closes WOOA7S-313 .
(For Bug Fixes) Bug introduced in PR # .
Screenshots or screen recordings:
How to test the changes in this Pull Request:
Using the WooCommerce Testing Instructions Guide, include your detailed testing instructions:
Testing that has already taken place:
Changelog entry
Changelog Entry Details
Significance
Type
Message
Changelog Entry Comment
Comment