Skip to content
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.

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

add option for disabling existing feed #1932

Merged
merged 1 commit into from May 9, 2021

Conversation

haszari
Copy link
Member

@haszari haszari commented May 7, 2021

Fixes #1873

Changes proposed in this Pull Request:

This PR, inspired by #1874, adds a new hidden option to allow sites to disable the current feed generation code.

  • Option: wc_facebook_legacy_feed_file_generation_enabled
  • Value: yes | no string
  • Default: yes

Feature flag for new feed engine?

This PR is focused tightly on the current issue for larger stores and giving them a targeted option to disable. At the same time, we may want a feature flag to help us ship the new-improved feed engine (work in progress).

It might be a good idea to consider the feature flag use case (or even implement in this PR) to ensure this remedial/temporary option doesn't impact our ability to ship improvements to larger stores! See code comment for details.

On the other hand, if we end up using a UI or guided process for enabling the new feed engine, the option can be reset/removed as part of running that. E.g. task list item says you should enable a feed for improved product sync > merchant clicks Set it up, accepts defaults, feed is configured, disable option is removed.

Reviewers let me know what you think about this :) cc @budzanowski @danielbitzer

How to test the changes in this Pull Request:

  1. Check out master. Confirm that the feed generation job is running:
  • Enable debug logging - Facebook > Connection > Log events for debugging.
  • Should see feed profiler logs in facebook_for_woocommerce_profiling.
    • e.g. 2021-05-07T02:27:17+00:00 DEBUG generate_feed - Memory: 23,045.33 KB, time: 2.49s
  • Should see feed job in `WooCommerce > Status > Scheduled Actions.
    • wc_facebook_regenerate_feed

Screen Shot 2021-05-07 at 2 31 09 PM

Screen Shot 2021-05-07 at 2 45 51 PM

Optional - set up your site with lots of products to reproduce performance issues.

  1. Set the option to opt-out of feed. e.g. using wp-cli wp option set wc_facebook_legacy_feed_file_generation_enabled no.
  2. Confirm that the wc_facebook_regenerate_feed is no longer pending and the feed is not regenerated. Previous feed generation interval was every fifteen minutes.
  3. Opt back in - set the option to yes or delete it. Confirm that the feed starts regenerating again (should happen within ~30 mins?).

Changelog entry

New - Add option to allow larger sites to opt-out of current feed generation (product sync)

@haszari haszari self-assigned this May 7, 2021
@haszari haszari requested a review from a team May 7, 2021 02:09
@haszari haszari force-pushed the fix/allow-large-sites-to-disable-legacy-feed branch from abd3c7e to c711a0a Compare May 7, 2021 02:13
* @return bool
*/
public function is_legacy_feed_file_generation_enabled() {
return (bool) ( 'yes' === get_option( self::OPTION_LEGACY_FEED_FILE_GENERATION_ENABLED, 'yes' ) );
Copy link
Member Author

@haszari haszari May 7, 2021

Choose a reason for hiding this comment

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

Keen for feedback on how this option works.

  • Our immediate, urgent need is to disable the current feed generation (I'm calling this "legacy" in the code/option name).
  • We also want to ship an improved feed engine soon - should the option affect that?

To limit the scope of this option, I've decided it should only affect the legacy feed system. When we merge the new improved version, we will need to adjust the logic for the option too.

Alternatives

  • Add two options - enable/disable, and "mode". Like a feature flag, so we can ship the new feed engine in parallel and sites can opt-in to the new or old version. This would allow us to ship early and test improvements without them affecting customers. This is most flexible, but might be overkill.
  • Add a single "mode" option which includes some combinations of enable/disable and engine mode, e.g. disable | allow_default | allow_experimental. This is a feature flag style approach, but with 'stable' (aka default) and 'experimental'; at some point the new engine code would move from experimental to default and apply to all stores.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We also want to ship an improved feed engine soon - should the option affect that?

No. The control structure for the new feed is in a totally different place.

Add two options - enable/disable, and "mode". Like a feature flag, so we can ship the new feed engine in parallel and sites can opt-in to the new or old version. This would allow us to ship early and test improvements without them affecting customers. This is most flexible, but might be overkill.

This would confuse people. Besides if the new feed works why have the old one still around?

Add a single "mode" option which includes some combinations of enable/disable and engine mode, e.g. disable | allow_default | allow_experimental. This is a feature flag style approach, but with 'stable' (aka default) and 'experimental'; at some point the new engine code would move from experimental to default and apply to all stores.

That could be an interesting idea but it should be introduced with an "experimental" build of the plugin and not by a flag.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good - we can look at ways to selectively enable the new engine in future (in line with your and @danielbitzer feedback).

@haszari haszari added this to the 2.5.0 milestone May 7, 2021
$store_allows_sync = $configured_ok && $integration->is_product_sync_enabled();
// Only schedule if has not opted out of feed generation (e.g. large stores).
$store_allows_feed = $configured_ok && $integration->is_legacy_feed_file_generation_enabled();
if ( ! $store_allows_sync || ! $store_allows_feed ) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if ( ! $store_allows_sync || ! $store_allows_feed ) {
if ( ! ( $store_allows_sync && $store_allows_feed ) ) {

I think that this way we are more explicit about the intention.

Copy link
Member Author

@haszari haszari May 9, 2021

Choose a reason for hiding this comment

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

The suggested change would be clearer IMO if the if condition was the positive case, but since we have to negate that undermines the clarity (and adds brackets, making it more complex). I'm leaving as is - the idea is that if either option is disabled, we'll bail - and the code reads like that. If that doesn't make sense let me know, we can revisit :)

@budzanowski
Copy link
Collaborator

This needs a paragraph in the documentation.

@danielbitzer
Copy link
Contributor

This looks good to me. I think we should hold off on considering whether to implement a "new-feed feature flag" until the new feed is in a usable state.

@haszari haszari added type: documentation The issue is a request for better documentation. type: enhancement The issue is a request for an enhancement. labels May 9, 2021
@haszari
Copy link
Member Author

haszari commented May 9, 2021

This needs a paragraph in the documentation.

Good point - thanks @budzanowski. Added documentation label for now, I'll handle updating docs as part of 2.5.0.

Note - I don't think this should be promoted and documented as a regular option - it's a temporary workaround for specific (large) stores. IMO the main thing is that it's documented for tech support and in troubleshooting section of doc.

@haszari haszari merged commit d7a6a65 into master May 9, 2021
@haszari haszari deleted the fix/allow-large-sites-to-disable-legacy-feed branch May 9, 2021 20:19
@haszari haszari added the feature: product sync Relating to syncing product data to Facebook. label May 10, 2021
@haszari
Copy link
Member Author

haszari commented May 20, 2021

If anyone is looking for docs on how to use this option, here's a brief outline.

To disable the legacy feed engine, you'll need to set the option to no on your store. For more information about WordPress options, check out the documentation.

You can do this with WP-CLI, or with a mu plugin. For example:

wp option set wc_facebook_legacy_feed_file_generation_enabled no

or

<?php
/* mu-plugins/disable-legacy-facebook-feed.php */

function yourprefix_disable_facebook_legacy_feed() {
  update_option( 'wc_facebook_legacy_feed_file_generation_enabled', 'no' );
}
add_action( 'init', 'yourprefix_disable_facebook_legacy_feed' );

If you need to re-enable the feed again, simply delete the option.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature: product sync Relating to syncing product data to Facebook. type: documentation The issue is a request for better documentation. type: enhancement The issue is a request for an enhancement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add setting that allows enabling/disabling feed file generation.
3 participants