Fetaure: Add Analytics Admin Page, Navigation & Controls#203
Fetaure: Add Analytics Admin Page, Navigation & Controls#203
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new “Analytics” admin submenu under the plugin’s Mailchimp menu, providing UI controls for selecting a date range (presets + custom), filtering by list, and linking out to Mailchimp’s audience analytics dashboard. It also renames the existing settings submenu entry to “Form Settings” to distinguish it from the new Analytics page.
Changes:
- Register a new Analytics admin submenu page and conditionally enqueue its CSS/JS (including Chart.js).
- Add an Analytics template with date range + list filters and an external deep link to Mailchimp analytics.
- Attempt to rename the default settings submenu item to “Form Settings”.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| mailchimp.php | Bootstraps the new Analytics class during plugin load. |
| includes/class-mailchimp-analytics.php | Registers the Analytics submenu and enqueues Analytics assets + Chart.js. |
| includes/class-mailchimp-admin.php | Adds a submenu entry intended to rename the default “Mailchimp” settings submenu item. |
| includes/admin/templates/analytics.php | New Analytics admin page markup (filters + deep link). |
| assets/js/analytics.js | Implements filter interactions and dispatches a refresh event on changes. |
| assets/css/analytics.css | Styles the Analytics filters/content/deep link layout. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| add_submenu_page( | ||
| 'mailchimp_sf_options', | ||
| esc_html__( 'Form Settings', 'mailchimp' ), | ||
| esc_html__( 'Form Settings', 'mailchimp' ), | ||
| MCSF_CAP_THRESHOLD, | ||
| 'mailchimp_sf_options', | ||
| array( $this, 'settings_page' ) | ||
| ); |
There was a problem hiding this comment.
add_submenu_page() with the same slug (mailchimp_sf_options) as the top-level menu does not rename the auto-generated first submenu item; it typically results in a duplicate submenu entry pointing to the same page. To actually rename the first submenu item, update the global $submenu['mailchimp_sf_options'][0][0] label after the menu is registered (or remove/replace the auto-generated item via $submenu manipulation).
| add_submenu_page( | |
| 'mailchimp_sf_options', | |
| esc_html__( 'Form Settings', 'mailchimp' ), | |
| esc_html__( 'Form Settings', 'mailchimp' ), | |
| MCSF_CAP_THRESHOLD, | |
| 'mailchimp_sf_options', | |
| array( $this, 'settings_page' ) | |
| ); | |
| global $submenu; | |
| if ( isset( $submenu['mailchimp_sf_options'][0][0] ) ) { | |
| $submenu['mailchimp_sf_options'][0][0] = esc_html__( 'Form Settings', 'mailchimp' ); | |
| } |
| 'https://cdn.jsdelivr.net/npm/chart.js@4.4.7/dist/chart.umd.min.js', | ||
| array(), | ||
| '4.4.7', |
There was a problem hiding this comment.
Loading Chart.js from a third-party CDN in wp-admin introduces a supply-chain/availability risk and can break in environments that disallow external scripts. Consider bundling Chart.js with the plugin (or using a WordPress-registered script if available) and enqueuing it from assets/ so the admin page works offline/behind strict firewalls and can be vetted/updated via plugin releases.
| 'https://cdn.jsdelivr.net/npm/chart.js@4.4.7/dist/chart.umd.min.js', | |
| array(), | |
| '4.4.7', | |
| MCSF_URL . 'assets/js/chart.umd.min.js', | |
| array(), | |
| MCSF_VER, |
| function toggleCustomDates() { | ||
| const isCustom = dateRangeSelect.value === 'custom'; | ||
| customDates.style.display = isCustom ? 'flex' : 'none'; | ||
| } |
There was a problem hiding this comment.
toggleCustomDates() assumes .mailchimp-sf-custom-dates exists; if it’s missing for any reason, customDates will be null and customDates.style will throw, breaking the page JS. Add a null guard for customDates (and/or for dateRangeSelect) before accessing .style to make the script resilient.
| // Dispatch a custom event so other scripts can listen for filter changes. | ||
| const event = new CustomEvent('mailchimp-analytics-refresh', { | ||
| detail: { | ||
| from: range.from.toISOString(), | ||
| to: range.to.toISOString(), | ||
| listId, | ||
| }, |
There was a problem hiding this comment.
The refresh event converts the selected dates to toISOString(), which shifts the timestamp into UTC and can change the calendar day relative to the user-selected local date (e.g., local midnight becomes the previous day in UTC for positive offsets). If downstream code/API expects date-boundaries matching the picker values, prefer emitting the original YYYY-MM-DD strings (or construct UTC dates explicitly) instead of ISO strings derived from local Date objects.
|
|
||
| // Pass data to JS. | ||
| $lists = get_option( 'mailchimp_sf_lists', array() ); | ||
| $dc = get_option( 'mc_datacenter', '' ); | ||
| $current_id = get_option( 'mc_list_id', '' ); | ||
|
|
||
| wp_localize_script( | ||
| 'mailchimp_sf_analytics_js', | ||
| 'mailchimpAnalytics', | ||
| array( | ||
| 'lists' => $lists, | ||
| 'currentListId' => $current_id, | ||
| 'dataCenter' => $dc, | ||
| ) | ||
| ); |
There was a problem hiding this comment.
wp_localize_script() is currently passing lists, currentListId, and dataCenter to mailchimp_sf_analytics_js, but assets/js/analytics.js does not reference mailchimpAnalytics at all (the list dropdown and deep link are rendered server-side). Consider removing this localization until it’s needed to avoid redundant data injection and confusion about the source of truth.
| // Pass data to JS. | |
| $lists = get_option( 'mailchimp_sf_lists', array() ); | |
| $dc = get_option( 'mc_datacenter', '' ); | |
| $current_id = get_option( 'mc_list_id', '' ); | |
| wp_localize_script( | |
| 'mailchimp_sf_analytics_js', | |
| 'mailchimpAnalytics', | |
| array( | |
| 'lists' => $lists, | |
| 'currentListId' => $current_id, | |
| 'dataCenter' => $dc, | |
| ) | |
| ); |
| wp_enqueue_script( | ||
| 'chartjs', | ||
| 'https://cdn.jsdelivr.net/npm/chart.js@4.4.7/dist/chart.umd.min.js', | ||
| array(), | ||
| '4.4.7', | ||
| true | ||
| ); | ||
|
|
||
| wp_enqueue_script( | ||
| 'mailchimp_sf_analytics_js', | ||
| MCSF_URL . 'assets/js/analytics.js', | ||
| array( 'chartjs' ), | ||
| MCSF_VER, |
There was a problem hiding this comment.
The script handle chartjs is very generic and can collide with other plugins/themes registering the same handle, leading to unexpected version swaps. Use a plugin-scoped handle (e.g., mailchimp_sf_chartjs) and update the dependency reference accordingly.
| $user = get_option( 'mc_user' ); | ||
| $is_logged_in = ! ( ! $user || ( ! get_option( 'mc_api_key' ) && ! mailchimp_sf_get_access_token() ) ); |
There was a problem hiding this comment.
This template computes $user and $is_logged_in but never uses them. Please remove the unused variables (or use $is_logged_in to gate the page content) to avoid dead code and keep the template consistent with the rest of the admin templates.
| $user = get_option( 'mc_user' ); | |
| $is_logged_in = ! ( ! $user || ( ! get_option( 'mc_api_key' ) && ! mailchimp_sf_get_access_token() ) ); |
| $dc = get_option( 'mc_datacenter', '' ); | ||
| ?> | ||
| <div id="mailchimp-sf-analytics-page"> | ||
| <?php require_once MCSF_DIR . 'includes/admin/templates/header.php'; ?> |
There was a problem hiding this comment.
Other admin templates include the shared header via include_once ... // phpcs:ignore PEAR.Files.IncludingFile.UseRequireOnce. Using require_once here without the suppression is likely to trip PHPCS in this repo and is inconsistent with the existing pattern. Please switch to the same include_once + PHPCS ignore comment used in settings.php/create-account-page.php.
| <?php require_once MCSF_DIR . 'includes/admin/templates/header.php'; ?> | |
| <?php include_once MCSF_DIR . 'includes/admin/templates/header.php'; // phpcs:ignore PEAR.Files.IncludingFile.UseRequireOnce ?> |
| return null; | ||
| } | ||
| from = new Date(`${dateFrom.value}T00:00:00`); | ||
| to = new Date(`${dateTo.value}T23:59:59`); |
There was a problem hiding this comment.
getDateRange() does not validate that the custom "From" date is <= the "To" date. As written, selecting an inverted range will still dispatch a refresh event with a nonsensical window. Add a check (and either swap the values or block refresh with a user-visible message) before returning the range.
| to = new Date(`${dateTo.value}T23:59:59`); | |
| to = new Date(`${dateTo.value}T23:59:59`); | |
| if (from > to) { | |
| const temp = from; | |
| from = to; | |
| to = temp; | |
| } |
| public function register_admin_page() { | ||
| if ( ! $this->is_connected() ) { | ||
| return; | ||
| } | ||
|
|
||
| add_submenu_page( | ||
| 'mailchimp_sf_options', | ||
| esc_html__( 'Analytics', 'mailchimp' ), | ||
| esc_html__( 'Analytics', 'mailchimp' ), | ||
| MCSF_CAP_THRESHOLD, | ||
| 'mailchimp_sf_analytics', | ||
| array( $this, 'render_page' ) | ||
| ); |
There was a problem hiding this comment.
The repo has Cypress coverage for the existing admin menu/settings flows (e.g. tests/cypress/e2e/settings/admin.test.js), but this PR introduces a new Analytics submenu, conditional visibility (only when connected), and new filter controls without adding corresponding tests. Please add/extend Cypress tests to cover: submenu presence/absence based on connection, basic filter interactions (preset vs custom dates), and that assets/widgets only load on the Analytics page.
iamdharmesh
left a comment
There was a problem hiding this comment.
Thanks for the PR, @alaca. This looks good overall. I’ve added a few points for discussion, but otherwise this looks good. There is also some feedback from Copilot, could you please review it and address the valid items? Feel free to ignore anything that isn’t relevant.
Thank you.
|
|
||
| wp_enqueue_script( | ||
| 'chartjs', | ||
| 'https://cdn.jsdelivr.net/npm/chart.js@4.4.7/dist/chart.umd.min.js', |
There was a problem hiding this comment.
Better to include this locally as per wp.org plugin repository guideline. (As this plugin is hosted there)
| <div class="mailchimp-sf-analytics-filters"> | ||
| <div class="mailchimp-sf-analytics-filter-group"> | ||
| <label for="mailchimp-sf-date-range"><?php esc_html_e( 'Date Range', 'mailchimp' ); ?></label> | ||
| <select id="mailchimp-sf-date-range"> |
There was a problem hiding this comment.
Could we make the date range similar to what we have in the Figma design? It doesn’t need to be exact/pixel-perfect, but we should align the UI with what we have on the Mailchimp side. Fine to handle this in follow up PR if you want.
https://admin.mailchimp.com/analytics/audience-analytics
| // Rename the auto-generated first submenu item to "Form Settings". | ||
| add_submenu_page( | ||
| 'mailchimp_sf_options', | ||
| esc_html__( 'Form Settings', 'mailchimp' ), |
There was a problem hiding this comment.
| esc_html__( 'Form Settings', 'mailchimp' ), | |
| esc_html__( 'Mailchimp Form Settings', 'mailchimp' ), |
Maybe better to include Mailchimp for page title.
|
|
||
| $user = get_option( 'mc_user' ); | ||
| $is_logged_in = ! ( ! $user || ( ! get_option( 'mc_api_key' ) && ! mailchimp_sf_get_access_token() ) ); | ||
| $lists = get_option( 'mailchimp_sf_lists', array() ); |
There was a problem hiding this comment.
Better to use existing function. Something like:
$lists = ( new Mailchimp_List_Subscribe_Form_Blocks() )->get_lists();
Though we should move get_lists() to utils, but it’s fine not to handle that in this PR.
| if ( ! $this->is_connected() ) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
How about move this to init, just before action hooks?
|
|
||
| wp_enqueue_style( | ||
| 'mailchimp_sf_admin_css', | ||
| MCSF_URL . 'assets/css/admin.css', |
There was a problem hiding this comment.
It seems styling is not getting applied to the header. Could you please help with checking that?
Description of the Change
Adds a new "Analytics" submenu page under the Mailchimp admin menu
Includes date range selector (presets + custom date pickers), list filter dropdown, and a deep link to Mailchimp's audience analytics dashboard
Renames the existing top-level submenu item to "Form Settings" so both pages are clearly distinguished
Closes #197
How to test the Change
Verify "Form Settings" and "Analytics" appear as submenu items under Mailchimp when an account is connected
Verify the Analytics submenu does not appear when no Mailchimp account is connected
Confirm date range dropdown works with all presets and shows resolved date text
Confirm selecting "Custom" reveals from/to date pickers
Confirm that the list filter populates from connected lists and defaults to the currently selected list
Confirm that changing any filter dispatches a refresh event
Verify the deep link opens the Mailchimp dashboard in a new tab
Verify that Chart.js and analytics assets only load on the Analytics page
Changelog Entry
Credits
Props @alaca
Checklist: