Open Graph Ingestion Flow #643

diegoquinteiro · 2017-03-16T01:46:48Z

This PR is a clean/rebased version of #548

Creates a new Ingestion flow for instant articles
Simplifies the configuration on the WordPress plugin, since only page ID is needed
Removed the configuration wizard, since the new ingestion flow covers all features we had on the API

everton-rosario

What an amazing cleanup!
Just mind the small observations I've made thru this diff.

everton-rosario · 2017-03-16T12:39:21Z

facebook-instant-articles.php

@@ -342,20 +334,47 @@ function inject_url_claiming_meta_tag() {
 		$publishing_settings = Instant_Articles_Option_Publishing::get_option_decoded();
 		$fb_page_settings = Instant_Articles_Option_FB_Page::get_option_decoded();

-		if ( isset( $fb_page_settings['page_id'] ) ) {
+		if ( isset( $fb_page_settings[ 'page_id' ] ) ) {


nice catch 💅

everton-rosario · 2017-03-16T12:39:33Z

facebook-instant-articles.php

 			?>
-			<meta property="fb:pages" content="<?php echo esc_attr( $fb_page_settings['page_id'] ); ?>" />
+			<meta property="fb:pages" content="<?php echo esc_attr( $fb_page_settings[ 'page_id' ] ); ?>" />


nice catch 💅

everton-rosario · 2017-03-16T12:40:43Z

facebook-instant-articles.php

+		// Transform the post to an Instant Article.
+		$adapter = new Instant_Articles_Post( $post );
+		$url = $adapter->get_canonical_url();
+		$url = add_query_arg( 'op', '1', $url );


Since we removed the op: ("Open publisher") thing. Shouldn't we change this op=1 thing?

Will replace "op" by "ia_markup"

everton-rosario · 2017-03-16T12:41:19Z

facebook-instant-articles.php

+		<meta property="ia:markup_url" content="<?php echo esc_attr( $url ); ?>" />
+		<?php
+	}
+	add_action( 'wp_head', 'inject_op_markup_meta_tag' );


Same thing about "op" usage. Maybe we should drop this OP from everywhere.

everton-rosario · 2017-03-16T12:42:43Z

facebook-instant-articles.php

+	function op_markup_version( ) {
+		$post = get_post();
+
+		if (isset($_GET['op']) && $_GET['op']) {


2 things:

Formating nit, should be if ( isset( $_GET[ 'op' ] ) && $_GET[ 'op' ] ) {

Same thing about "op" usage. Maybe we should drop this OP from everywhere.

everton-rosario · 2017-03-16T12:43:11Z

facebook-instant-articles.php

+			// Transform the post to an Instant Article.
+			$adapter = new Instant_Articles_Post( $post );
+			$article = $adapter->to_instant_article();
+			echo $article->render(null, true);


💅
echo $article->render( null, true );

everton-rosario · 2017-03-16T12:43:27Z

facebook-instant-articles.php

+			die();
+		}
+	}
+	add_action( 'wp', 'op_markup_version' );


Same thing about "op" usage. Maybe we should drop this OP from everywhere.

everton-rosario · 2017-03-16T12:44:31Z

js/instant-articles-wizard.js

@@ -1,135 +0,0 @@
-function instant_articles_wizard_load ( data ) {


Good code cleanup! 👍

everton-rosario · 2017-03-16T12:45:50Z

tests/Facebook/InstantArticles/Transformer/WPTransformerTest.php

@@ -73,6 +73,7 @@ public function testTransformerLikeWPContent()

        $this->assertEquals($expected, $result);
        // there must be 3 warnings related to <img> inside <li> that is not supported by IA
+        // And 1 warning related to the getter


Are you sure this comment should be added?
If there are 3 related to img + 1, the test assertion should be 4.
I guess this came from some dirty merge.

Makes sense =) I'll remove it.

everton-rosario

🔥 Burning code!

sakatam

LGTM! Great work, @diegoquinteiro!

everton-rosario

Just rebase before landing

This ingestion flow takes in consideration how is the current status of the setup. If setup is already complete on API, it means a migration, set the configuration as API and the flow goes that way otherwise set the configuration as Open Graph and flow goes this way

- [x] Edit doesn't erase content - [x] Fixed issue on Advanced Settings - [x] Removed extra code

* Removed the wizard * Added a page_id field on the settings * Removed unused pages, javascripts and templates

…cles-wp/pull/548/files/603561e4afc1cfa3de85c17961311e26f4e860d2#r97752732

diegoquinteiro

We (I and @vkama) agreed to merge it to v4.0 then fix the issues on a new PR.

diegoquinteiro · 2017-05-29T21:15:55Z

meta-box/meta-box-template.php

+	</b>
+</p>
+<hr>
+<?php elseif ( ! $should_submit_post ) : ?>


Rename this variable

diegoquinteiro · 2017-05-29T21:18:07Z

meta-box/meta-box-template.php

+	</b>
+</p>
+<hr>
+<?php elseif ( ! $article->getHeader() || ! $article->getHeader()->getTitle() ) : ?>


Move to a warning on the SDK

diegoquinteiro · 2017-05-29T21:18:22Z

meta-box/meta-box-template.php

+<p>
+	<b>
+		<span class="dashicons dashicons-no-alt"></span>
+		This post will not be submitted to Instant Articles because it is missings content.


Typo (missings)

diegoquinteiro · 2017-05-29T21:18:30Z

meta-box/meta-box-template.php

+	</b>
+</p>
+<hr>
+<?php elseif ( count( $article->getChildren() ) === 0 ) : ?>


Move to a warning on the SDK

vkama

Very nice work! Excited to see such a huge clean up and improved user experience through not having to deal with tokens and other stuff!

Great job!

vkama · 2017-05-30T11:55:00Z

facebook-instant-articles.php

@@ -102,7 +102,7 @@ function instant_articles_activate() {
 	add_action( 'network_admin_notices', 'instant_articles_setup_admin_notice' ); // also show message on multisite
 	function instant_articles_setup_admin_notice() {
 		global $pagenow;
-		if ( $pagenow === 'plugins.php' && Instant_Articles_Wizard_State::get_current_state() !== Instant_Articles_Wizard_State::STATE_REVIEW_SUBMISSION ) {
+		if ( $pagenow === 'plugins.php' && ! Instant_Articles_Option_FB_Page::get_option_decoded()[ "page_id" ] ) {


Can this possibly throw a warning if no 'page_id' is set? Also, maybe using single quotes.

vkama · 2017-05-30T11:57:15Z

meta-box/class-instant-articles-meta-box.php

@@ -96,7 +96,7 @@ public static function render_meta_box() {
 		$published = ( 'publish' === $post->post_status );
 		$dev_mode = false;
 		$force_submit = get_post_meta( $post_id, IA_PLUGIN_FORCE_SUBMIT_KEY, true );
-		$should_submit_post = apply_filters( 'instant_articles_should_submit_post', true, $adapter );
+		$instant_articles_should_submit_post_filter = apply_filters( 'instant_articles_should_submit_post', true, $adapter );


I am seeing this a lot in the code (lines 92 to 99), maybe we should think of a better solution so this boilerplate gets reused? Good for now, but lets aim for this refactor.

vkama · 2017-05-30T12:04:03Z

facebook-instant-articles.php

+					60
+				);
+				foreach ( $old_slugs as $slug ) {
+					$clone_post = clone $post;


should this clonning really be inside the foreach? since only the post name is getting changed, maybe you can clone it once, outside, if at all.

good call, I'll create a PR for improving the performance here

vkama · 2017-05-30T12:06:19Z

facebook-instant-articles.php

+		$old_slugs = get_post_meta( $post_id, '_wp_old_slug' );
+		if ( $adapter->should_submit_post() ) {
+			try {
+				$client = Facebook\HttpClients\HttpClientsFactory::createHttpClient( null );


is this now the only time we use the graph-sdk? if so, maybe we should consider either using builtin functions for a simple post or actually copying the class and dropping it as a dep?

vkama · 2017-05-30T12:07:36Z

facebook-instant-articles.php

@@ -404,4 +404,44 @@ function ia_markup_version( ) {
 	add_action( 'wp', 'ia_markup_version' );

 	Instant_Articles_Wizard::init();
+
+	function rescrape_article( $post_id, $post ) {


is the $post_id required? isnt that the same as $post->ID?

It is not required, but these are the parameters passed to the hook by WordPress.

diegoquinteiro added the SemVer: MAJOR label Mar 16, 2017

diegoquinteiro added this to the 4.0 milestone Mar 16, 2017

diegoquinteiro self-assigned this Mar 16, 2017

diegoquinteiro requested a review from everton-rosario March 16, 2017 01:46

diegoquinteiro mentioned this pull request Mar 16, 2017

Og ingestion clean #548

Closed

3 tasks

diegoquinteiro force-pushed the og_ingestion_flow branch from e387f6d to 78c1f54 Compare March 16, 2017 01:53

diegoquinteiro requested a review from sakatam March 16, 2017 01:54

diegoquinteiro force-pushed the og_ingestion_flow branch from 78c1f54 to e6f6840 Compare March 16, 2017 01:54

everton-rosario suggested changes Mar 16, 2017

View reviewed changes

diegoquinteiro changed the title ~~Og ingestion flow~~ Open Graph Ingestion Flow Mar 16, 2017

diegoquinteiro force-pushed the og_ingestion_flow branch 2 times, most recently from dec8fdd to cd9b2ce Compare March 16, 2017 16:52

everton-rosario approved these changes Mar 16, 2017

View reviewed changes

sakatam approved these changes Mar 21, 2017

View reviewed changes

everton-rosario approved these changes Apr 3, 2017

View reviewed changes

everton-rosario reviewed Apr 3, 2017

View reviewed changes

diegoquinteiro force-pushed the og_ingestion_flow branch from f8dc6e4 to 738ff77 Compare May 15, 2017 20:49

everton-rosario and others added 12 commits May 29, 2017 17:26

Refactoring to fix menu issues and the configuration save issue.

0a0121c

Removed unused resources

d06c0e7

Removed entries from styles and scripts not used anymore

05de48c

- [x] Apparted content data from OpenGraph setup from API setup

d872800

- [x] Edit doesn't erase content - [x] Fixed issue on Advanced Settings - [x] Removed extra code

Clean up new ingestion flow:

890a450

* Removed the wizard * Added a page_id field on the settings * Removed unused pages, javascripts and templates

Test fix

2208d29

@demoive request: https://github.com/Automattic/facebook-instant-arti…

e690b75

…cles-wp/pull/548/files/603561e4afc1cfa3de85c17961311e26f4e860d2#r97752732

Remove API triggers

c7ad1e9

Change meta-tag to ia:markup_url

7504fe1

@everton-rosario's comments

1d4cc63

Further cleanup of unused code

af398c3

Diego Moreno Quinteiro added 4 commits May 29, 2017 17:26

Preserve meta-box and submission logic

1019133

Forces filling the app id before viewing other settings

d115061

Minor text/flow fixes

72bac1f

Add rescrape for post updates and permalink changes

9013127

diegoquinteiro force-pushed the og_ingestion_flow branch from 57a9635 to 9013127 Compare May 29, 2017 20:37

diegoquinteiro changed the base branch from master to v4.0 May 29, 2017 20:38

diegoquinteiro commented May 29, 2017

View reviewed changes

vkama self-requested a review May 30, 2017 11:49

vkama approved these changes May 30, 2017

View reviewed changes

diegoquinteiro merged commit 39dc2d1 into v4.0 May 30, 2017

diegoquinteiro deleted the og_ingestion_flow branch May 30, 2017 12:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Open Graph Ingestion Flow #643

Open Graph Ingestion Flow #643

diegoquinteiro commented Mar 16, 2017 •

edited

Loading

everton-rosario left a comment

everton-rosario Mar 16, 2017

everton-rosario Mar 16, 2017

everton-rosario Mar 16, 2017

diegoquinteiro Mar 16, 2017

everton-rosario Mar 16, 2017

everton-rosario Mar 16, 2017

everton-rosario Mar 16, 2017

everton-rosario Mar 16, 2017

everton-rosario Mar 16, 2017

everton-rosario Mar 16, 2017

diegoquinteiro Mar 16, 2017

everton-rosario left a comment

sakatam left a comment

everton-rosario left a comment

diegoquinteiro left a comment

diegoquinteiro May 29, 2017

diegoquinteiro May 29, 2017

diegoquinteiro May 29, 2017

diegoquinteiro May 29, 2017

vkama left a comment

vkama May 30, 2017

vkama May 30, 2017

vkama May 30, 2017

diegoquinteiro May 30, 2017

vkama May 30, 2017

vkama May 30, 2017

diegoquinteiro May 30, 2017

		@@ -1,135 +0,0 @@
		function instant_articles_wizard_load ( data ) {

Open Graph Ingestion Flow #643

Open Graph Ingestion Flow #643

Conversation

diegoquinteiro commented Mar 16, 2017 • edited Loading

everton-rosario left a 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

everton-rosario left a comment

Choose a reason for hiding this comment

sakatam left a comment

Choose a reason for hiding this comment

everton-rosario left a comment

Choose a reason for hiding this comment

diegoquinteiro left a 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

vkama left a 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

diegoquinteiro commented Mar 16, 2017 •

edited

Loading