Skip to content

EP-4812: Refactor to Add Ads Enabled Option #183

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
Mwindo wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

EP-4812: Refactor to Add Ads Enabled Option #183

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
fae6b97
refactor isEnabled to adsEnabled
Jun 29, 2023
492bf2e
small refactor: alphabetize, re-segment admin settings
Jul 3, 2023
ad0a377
see if more debug offers any hints as to build error
Jul 3, 2023
3468cb6
remove -vvv flag
Jul 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions src/Organic/AdConfigSyncCommand.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,11 @@ public function run() {
* @return void
*/
public function __invoke( $args ) {
// Only both trying if the API key is set
if ( ! $this->organic->isEnabledAndConfigured() ) {
// Only worth trying if Ads is enabled and the API key is set
Copy link
Contributor Author

@Mwindo Mwindo Jul 5, 2023

Choose a reason for hiding this comment

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

I think Ads needs to be enabled, but maybe not?

Copy link
Contributor Author

@Mwindo Mwindo Jul 5, 2023

Choose a reason for hiding this comment

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

(E.g., maybe similar to Ads.txt redirect not needing Ads enabled)

Copy link
Contributor

@sergunich sergunich Jul 6, 2023

Choose a reason for hiding this comment

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

Yeah, I think it worth syncing to make sure we have config before enabling Ads

if ( ! $this->organic->adsEnabled() ) {
$this->organic->warning( 'Cannot sync AdConfig: Ads not enabled!' );
}
if ( ! $this->organic->isConfigured() ) {
$this->organic->warning( 'Cannot sync AdConfig without Organic SDK API Key and Site ID' );
return;
}
Expand Down
133 changes: 68 additions & 65 deletions src/Organic/AdminSettings.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,11 +77,6 @@ protected function updateSettings() {
}

// Organic Settings
$this->organic->updateOption(
'organic::enabled',
isset( $_POST['organic_enabled'] ) ? true : false,
false
);
Comment on lines -80 to -84
Copy link
Contributor Author

@Mwindo Mwindo Jul 3, 2023

Choose a reason for hiding this comment

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

"Enabled" will be offloaded to WordPress, i.e., plugin is activated or deactivated. If activated, we will now have more granular options about what, exactly, is enabled, e.g., Ads, Affiliate, etc.

Copy link
Contributor Author

@Mwindo Mwindo Jul 6, 2023

Choose a reason for hiding this comment

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

Nevermind, see #183 (comment)

$this->organic->updateOption(
'organic::test_mode',
isset( $_POST['organic_test_mode'] ) ? true : false,
Expand Down Expand Up @@ -136,21 +131,6 @@ protected function updateSettings() {
false
);

// Organic Affiliate
$this->organic->updateOption(
'organic::affiliate_enabled',
isset( $_POST['organic_affiliate_enabled'] ) ? true : false,
false
);

// Organic Campaigns
$this->organic->updateOption(
'organic::campaigns_enabled',
isset( $_POST['organic_campaigns_enabled'] ) ? true : false,
false
);

// Organic Ads
$this->organic->updateOption(
'organic::feed_images',
isset( $_POST['organic_feed_images'] ) ? true : false,
Expand All @@ -168,16 +148,36 @@ protected function updateSettings() {
);
$this->organic->setPostTypes( $val );

// Organic Ads
$this->organic->updateOption(
'organic::ads_enabled',
isset( $_POST['organic_ads_enabled'] ) ? true : false,
false
);

$this->organic->updateOption(
'organic::content_foreground',
isset( $_POST['organic_content_foreground'] ) ? true : false,
false
);

// Organic Affiliate
$this->organic->updateOption(
'organic::affiliate_enabled',
isset( $_POST['organic_affiliate_enabled'] ) ? true : false,
false
);

// Organic Campaigns
$this->organic->updateOption(
'organic::campaigns_enabled',
isset( $_POST['organic_campaigns_enabled'] ) ? true : false,
false
);

}

public function showSettings() {
$enabled = $this->organic->getOption( 'organic::enabled' );
$test_mode = $this->organic->getOption( 'organic::test_mode' );
$organic_test = $this->organic->getOption( 'organic::percent_test' );
$organic_value = $this->organic->getOption( 'organic::test_value' );
Expand All @@ -190,6 +190,8 @@ public function showSettings() {
$ad_slots_prefill_enabled = $this->organic->getOption( 'organic::ad_slots_prefill_enabled' );
$log_to_sentry = $this->organic->getOption( 'organic::log_to_sentry', true );

$ads_enabled = $this->organic->getOption( 'organic::ads_enabled' );
Copy link
Contributor

@sergunich sergunich Jul 6, 2023

Choose a reason for hiding this comment

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

This means that after the plugin upgrade, the Ads will become disabled because the organic::ads_enabled field in DB will be empty

If the organic::ads_enabled is empty, we should pre-populate it based on behavior of previous versions of the plugin: if plugin/integration is enabled and configured, then set it to true

Copy link
Contributor Author

@Mwindo Mwindo Jul 6, 2023

Choose a reason for hiding this comment

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

Good call, thank you!


$affiliate_enabled = $this->organic->getOption( 'organic::affiliate_enabled' );

$campaigns_enabled = $this->organic->getOption( 'organic::campaigns_enabled' );
Expand Down Expand Up @@ -217,16 +219,6 @@ public function showSettings() {
<h1>Organic Settings</h1>
<form method="post">
<?php wp_nonce_field( 'organic_settings_nonce' ); ?>
<p>
<label>
<input
type="checkbox"
name="organic_enabled"
<?php echo $enabled ? 'checked' : ''; ?>
>
Organic Integration Enabled
</label>
</p>
<p>
<label>
<input
Expand Down Expand Up @@ -336,76 +328,87 @@ class="organic-wide-input"
Automatically send plugin errors to Organic
</label>
</p>

<hr />
<h2>Organic Affiliate</h2>
<p>
<label>
<input
type="checkbox"
name="organic_affiliate_enabled"
<?php echo $affiliate_enabled ? 'checked' : ''; ?>
type="checkbox"
name="organic_feed_images"
<?php echo $feed_images ? 'checked' : ''; ?>
/>
Organic Affiliate Enabled
Inject Images into RSS Feed (for Connatix Playspace player)
</label>
</p>

<hr />
<h2>Organic Campaigns</h2>
<p>
<label>
<input
type="checkbox"
name="organic_campaigns_enabled"
<?php echo $campaigns_enabled ? 'checked' : ''; ?>
type="checkbox"
name="organic_enable_ads_txt_redirect"
<?php echo $ads_txt_redirect ? 'checked' : ''; ?>
/>
Organic Campaigns Enabled
Ads.txt Redirect Enabled
</label>
</p>
<hr />
<h2>Content Sync Settings</h2>
<fieldset>
<p>
Which post types from your CMS should be treated as content for synchronization with
the Organic Platform and as eligible for the Organic SDK to be loaded on?
<ul>
<?php $this->injectPostTypesList(); ?>
</ul>
</p>

</fieldset>
<p>
<label>
<input
type="checkbox"
name="organic_content_foreground"
<?php echo $content_foreground ? 'checked' : ''; ?>
/>
Force content sync on Save (use only if CRON is disabled on your site)
</label>
</p>
<hr />
<h2>Organic Ads</h2>
<p>
<label>
<input
type="checkbox"
name="organic_feed_images"
<?php echo $feed_images ? 'checked' : ''; ?>
type="checkbox"
name="organic_ads_enabled"
<?php echo $ads_enabled ? 'checked' : ''; ?>
/>
Inject Images into RSS Feed (for Connatix Playspace player)
Organic Ads Enabled
</label>
</p>
<hr />
<h2>Organic Affiliate</h2>
<p>
<label>
<input
type="checkbox"
name="organic_enable_ads_txt_redirect"
<?php echo $ads_txt_redirect ? 'checked' : ''; ?>
name="organic_affiliate_enabled"
<?php echo $affiliate_enabled ? 'checked' : ''; ?>
/>
Ads.txt Redirect Enabled
Organic Affiliate Enabled
</label>
</p>

<hr />
<h2>Organic Campaigns</h2>
<p>
<label>
<input
type="checkbox"
name="organic_content_foreground"
<?php echo $content_foreground ? 'checked' : ''; ?>
name="organic_campaigns_enabled"
<?php echo $campaigns_enabled ? 'checked' : ''; ?>
/>
Force content sync on Save (use only if CRON is disabled on your site)
Organic Campaigns Enabled
</label>
</p>
<fieldset>
<p>
Which post types from your CMS should be treated as content for synchronization with
the Organic Platform and as eligible for the Organic SDK to be loaded on?
<ul>
<?php $this->injectPostTypesList(); ?>
</ul>
</p>

</fieldset>

<hr />
<p>
<button type="submit" name="organic_action" value="organic_update_settings">
Save settings
Expand Down
2 changes: 1 addition & 1 deletion src/Organic/AdsTxtSyncCommand.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ public function __invoke( $args ) {
}

// Only both trying if the API key is set
if ( ! $this->organic->isEnabledAndConfigured() ) {
if ( ! $this->organic->isConfigured() ) {
$this->organic->warning( 'Cannot sync Ads.txt without Organic SDK API Key and Site ID' );
return;
}
Expand Down
2 changes: 1 addition & 1 deletion src/Organic/ContentIdMapSyncCommand.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ public function run() {
* @return void
*/
public function __invoke( $args ) {
if ( ! $this->organic->isEnabledAndConfigured() ) {
if ( ! $this->organic->isConfigured() ) {
$this->organic->warning( 'Cannot sync Content Id Map without enabled integration with SDK API Key and Site ID' );
return;
}
Expand Down
2 changes: 1 addition & 1 deletion src/Organic/ContentSyncCommand.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ public function run() {
* @since 0.1.0
*/
public function __invoke( $args, $opts ) {
if ( ! $this->organic->isEnabledAndConfigured() ) {
if ( ! $this->organic->isConfigured() ) {
$this->organic->warning( 'Cannot sync articles without enabled integration with SDK API Key and Site ID' );
return;
}
Expand Down
2 changes: 1 addition & 1 deletion src/Organic/GraphQL.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ public function register_organic_types() {
$adsConfig = $organic->getAdsConfig();
$testEnabled = $organic->useSplitTest();
return [
'organicEnabled' => $organic->isEnabledAndConfigured(),
'organicEnabled' => $organic->isConfigured(),
Copy link
Contributor

@sergunich sergunich Jul 6, 2023

Choose a reason for hiding this comment

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

Right now, a lot of Recurrent sites do have Organic WP plugin enabled and configured in WP, but the "integration" checkbox is disabled

That's because Organic Web has a hard dependency on Organic WP plugin GQL API and they can't just disable/delete the whole plugin

And by passing the $organic->isConfigured() as organicEnabled value will lead to injecting and setting up Organic SDK on those sites (even if Ads and Affiliate are disabled the Core is always enabled in the SDK)
https://github.com/orgnc/organic-content/blob/ae0c8844ae433483ac26300a8f8a2b8489490a19/frontend/packages/core/components/common/OrganicSDK/DocumentHeadOrganicTags.tsx#L184

Copy link
Contributor Author

@Mwindo Mwindo Jul 6, 2023

Choose a reason for hiding this comment

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

That's because Organic Web has a hard dependency on Organic WP plugin GQL API

Ah, ok, I see. I wonder if we can modify the language or add plugin field descriptions to make this all clearer? As a client looking at the settings page, I would be extremely confused. What does it mean for the integration to be enabled--isn't the plugin already active? If it's enabled but Ads, Affiliate, and Campaigns are not, then what is the plugin doing? Etc.

Tagging @cjkie here too.

Copy link
Contributor Author

@Mwindo Mwindo Jul 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

For instance, I wonder if something like this taxonomy would work:

General Settings

  • Organic API Key
  • Organic Site ID
  • Consent Management Program
  • Prefill Containers Enabled
  • AMP Enabled
  • Inject Images into RSS Feed ...
  • Automatically send plugin errors to Organic

SDK and Content Sync

  • SDK enabled (automatically load the SDK on selected pages) [= isEnabled for Organic Web]
  • Which post types from your CMS should be ...
  • Force content sync on Save ...

Organic Ads

  • Ads.txt Redirect Enabled <-- Does not need Ads Enabled, but logically fits under Ads
  • Organic Ads Enabled

Organic Affiliate

  • Organic Affiliate Enabled

Organic Campaigns

  • Organic Campaigns Enabled

'sdkVersion' => $organic->getSdkVersion(),
'siteDomain' => $organic->siteDomain,
'siteId' => $organic->getSiteId(),
Expand Down
Loading