Skip to content

REST API: enable widgets module when inserting Business Address widget during JPO#8481

Closed
AnnaMag wants to merge 1 commit intoadd/jpo-save-business-addressfrom
update/jpo-enable-widgets
Closed

REST API: enable widgets module when inserting Business Address widget during JPO#8481
AnnaMag wants to merge 1 commit intoadd/jpo-save-business-addressfrom
update/jpo-enable-widgets

Conversation

@AnnaMag
Copy link
Copy Markdown
Contributor

@AnnaMag AnnaMag commented Jan 8, 2018

Depends on #8480

This patch extends the JPO onboarding request, which inserts Business Address widget, to automatically enable the widgets module, if needed.

Testing instructions:

  • follow the instructions in REST API: add functionality for saving Business Address in JPO #8426 to inject and update Business Address widget
  • deactivate the widgets module wp jetpack module deactivate widgets, go through the steps above and verify that the module was enabled automatically ( wp jetpack module list shows active and the widget is visible with the correct content).

@AnnaMag AnnaMag self-assigned this Jan 8, 2018
@AnnaMag AnnaMag requested a review from a team as a code owner January 8, 2018 09:42
@AnnaMag AnnaMag requested review from ockham and tyxla January 8, 2018 09:42
Business Address is injected and stored in a JP widget, so we need to make sure it is enabled.
@AnnaMag AnnaMag force-pushed the update/jpo-enable-widgets branch from 3029fc2 to bf05241 Compare January 8, 2018 09:58
@AnnaMag AnnaMag changed the base branch from master to add/jpo-save-business-address January 8, 2018 10:01
@tyxla
Copy link
Copy Markdown
Member

tyxla commented Jan 8, 2018

Let's land #8426 and rebase this one before reviewing - it can get tedious to review a chain of PRs. Approach seems sensible though 👍

} else {
Jetpack_Widgets::activate_widget( $id_base, $first_sidebar, $position, $widget_options);
if ( ! $widgets_module_active ) {
if( $first_sidebar ) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could combine those conditions in one if.

}
}
}
} else { $error[] = 'widgets activate'; }
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd move $error[] = 'widgets activate'; on its own line.

@AnnaMag
Copy link
Copy Markdown
Contributor Author

AnnaMag commented Jan 8, 2018

@tyxla thanks for jumping in here. Yes, let's land the previous one first.

@AnnaMag
Copy link
Copy Markdown
Contributor Author

AnnaMag commented Jan 15, 2018

Closing as it will be handled in #8523.

@AnnaMag AnnaMag closed this Jan 15, 2018
@matticbot matticbot removed the [Status] Needs Review This PR is ready for review. label Jan 15, 2018
@tyxla tyxla deleted the update/jpo-enable-widgets branch January 15, 2018 11:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants