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

Clear Used CSS when Divi saves template item #5837

Closed
2 of 6 tasks
DahmaniAdame opened this issue Mar 24, 2023 · 12 comments · Fixed by #6074
Closed
2 of 6 tasks

Clear Used CSS when Divi saves template item #5837

DahmaniAdame opened this issue Mar 24, 2023 · 12 comments · Fixed by #6074
Assignees
Labels
3rd party compatibility Issues related to 3rd party compatibility like theme, plugin or hosting effort: [S] 1-2 days of estimated development time module: remove unused css needs: copywriting needs: documentation Issues which need to create or update a documentation priority: medium Issues which are important, but no one will go out of business. type: enhancement Improvements that slightly enhance existing functionality and are fast to implement
Milestone

Comments

@DahmaniAdame
Copy link
Contributor

DahmaniAdame commented Mar 24, 2023

Before submitting an issue please check that you’ve completed the following steps:

  • Made sure you’re on the latest version 3.12.6.1
  • Used the search feature to ensure that the bug hasn’t been reported before

Describe the bug
Divi has a template builder feature where the header, body, and footer can be set globally or under specific conditions.

If one of them is changed, we are not triggering the Used CSS clearing. This results in whatever was changed to have their style missing.

Divi stores them on specific post types:

et_footer_layout
et_body_layout
et_header_layout

All posts linked to a specific header, body, or footer will be documented on a customer field called, respectively, _et_header_layout_id, _et_body_layout_id, _et_footer_layout_id.

We can track et_save_post, check if one of the specific post types is being saved, and then fire the following for any post with the corresponding custom field to have its Used CSS cleared, which should also clear the cache.

et_save_post takes the $post_id as a parameter.

The custom field to target can be built using:

'_' . get_post_type( $post_id ) . '_id'

With an SQL command that uses that specific custom field to get the matching posts to be processed.

To Reproduce
Steps to reproduce the behavior:

  1. Set a global/custom header, body, or footer
  2. Generate Used CSS
  3. Change the global/custom header, body, or footer by adding a new element with a specific style
  4. Clear the cache
  5. See that the style of the changed item is missing

Expected behavior
We should clear the Used CSS when items from Divi's Theme Builder change.

Screenshots
N/A

Additional context
Ticket - https://secure.helpscout.net/conversation/2184716932/408393/

Backlog Grooming (for WP Media dev team use only)

  • Reproduce the problem
  • Identify the root cause
  • Scope a solution
  • Estimate the effort
@piotrbak piotrbak added type: enhancement Improvements that slightly enhance existing functionality and are fast to implement 3rd party compatibility Issues related to 3rd party compatibility like theme, plugin or hosting priority: medium Issues which are important, but no one will go out of business. needs: grooming module: remove unused css labels Mar 27, 2023
@jeawhanlee jeawhanlee added GROOMING IN PROGRESS Use this label when the issue is currently being groomed. and removed needs: grooming labels Apr 5, 2023
@jeawhanlee
Copy link
Contributor

Reproduce the problem ✅

The problem was easy to reproduce.

Identify the root cause ✅

Like @DahmaniAdame we are not clearing the used css when a template item changes.

Scope a solution ✅

We will create a new method in the divi compatibility class which will take $post_id as a parameter and subscribe to the et_save_post event.

we will bail out if non of the custom post types match any of this _et_header_layout_id or this _et_body_layout_id or this _et_footer_layout_id
then get url to clear used css:

$url = get_permalink( $post_id );
$this->used_css->clear_url_usedcss( $url );

Add tests too

Estimate the effort ✅

[S]

@jeawhanlee jeawhanlee added effort: [S] 1-2 days of estimated development time and removed GROOMING IN PROGRESS Use this label when the issue is currently being groomed. labels Apr 5, 2023
@engahmeds3ed engahmeds3ed self-assigned this Apr 6, 2023
@piotrbak
Copy link
Contributor

piotrbak commented Apr 9, 2023

@engahmeds3ed Not sure if you're still working on it, if not, could you unassign and move it back to to do?

@engahmeds3ed
Copy link
Contributor

I'm working on it

@engahmeds3ed
Copy link
Contributor

After a discussion with @jeawhanlee we agreed on the following:

As @DahmaniAdame mentioned in the issue, When saving the template, the action et_save_post is fired so we can check the post type if it matches the template types so get the affected posts by making a raw query to get the posts with the meta key_POSTTYPE_id then clear the used CSS and cache for them.

My concern here is that may be a resource consuming enhancement on the customer's side and our SaaS side for large sites so I was thinking of two approaches:

  1. Provide a filter to enable/disable this feature so if a site reached us about that we can disable it with a helper.
  2. Check the number of affected posts and if exceeded a predefined number we handle them in patches using action scheduler.

Any suggestions? or you see that over complicate it?

@DahmaniAdame
Copy link
Contributor Author

@engahmeds3ed why not do both?
Sounds like we will be covering all possible scenarios if both approaches are combined while making sure the process will be under control for large websites.

@piotrbak piotrbak added this to the 3.13.3 milestone Apr 14, 2023
@engahmeds3ed
Copy link
Contributor

@engahmeds3ed why not do both? Sounds like we will be covering all possible scenarios if both approaches are combined while making sure the process will be under control for large websites.

Good one, Yes we can do that and I believe that would work.

@piotrbak piotrbak modified the milestones: 3.13.3, 3.14 Apr 23, 2023
@piotrbak piotrbak added the needs: documentation Issues which need to create or update a documentation label Apr 25, 2023
@piotrbak piotrbak removed this from the 3.14 milestone May 26, 2023
@engahmeds3ed
Copy link
Contributor

there is a missing part here, which is: the post ids mentioned in the last step before clearing used CSS are not real post ids they are the divi layouts' ids so with those ids we need to get their post meta with the key _et_use_on which are multiple values in the following format:

image

and there are many variations in divi like here:

image

Thanks @DahmaniAdame for confirming that, seems like we have few approaches here that we need to take a decision about:-

  1. Clear the whole RUCSS table automatically with any change in template.
  2. Handle all scenarios mentioned in Divi variations (specific posts, all posts, specific post type posts, all post type posts, specific authot, tag, term, archive, ...etc.) and only clear what is needed to be cleared.
  3. After saving any template we can show admin notice to ask the user to clear used CSS manually.

The second approach will take more effort that the first one but it will make the process of saving templates more heavy and more CPU usage.

What do u think @wp-media/php I'm out of ideas

@piotrbak
Copy link
Contributor

@DahmaniAdame What do you think about 3rd approach? It would be similar to what we'll be doing with Elementor, and applying that kind of pattern for different themes/builders would make a good UX while keeping the code easy to maintain.

We need to keep in mind that any breaking changes in the 3rd-party code might need reopening this issue if we go with handle all scenarios way

@DahmaniAdame
Copy link
Contributor Author

DahmaniAdame commented Jul 28, 2023

@piotrbak @engahmeds3ed
2 will be maintenance intensive.
Ideally, it should be 1. But it would be problematic in case the customer is editing the template and doing multiple saves.
3 sounds like a good middle ground. Let's go for it if there are no other options besides these to consider.

@piotrbak
Copy link
Contributor

piotrbak commented Jul 28, 2023

Okay, so, the Acceptance Criteria here will be:

  1. Whenever Divi Template is updated and Used CSS is enabled, we'll display a dismissible warning on all admin pages (just like for activation/deactivation of the plugins, lack of the CSS is major thing)
  2. Whenever Divi Template is updated and Used CSS is disabled, notice will not be displayed.
  3. Wording will be as following: WP Rocket: Your Divi template was updated. Clear the Used CSS if the layout, design or CSS styles were changed.
  4. After clicking on Clear Used CSS, we'll purge the Used CSS and Cache
  5. After clicking on Dismiss, the message will disappear
  6. If the Divi template will be updated in the future, the message should be displayed once again

@engahmeds3ed
Copy link
Contributor

@piotrbak I have two questions here:

  1. When the RUCSS option is enabled then disabled (so we still have used CSS in the DB table) and we change divi templates then enabled RUCSS again, should we show the message? => if Yes this will add a query to check if the RUCSS DB table has at least one row when saving our settings to decide if we need to remove the transient or not.
  2. In the notice, u mean to add a direct button to clear used CSS? I thought u mean when clicking on the clear used CSS button in the dashboard.

@piotrbak
Copy link
Contributor

piotrbak commented Aug 2, 2023

Thanks @engahmeds3ed good point about the used css in the database. Let's go with proposed solution if used CSS is disabled.

For the notice, yes, I meant introducing a button, sorry for not being clear enough

@piotrbak piotrbak modified the milestone: 3.14.3 Aug 2, 2023
@piotrbak piotrbak added this to the 3.14.4 milestone Aug 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3rd party compatibility Issues related to 3rd party compatibility like theme, plugin or hosting effort: [S] 1-2 days of estimated development time module: remove unused css needs: copywriting needs: documentation Issues which need to create or update a documentation priority: medium Issues which are important, but no one will go out of business. type: enhancement Improvements that slightly enhance existing functionality and are fast to implement
Projects
None yet
4 participants