Skip to content
This repository has been archived by the owner on Feb 23, 2024. It is now read-only.

Store Editing Templates: Added By template information incorrect for customised template based off product-archive.html #5441

Closed
tjcafferkey opened this issue Dec 22, 2021 · 2 comments · Fixed by #5447
Assignees
Labels
focus: FSE Work related to prepare WooCommerce for FSE. focus: template Related to API powering block template functionality in the Site Editor type: bug The issue/PR concerns a confirmed bug.

Comments

@tjcafferkey
Copy link
Contributor

Describe the bug

This appears to be a fairly niche bug and is related to #5380 however this wasn't an issue at the time of merging.

If we are using the product-archive.html template from the theme also for the product categories/tags (because templates for these dont exist in the theme), when we customise one of the product category template (or product tag template) and save it. It will set the template icon as your Gravatar and the added by value will be "demo"

To reproduce

Steps to reproduce the behavior:

  1. Duplicate the Woo Blocks archive-product.html template into your FSE enabled theme.
  2. Go to the templates list within the Site Editor
  3. You will see now that "Product Archive", "Product Category" and "Product Tag" are added by the theme. This is because we are using the themes archive-product.html file for the category and tag templates as they're typically the same.
  4. Customize the Product Category template and save it.
  5. Navigate back to the templates list within the Site Editor. You will notice under the "Added By" column it should say your theme along with a theme icon. However it will display your Gravatar icon along with the text "demo".

Expected behavior

I expect the "Added By" value to be the theme

Screenshots

Screenshot 2021-12-22 at 14 20 41

@tjcafferkey tjcafferkey added type: bug The issue/PR concerns a confirmed bug. focus: FSE Work related to prepare WooCommerce for FSE. focus: template Related to API powering block template functionality in the Site Editor labels Dec 22, 2021
@tjcafferkey
Copy link
Contributor Author

tjcafferkey commented Dec 22, 2021

Whilst having an initial look with @sunyatasattva we've narrowed it down so far to this piece of logic which is responsible for it https://github.com/woocommerce/woocommerce-gutenberg-products-block/blob/trunk/src/BlockTemplatesController.php#L178-L189

And because of this, it doesn't have the required data theme_has_file which the added-by.js component is expecting.

@sunyatasattva
Copy link
Contributor

So, I did some more research and I have also a proposed fix. However, since the intended purpose is a bit obscure to me, I am not going to take any decision just yet. I'll post a little graph of the way I understand the problem: I know it's a bit overkill and I usually wouldn't do that, but since we were pairing and—again—I am not sure whether I understand the problem correctly, I thought it could help.

5441-research

So, as mentioned here, there are basically two lines of responsibility:

  1. One is with us: we could act on our repo to change the duplication avoidance logic mentioned in your comment above and, instead, favor our own parsed template over the one fed to us by Gutenberg.
  2. The second one is Gutenberg: Gutenberg doesn't know of our fallback logic, so it doesn't know one file can serve multiple templates.

One solution for (1.) would be replacing those lines we were talking about with something like this:

array_map(
  function( $query_result_template ) use ( $template_file ) {
    if (
      $query_result_template->slug === $template_file->slug
      && $query_result_template->theme === $template_file->theme
    ) {
      return $template_file;
    } else {
      return $query_result_template;
    }
  }
);

Basically what we are doing here is, instead of continueing if our template is already present, we replace it. Result: no duplicates, our props. It seems to work in my tests.

As for a solution for (2.), I have no idea honestly because I don't know Gutenberg at all. But it seems to me that it could be worth adding some sort of logic or options there to account for these use-cases, which I assume are not entirely unique.

What say ye @tjcafferkey ?

@sunyatasattva sunyatasattva self-assigned this Dec 22, 2021
sunyatasattva added a commit that referenced this issue Dec 23, 2021
Category and tags templates can fallback to the generic archive if, e.g., the theme
provides one for the latter but not for the former. However, since Gutenberg is not
aware of this fallback mechanism, it would incorrectly attribute the custom template
to the user instead of the theme.

Here we are explicitly setting the `has_theme_file` to make sure Gutenberg knows
we do, in fact, have a theme fail (if not what it expects).

Fixes #5441
sunyatasattva added a commit that referenced this issue Dec 24, 2021
* Fix custom templates with fallback being incorrectly attributed

Category and tags templates can fallback to the generic archive if, e.g., the theme
provides one for the latter but not for the former. However, since Gutenberg is not
aware of this fallback mechanism, it would incorrectly attribute the custom template
to the user instead of the theme.

Here we are explicitly setting the `has_theme_file` to make sure Gutenberg knows
we do, in fact, have a theme fail (if not what it expects).

Also skip the loop if template is duplicate but has no fallback

Fixes #5441
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
focus: FSE Work related to prepare WooCommerce for FSE. focus: template Related to API powering block template functionality in the Site Editor type: bug The issue/PR concerns a confirmed bug.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants