From 8c4949dc39ca4a045e0d9958ed243a19a0eccb68 Mon Sep 17 00:00:00 2001 From: Alberto Medina Date: Wed, 21 Jun 2017 13:53:30 -0700 Subject: [PATCH] Incorporate @Mo's reviews --- includes/admin/amp-wp-admin-styles.php | 15 ---- includes/admin/functions.php | 38 +++++----- .../class-amp-analytics-options-submenu.php | 23 +++++- includes/options/class-amp-options-menu.php | 8 +- ...class-amp-analytics-options-serializer.php | 41 ++++++++--- ...ass-amp-analytics-options-submenu-page.php | 19 +++-- .../views/class-amp-options-menu-page.php | 2 +- tests/test-amp-analytics-options.php | 73 ++++++++++--------- 8 files changed, 126 insertions(+), 93 deletions(-) delete mode 100644 includes/admin/amp-wp-admin-styles.php diff --git a/includes/admin/amp-wp-admin-styles.php b/includes/admin/amp-wp-admin-styles.php deleted file mode 100644 index c2f381486f2..00000000000 --- a/includes/admin/amp-wp-admin-styles.php +++ /dev/null @@ -1,15 +0,0 @@ - - ; - init(); } +add_action('admin_menu','amp_add_options_menu'); +// Action to trigger analytics options serializer on analytics option's save +add_action( 'admin_post_analytics_options', 'Analytics_Options_Serializer::save' ); + /** * Grab the analytics options from the DB and return $analytics option * @return array */ -function get_analytics_component_fields($option) { +function amp_get_analytics_component_fields($option) { - $id= $option[0]; - $type = $option[1]; - $config = $option[2]; + list( $id, $type, $config ) = $option; $fields = array(); $component_index = $type . '-' . $id; @@ -104,18 +99,25 @@ function get_analytics_component_fields($option) { $fields['type'] = $type; $fields['attributes'] = array(); - $analytics_json = json_decode( stripslashes( $config ), true ); + $analytics_json = json_decode( $config , true ); $fields['config_data'] = $analytics_json; return $fields; } -function amp_add_custom_analytics( ) { - $analytics = array(); - $analytics_options = get_option( 'analytics', array() ); +function amp_add_custom_analytics() { + $amp_options = get_option('amp-options'); + if ( $amp_options ) { + $analytics_options = $amp_options[ 'amp-analytics']; + } + + if ( ! $analytics_options ) + return; + + $analytics = array(); foreach ( $analytics_options as $option ) { - $fields = get_analytics_component_fields($option); + $fields = amp_get_analytics_component_fields($option); $analytics[$fields['id']] = $fields; } @@ -123,3 +125,5 @@ function amp_add_custom_analytics( ) { } add_filter( 'amp_post_template_analytics', 'amp_add_custom_analytics' ); + + diff --git a/includes/options/class-amp-analytics-options-submenu.php b/includes/options/class-amp-analytics-options-submenu.php index 5649173f798..b0f37e4c83a 100644 --- a/includes/options/class-amp-analytics-options-submenu.php +++ b/includes/options/class-amp-analytics-options-submenu.php @@ -16,16 +16,33 @@ public function __construct( $parent_menu_slug ) { public function init() { $this->add_submenu(); + add_action( + 'admin_head', + 'AMP_Analytics_Options_Submenu::amp_options_styles' + ); } private function add_submenu() { add_submenu_page( $this->parent_menu_slug, - 'AMP Analytics Options', - 'Analytics', + __( 'AMP Analytics Options', 'amp' ), + __( 'Analytics', 'amp' ), 'manage_options', $this->menu_slug, array($this->menu_page, 'render') ); } -} \ No newline at end of file + + public static function amp_options_styles() { + ?> + ; + menu_slug, array( $this->menu_page, 'render' ) @@ -37,4 +38,5 @@ public function add_amp_options_menu( $submenus ) { $submenu->init($this->menu_slug); } } -} \ No newline at end of file +} + diff --git a/includes/options/views/class-amp-analytics-options-serializer.php b/includes/options/views/class-amp-analytics-options-serializer.php index e3fe04ed690..4f950def0fd 100644 --- a/includes/options/views/class-amp-analytics-options-serializer.php +++ b/includes/options/views/class-amp-analytics-options-serializer.php @@ -2,38 +2,55 @@ class Analytics_Options_Serializer { + private static function valid_json( $data ) { + if (!empty($data)) { + @json_decode($data); + return (json_last_error() === JSON_ERROR_NONE); + } + return false; + } + public static function save() { - $option_name = 'analytics'; + $option_name = 'amp-analytics'; - if ( !( empty( $_POST['vendor-type'] ) || - empty( $_POST['config'] ) ) ) { + if ( ! ( empty( $_POST['vendor-type'] ) || empty( $_POST['config'] ) ) && + Analytics_Options_Serializer::valid_json( stripslashes($_POST['config'] ) ) ) { if ( empty( $_POST['id-value'] ) ) { $_POST['id-value'] = md5( $_POST['config'] ); } - $new_analytics_options = array( + // Prepare the data for the new analytics setting + $new_analytics_option = array( $_POST['id-value'], $_POST['vendor-type'], stripslashes( $_POST['config'] ) ); - + // Identifier for analytics option $inner_option_name = $_POST['vendor-type'] . '-' . $_POST['id-value']; - $analytics_options = get_option( $option_name ); - if ( ! $analytics_options ) { - $analytics_options = array(); + + // Grab the amp_options from the DB + $amp_options = get_option( 'amp-options' ); + if ( ! $amp_options ) { + $amp_options = array(); } + // Grab the amp-analytics options + $amp_analytics = isset($amp_options[ $option_name ]) + ? $amp_options[ $option_name ] + : array(); + if ( isset( $_POST['delete'] ) ) { - unset( $analytics_options[ $inner_option_name ] ); + unset( $amp_analytics[ $inner_option_name ] ); } else { - $analytics_options[ $inner_option_name ] = $new_analytics_options; + $amp_analytics[ $inner_option_name ] = $new_analytics_option; } - update_option( $option_name, $analytics_options, false ); + $amp_options[ $option_name ] = $amp_analytics; + update_option( 'amp-options' , $amp_options, false ); } // [Redirect] Keep the user in the analytics options page - // Wrap in is_admin() to enable phpunit tests to exercise this code + // Wrap with is_admin() to enable phpunit tests to exercise this code if ( is_admin() ) { wp_redirect( admin_url( 'admin.php?page=amp-analytics-options' ) ); } diff --git a/includes/options/views/class-amp-analytics-options-submenu-page.php b/includes/options/views/class-amp-analytics-options-submenu-page.php index 591b7ab84cb..8c362f17cbb 100644 --- a/includes/options/views/class-amp-analytics-options-submenu-page.php +++ b/includes/options/views/class-amp-analytics-options-submenu-page.php @@ -9,19 +9,19 @@ private function render_option($id = "", $type = "", $config = "") { ?>
-

Analytics Component:

+

:

- - - - + + + +

- +
- +

@@ -42,7 +42,10 @@ public function render() {

-

AMP Plugin Options

+

This admin panel menu contains configuration options for the AMP Plugin. diff --git a/tests/test-amp-analytics-options.php b/tests/test-amp-analytics-options.php index f61fd6a680f..bbf8f3760fa 100644 --- a/tests/test-amp-analytics-options.php +++ b/tests/test-amp-analytics-options.php @@ -46,13 +46,26 @@ public function setUp() { $this->serializer = new Analytics_Options_Serializer(); } - public function tearDown() { - global $wpdb; - $wpdb->query( 'ROLLBACK' ); + private function get_options() { + $analytics_options = false; + $amp_options = get_option('amp-options'); + if ( $amp_options ) { + $analytics_options = $amp_options[ 'amp-analytics']; + } + + return $analytics_options; } - private function get_options() { - return get_option('analytics'); + private function render_post() { + $user_id = $this->factory->user->create(); + $post_id = $this->factory->post->create( array( 'post_author' => $user_id ) ); + + // Need to use ob here since the method echos + ob_start(); + amp_render_post( $post_id ); + $amp_rendered = ob_get_clean(); + + return $amp_rendered; } private function insert_one_option($vendor, $config) { @@ -75,8 +88,9 @@ function test_no_options() { * Test that exactly one analytics component is inserted into the DB */ function test_one_option_inserted() { - /* Delete analytics options, if any */ - delete_option( 'analytics' ); + + // Delete options, if any + delete_option( 'amp-options' ); /* Insert analytics option */ $this->insert_one_option( @@ -92,8 +106,9 @@ function test_one_option_inserted() { * Test that two analytics components are inserted into the DB */ function test_two_options_inserted() { - /* Delete analytics options, if any */ - delete_option( 'analytics' ); + + // Delete options, if any + delete_option( 'amp-options' ); /* Insert analytics option one */ $this->insert_one_option( @@ -124,13 +139,7 @@ function test_analytics_js_added() { $this->config_one ); - $user_id = $this->factory->user->create(); - $post_id = $this->factory->post->create( array( 'post_author' => $user_id ) ); - - // Need to use ob here since the method echos - ob_start(); - amp_render_post( $post_id ); - $amp_rendered = ob_get_clean(); + $amp_rendered = $this->render_post(); $libxml_previous_state = libxml_use_internal_errors( true ); @@ -156,8 +165,9 @@ function test_analytics_js_added() { * Test that exactly one analytics component are added to the page */ function test_one_analytics_component_added() { - /* Delete analytics options, if any */ - delete_option( 'analytics' ); + + // Delete options, if any + delete_option( 'amp-options' ); /* Insert analytics option */ $this->insert_one_option( @@ -165,20 +175,17 @@ function test_one_analytics_component_added() { $this->config_one ); - $user_id = $this->factory->user->create(); - $post_id = $this->factory->post->create( array( 'post_author' => $user_id ) ); - - // Need to use ob here since the method echos - ob_start(); - amp_render_post( $post_id ); - $amp_rendered = ob_get_clean(); - + // Render AMP post + $amp_rendered = $this->render_post(); + $libxml_previous_state = libxml_use_internal_errors( true ); $dom = new DOMDocument; $dom->loadHTML( $amp_rendered ); $components = $dom->getElementsByTagName( 'amp-analytics' ); + + // One amp-analytics component should be in the page $this->assertEquals(1, $components->length ); libxml_clear_errors(); libxml_use_internal_errors( $libxml_previous_state ); @@ -189,6 +196,10 @@ function test_one_analytics_component_added() { * Test that two analytics components are added to the page */ function test_two_analytics_components_added() { + + // Delete options, if any + delete_option( 'amp-options' ); + $this->insert_one_option( $this->vendor, $this->config_one @@ -199,20 +210,14 @@ function test_two_analytics_components_added() { $this->config_two ); - $user_id = $this->factory->user->create(); - $post_id = $this->factory->post->create( array( 'post_author' => $user_id ) ); - - // Need to use ob here since the method echos - ob_start(); - amp_render_post( $post_id ); - $amp_rendered = ob_get_clean(); + $amp_rendered = $this->render_post(); $libxml_previous_state = libxml_use_internal_errors( true ); $dom = new DOMDocument; $dom->loadHTML( $amp_rendered ); - $components = $dom->getElementsByTagName( 'amp-analytics' ); + // Two amp-analytics components should be in the page $this->assertEquals(2, $components->length ); libxml_clear_errors();