[Enhancement]: Allow dropdown options recording for WooCommerce Settings #38035
Conversation
github-actions
bot
added
the
plugin: woocommerce
Test Results SummaryCommit SHA: db314f1
To view the full API test report, click here. To view the full E2E test report, click here. To view all test reports, visit the WooCommerce Test Reports Dashboard. |
octaedro
changed the title
octaedro
changed the title
octaedro
changed the title
|
Hi , @woocommerce/mothra-enhancements Apart from reviewing the code changes, please make sure to review the testing instructions as well. You can follow this guide to find out what good testing instructions should look like: |
There was a problem hiding this comment.
Nice work on this, @octaedro ! Left a few comments just expressing some concern over potential issues if the option names were not valid Tracks event property names (or, if they conflicted), but we can handle that in a future issue (I'll backlog later).
| @@ -120,6 +141,12 @@ public function send_settings_change_event() { | |||
| } | |||
| } | |||
|
|
|||
| if ( ! empty( $this->modified_options ) ) { | |||
| foreach ( $this->modified_options as $option_name => $selected_option ) { | |||
| $properties[ $option_name ] = $selected_option ?? ''; | |||
There was a problem hiding this comment.
I'm a little concerned that we aren't checking if the $option_name conflicts with any built-in/other Tracks event property names. But, we are already not doing this for $toggled_options $state above, and I suppose our options are all going to start with woocommerce_ or wc_, so we are probably okay for now. Something to keep in mind in the future for tracking... we should ideally namespace/prefix these sort of things when we log them to avoid possible collisions.
There was a problem hiding this comment.
Also a little concerned that we don't do anything to make sure $option_name is a valid Tracks event property name (ensuring use of _ and not -, etc.).
I'll open up a separate issue for us to make this a bit more robust, since this isn't an immediate concern.
There was a problem hiding this comment.
Nice work on this, @octaedro ! Left a few comments just expressing some concern over potential issues if the option names were not valid Tracks event property names (or, if they conflicted), but we can handle that in a future issue (I'll backlog later).
Submission Review Guidelines:
Changes proposed in this Pull Request:
Record events for dropdown menus under WooCommerce Settings.
Closes #37989.
How to test the changes in this Pull Request:
Using the WooCommerce Testing Instructions Guide, include your detailed testing instructions:
General > Selling location(s)).Save changes./wp-admin/admin.php?page=wc-status&tab=logs) and select the file that starts withtracks-[date]-[hash].wcadmin_settings_changeand verify the option has been recorded. In the case of the example I mentioned, the settings name iswoocommerce_allowed_countries.