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

Introduce option to disable feed file generation. #1874

Conversation

budzanowski
Copy link
Collaborator

@budzanowski budzanowski commented Apr 12, 2021

Fixes: #1873.

This PR adds an option to disable feed file generation.
A follow-up to this item will be a documentation update that will describe when to disable or enable feed file generation.

In addition to adding the option, we also track it.

I have not updated the changelog since I target this for 2.5.0
Changelog:

Add - Settings option to disable feed file generation.
Add - Track feed file generation option in the tracker.

@budzanowski budzanowski requested a review from a team April 12, 2021 09:43
@budzanowski budzanowski self-assigned this Apr 12, 2021
@budzanowski budzanowski added this to the 2.5.0 milestone Apr 12, 2021
Copy link
Member

@haszari haszari left a comment

Choose a reason for hiding this comment

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

Thanks for prototyping this @budzanowski – this is a good start and maybe part of the process of moving away from feed-based sync.

However I think we need to step back and decide how we want this process to work - do we want to deprecate the option, or gather usage info, or disable in new stores (etc - lots of possibilities here). If we are planning on deprecating, we'll need to do more work on the UI and notices/guidance (maybe even emails) to work through this with merchants.

I tested this PR and it's working as intended. There's a new checkbox in the Product sync UI:

Screen Shot 2021-04-13 at 11 23 34 AM

Disabling this checkbox or the underlying option disables the async task for generating the feed. Here's a screenshot of the task after I re-enabled it:

Screen Shot 2021-04-13 at 11 40 16 AM

Next steps

I'm marking this as blocked until we've had discussion about how this fits in and what UI works best for our needs.

I can think of a few ideas for how we might handle this transition:

  • Add the option, with no UI. This is safe, but doesn't change anything, and allows merchants a way to disable feed on larger stores. We could add a notice or UI for larger stores (many products).
  • Add an option with flipped logic: make the feed disabled by default, and only enable it if stores opt-in. We might need to work through this over a few releases, with docs/guidance, so we don't break stores that rely on the feed.
  • Reduce the frequency of feed generation. It's every 15 minutes at present, if we do it less often, sites won't be "broken" but impact would be reduced. (This doesn't seem very useful.)
  • Disable by default, and track requests for the feed (handle_feed_data_request()) in the store in a transient or store setting somewhere. Then if the feed is requested we can either enable, or guide merchant to disable setting on Facebook end.

Those are just some raw ideas - we need to discuss this and work through the product implications to decide what to ship.

@@ -318,6 +318,27 @@ public function get_settings() {
],
[ 'type' => 'sectionend' ],

[
'name' => __( 'Feed File Settings', 'woocommerce' ),
Copy link
Member

Choose a reason for hiding this comment

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

Should be sentence case.

Though we might want to review the UI for this - having it as a regular option in the UI may be confusing if we're trying to encourage merchants to turn it off.

'title' => __( 'Enable feed file generation.', 'facebook-for-woocommerce' ),
'desc' => sprintf(
/* translators: url to documentation section. */
__( 'Feed file is used for cyclic catalog content synchronization. Please check the documentation at %s to learn if this option should be enabled for your store.', 'facebook-for-woocommerce' ),
Copy link
Member

Choose a reason for hiding this comment

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

Assuming we stick with a checkbox in UI for this, let's iterate on the wording to make it clear what we recommend. We can also use a tooltip to declutter the user interface.

@haszari haszari added the status: blocked The issue is blocked from progressing, waiting for another piece of work to be done. label Apr 12, 2021
@haszari
Copy link
Member

haszari commented Apr 12, 2021

Side note: @budzanowski is there a reason why this branch is on a fork of the repo? 😄

@haszari
Copy link
Member

haszari commented Apr 29, 2021

@budzanowski are you ok with closing this PR? As far as I understand, we're not planning on allowing disable of feed, as it is a necessary part of sync system. Closing - let me know or comment if there's changes on here you want get ship shape 😁

@haszari
Copy link
Member

haszari commented May 6, 2021

Reopening.

Although feed is an important part of product sync, the current implementation may cause issues for larger stores. We're working on improving feed code so it's reliable for all stores.

In the mean time, it would be useful for larger stores to have the option to disable the feed.

Opening a new PR to add that option (may shift to a new PR later). Note this option should not have UI - this option is a support tool for large stores, not a regular option.

Edit: this PR is on a fork, so I'm opening a fresh PR on the repo directly. #1932

@haszari haszari reopened this May 6, 2021
@danielbitzer
Copy link
Contributor

Are you thinking that this option would be supported long-term or is it just a stop-gap solution until we complete the feed rebuild?

@haszari
Copy link
Member

haszari commented May 7, 2021

Are you thinking that this option would be supported long-term or is it just a stop-gap solution until we complete the feed rebuild?

@danielbitzer definitely temporary. Need to consider what happens when we ship the improved feed generation engine - do we want the option to disable that?

@haszari
Copy link
Member

haszari commented May 7, 2021

Closing, see issue #1873 and new PR #1932

@haszari haszari closed this May 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: blocked The issue is blocked from progressing, waiting for another piece of work to be done.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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