Enable adding AMP Analytics components using the Options API #714

amedina · 2017-06-14T00:40:09Z

The goal is to provide a simple mechanism for users to add AMP analytics components to posts via a configuration panel of analytics options.

This is an initial PR to kick off the review process. Things missing: (1) testing; (2) ironing some rough edges (e.g. details on the AMP top-level menu); (3) Styling of the Analytics options page

…Options API

amedina · 2017-06-14T00:57:14Z

…ull page describing the options available

amedina · 2017-06-19T17:35:36Z

mjangda · 2017-06-20T00:27:32Z

includes/admin/amp-wp-admin-styles.php

+<?php
+
+add_action('admin_head', 'amp_options_styles');
+function amp_options_styles() {


Can we move this inside the AMP_Analytics_Options_Submenu class? We should also limit it to so that the CSS is only output on the AMP Analytics page.

(1) Agree. Moved.
(2) What is the best way to restrict it to the analytics options page? The page does not use a template; should I use the URL parameter 'page='amp-analytics'?

We can use this hook instead: https://developer.wordpress.org/reference/hooks/admin_print_styles-hook_suffix/

add_action( 'admin_print_styles-' . $this->menu_slug, array( $this, 'amp_options_styles' ) );

Hmm. I replaced the action added to admin_head with this one and the styles do not get applied.

mjangda · 2017-06-20T21:43:33Z

includes/admin/functions.php

+ * Grab the analytics options from the DB and return $analytics option
+ * @return array
+ */
+function get_analytics_component_fields($option) {


Should be prefixed with amp_

mjangda · 2017-06-20T21:45:17Z

includes/options/class-amp-analytics-options-submenu.php

+	private function add_submenu() {
+		add_submenu_page(
+			$this->parent_menu_slug,
+			'AMP Analytics Options',


Need to pass this and all other translatable strings through the appropriate i18n function.

mjangda · 2017-06-20T21:46:00Z

includes/admin/functions.php

-		AMP_CUSTOMIZER_QUERY_VAR   => true,
-	), 'customize.php' );
+	// Add a link to Settings
+	add_action( 'admin_menu', 'amp_add_amp_options_link' );


This should be moved out of amp_init_customizer since it's not related to the customizer. Probably a new callback hooked on admin_init.

Agree. Moved.

mjangda · 2017-06-20T23:57:58Z

includes/admin/functions.php

-		$menu_slug
-	);
+	// Trigger analytics options serializer on analytics option's save
+	add_action( 'admin_post_analytics_options', 'Analytics_Options_Serializer::save' );


We should probably use register_setting here with the sanitize_callback.

Let's discuss.

mjangda · 2017-06-21T00:02:09Z

includes/options/views/class-amp-analytics-options-submenu-page.php

+							<label>Type: </label>
+							<input class="option-input" type="text" name=vendor-type value="<?php echo $type; ?>" />
+							<label>Id: </label>
+							<input type="text" name=id value="<?php echo substr($id, -6); ?>" text="alberto" readonly />


text="alberto"? :)

esc_attr for the $id

text="alberto"? Of course! :) ==> removed.

mjangda · 2017-06-21T00:02:33Z

includes/options/views/class-amp-analytics-options-submenu-page.php

+							<input class="option-input" type="text" name=vendor-type value="<?php echo $type; ?>" />
+							<label>Id: </label>
+							<input type="text" name=id value="<?php echo substr($id, -6); ?>" text="alberto" readonly />
+                            <input type="hidden" name=id-value value="<?php echo $id; ?>" />


esc_attr for $id

mjangda · 2017-06-21T00:02:53Z

includes/options/views/class-amp-analytics-options-submenu-page.php

+						<p>
+							<label>JSON Configuration:</label>
+							<br />
+							<textarea rows="10" cols="100" name="config"><?php echo stripslashes($config); ?></textarea>


esc_textarea for $config.

mjangda · 2017-06-21T00:03:28Z

tests/test-amp-analytics-options.php

+
+	public function tearDown() {
+		global $wpdb;
+		$wpdb->query( 'ROLLBACK' );


Is this necessary? I think the class already does this for us?

True; leftover from trials.

mjangda · 2017-06-21T00:09:46Z

includes/options/views/class-amp-analytics-options-serializer.php

+
+class Analytics_Options_Serializer {
+
+	public static function save() {


As discussed, we should look into using the settings API for this instead of manually handling the save (e.g. https://konstantin.blog/2012/the-wordpress-settings-api/). Right now, this handler doesn't have proper capability checks or CSRF protection, which we get "for free" with the settings API.

We can register the setting with register_setting and with the sanitize_callback where most of the logic from this save would move to. Also, thinking to the future, it might be good to nest options under an amp_settings option so the structure looks something like:

amp_settings => array( amp_analytics => array( 'googleanalytics-basd12' => array( ... ), 'parsely-sua231' => array( ... ), ), )

If we want to keep it as a top-level option for analytics, we need to rename it to amp_analytics_config (to avoid conflicts with other plugins/themes).

I'll see if I can pull together an example of moving things over.

(1) Good idea re:Thinking about the future. The options are saved now according to the hierarchy you suggested.
(2) Thinking about using the Settings API. Need to change everything? :(

[Discussed offline] Keeping the use of the Options API.

amedina · 2017-06-22T17:46:13Z

tests/test-amp-analytics-options.php

-		delete_option( 'analytics' );
+
+		// Delete options, if any
+		delete_option( 'amp-options' );


Related to the ROLLBACK comment before: some tests were failing seemingly due the DB not being rolled back between tests; it should be but... Adding this delete_option back for now.

…user_can(manage_options)

…alytics option form action submission

mjangda · 2017-06-30T20:52:38Z

Looks like there are some test fatals related to the updated code:

PHP 5.2
https://travis-ci.org/Automattic/amp-wp/jobs/248203194

Fatal error: Call to undefined function json_last_error() in /home/travis/build/Automattic/amp-wp/includes/utils/class-amp-html-utils.php on line 24

PHP 5.3+
https://travis-ci.org/Automattic/amp-wp/jobs/248203203

1) AMP_Render_Post_Test::test__invalid_post
pre_amp_render_post action fire when it should not have.
Failed asserting that 3 matches expected 0.
/home/travis/build/Automattic/amp-wp/tests/test-amp-render-post.php:9
2) AMP_Render_Post_Test::test__valid_post
Failed asserting that 4 matches expected 1.

…test suite; Constrain placement of amp-analytics-options styles

Fix spacing, indents, and yoda conditions

…tic/amp-wp into amedina/amp-analytics-customizer

To simplify fetching nested options

Clearer titles, better escaping, code formatting.

…tic/amp-wp into amedina/amp-analytics-customizer

Done

Or if we have an error

On wp_loaded so that we can move the admin_post hook into it's init action, since they're closely tied.

To more accurately represent their function

For clarity and to store less data but in a more structured way.

…tic/amp-wp into amedina/amp-analytics-customizer

amedina added 2 commits June 12, 2017 18:10

Add configuration option to specify analytics components

b156c13

Second part of rnabling the addition of analytics components via the …

8a6154c

…Options API

amedina requested review from mjangda and pbakaus June 14, 2017 00:56

Fix hardwired redirect to analytics options page

edeb8ce

amedina force-pushed the amedina/amp-analytics-customizer branch from 48213a0 to edeb8ce Compare June 14, 2017 16:55

Add draft of test cases (not done\!)

c88a3da

amedina force-pushed the amedina/amp-analytics-customizer branch from d25f053 to c88a3da Compare June 15, 2017 20:22

amedina added 8 commits June 16, 2017 09:50

Merge branch 'master' into amedina/amp-analytics-customizer

5c8c781

Extend test cases

6bc95d1

Remove unused constructor

9b9f190

Add very simple content to the AMP Options page; we must add a nice f…

046b0d2

…ull page describing the options available

Add minimal style for delete button

1b13f74

Fix tests to work with auto-generated ids

38b6c52

Add function to define styles for admin pages

82f794b

Wrap form redirection in is_admin() to enable phpunit tests

8f22974

Fix proper handling of form submission in serializer

854da9c

amedina force-pushed the amedina/amp-analytics-customizer branch from 76aac15 to 854da9c Compare June 19, 2017 18:43

mjangda suggested changes Jun 21, 2017

View reviewed changes

amedina commented Jun 22, 2017

View reviewed changes

Incorporate @mo's reviews

8c4949d

amedina force-pushed the amedina/amp-analytics-customizer branch from 22e41dc to 8c4949d Compare June 22, 2017 17:47

amedina added 5 commits June 23, 2017 18:07

Make form submission safer; add nonce, check_admin_referrer, current_…

e9dcfe1

…user_can(manage_options)

Wrap safety checks around if_admin() to enable tests

ad30a3b

Fix access to DOMNodeList element

84b9716

Simplify test data with a simpler test 'requests' URL

50a2021

Add very simple feedback mechanism to indeicate success/failure of an…

3939fb9

…alytics option form action submission

amedina force-pushed the amedina/amp-analytics-customizer branch from f29bf55 to 3939fb9 Compare June 26, 2017 22:00

Address Mo's latest review

973c77a

Fix PHP 5.2 compatibility for json_last_error; add paret::setUp() to …

9b5a6b2

…test suite; Constrain placement of amp-analytics-options styles

amedina force-pushed the amedina/amp-analytics-customizer branch from d5704bf to 9b5a6b2 Compare June 30, 2017 23:01

amedina self-assigned this Jul 4, 2017

amedina mentioned this pull request Jul 6, 2017

Automatically add Google Analytics (and others?) when they are already enabled #541

Closed

mjangda and others added 8 commits July 10, 2017 16:59

Coding Standards

70d9a74

Fix spacing, indents, and yoda conditions

wp_redirect => wp_safe_redirect

7f37177

Add missing translation

2562a6e

Merge branch 'amedina/amp-analytics-customizer' of github.com:Automat…

d944f9f

…tic/amp-wp into amedina/amp-analytics-customizer

Add get_option helper method

e266eb2

To simplify fetching nested options

Cleanup analytics page

b0499d3

Clearer titles, better escaping, code formatting.

Wrap labels around inputs for better usability

58a6085

Merge branch 'amedina/amp-analytics-customizer' of github.com:Automat…

924dbd2

…tic/amp-wp into amedina/amp-analytics-customizer

mjangda and others added 13 commits July 14, 2017 17:13

Only show admin notice when we actually save

42b6fad

Or if we have an error

Add missing translation for Dismiss notice

e641cdf

More i18n translations

00efbbc

Rename valid_json => is_valid_json

b64bf03

Add AMP icon to menu

58aef94

Init option menu earlier

1d838e4

On wp_loaded so that we can move the admin_post hook into it's init action, since they're closely tied.

Rename Options Manager methods for clarity

b84d373

To more accurately represent their function

Flatten out update method for readability

be7a8ff

Use $data var instead of $POST object

c7772ab

Disable admin page on WP.com

2c059fc

Refactor Options Manager

800dd95

For clarity and to store less data but in a more structured way.

Merge branch 'amedina/amp-analytics-customizer' of github.com:Automat…

8c4244d

…tic/amp-wp into amedina/amp-analytics-customizer

Generate hash string for (vendor,config) to avoid duplicates

e7ba261

amedina merged commit c0bdac8 into master Jul 20, 2017

westonruter deleted the amedina/amp-analytics-customizer branch February 3, 2018 19:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable adding AMP Analytics components using the Options API #714

Enable adding AMP Analytics components using the Options API #714

amedina commented Jun 14, 2017 •

edited

Loading

amedina commented Jun 14, 2017

amedina commented Jun 19, 2017

mjangda Jun 20, 2017

amedina Jun 21, 2017

mjangda Jun 23, 2017

amedina Jun 26, 2017

mjangda Jun 20, 2017

amedina Jun 21, 2017

mjangda Jun 20, 2017

amedina Jun 21, 2017

mjangda Jun 20, 2017

amedina Jun 21, 2017

mjangda Jun 20, 2017

amedina Jun 21, 2017

mjangda Jun 21, 2017

mjangda Jun 21, 2017

amedina Jun 21, 2017

mjangda Jun 21, 2017

amedina Jun 21, 2017

mjangda Jun 21, 2017

amedina Jun 21, 2017

mjangda Jun 21, 2017

amedina Jun 21, 2017

mjangda Jun 21, 2017

amedina Jun 21, 2017

amedina Jun 26, 2017

amedina Jun 22, 2017 •

edited

Loading

mjangda commented Jun 30, 2017


		class Analytics_Options_Serializer {

		public static function save() {

Enable adding AMP Analytics components using the Options API #714

Enable adding AMP Analytics components using the Options API #714

Conversation

amedina commented Jun 14, 2017 • edited Loading

amedina commented Jun 14, 2017

amedina commented Jun 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amedina Jun 22, 2017 • edited Loading

Choose a reason for hiding this comment

mjangda commented Jun 30, 2017

amedina commented Jun 14, 2017 •

edited

Loading

amedina Jun 22, 2017 •

edited

Loading