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 template title and description on 'get_block_template' hook #44254

Merged
merged 7 commits into from Feb 6, 2024

Conversation

Aljullu
Copy link
Contributor

@Aljullu Aljullu commented Feb 1, 2024

Changes proposed in this Pull Request:

Closes #42221.

This PR adds the template title and description in the get_block_template hook, which fixes #42221. I also added some tests to verify there are no regressions in the future and took the opportunity to simplify how we attach the template title and description in add_block_templates().

How to test the changes in this Pull Request:

Verify there are no regressions:

Move around the Site Editor freely, keeping an eye on WooCommerce templates titles and descriptions, making sure they always appear correctly.

Verify #42221 is fixed:

  1. Enable a theme with custom WooCommerce template, like Tsubaki.
  2. Go to Appearance > Editor > Templates and edit a WooCommerce template (ie: Single Product). Note: make sure you didn't have any previous edits on that template. This fix doesn't work for templates that had been modified in the past.
  3. Make some edits in the template, save, and verify the title of the template doesn't change. (Before the fix, it would change from Single Product to single-product).
Enregistrament.de.pantalla.del.2024-02-01.11-10-29.webm

Changelog entry

  • Automatically create a changelog entry from the details below.

Significance

  • Patch
  • Minor
  • Major

Type

  • Fix - Fixes an existing bug
  • Add - Adds functionality
  • Update - Update existing functionality
  • Dev - Development related task
  • Tweak - A minor adjustment to the codebase
  • Performance - Address performance issues
  • Enhancement - Improvement to existing functionality

Message

Fix the Site Editor showing the template slug instead of the template title when saving a WooCommerce block template customized by the theme.

Comment

@Aljullu Aljullu added focus: template Issue related to WooCommerce templates. team: Kirigami & Origami labels Feb 1, 2024
@Aljullu Aljullu self-assigned this Feb 1, 2024
@github-actions github-actions bot added the plugin: woocommerce Issues related to the WooCommerce Core plugin. label Feb 1, 2024
@woocommercebot woocommercebot requested review from a team and gigitux and removed request for a team February 1, 2024 10:19
Copy link
Contributor

github-actions bot commented Feb 1, 2024

Hi @gigitux,

Apart from reviewing the code changes, please make sure to review the testing instructions as well.

You can follow this guide to find out what good testing instructions should look like:
https://github.com/woocommerce/woocommerce/wiki/Writing-high-quality-testing-instructions

@Aljullu Aljullu force-pushed the fix/42221-wrong-template-title branch from e0ea654 to 9fff6ed Compare February 1, 2024 10:27
Copy link
Contributor

github-actions bot commented Feb 1, 2024

Test Results Summary

Commit SHA: ef5d508

Test 🧪Passed ✅Failed 🚨Broken 🚧Skipped ⏭️Unknown ❔Total 📊Duration ⏱️
API Tests25900202610m 37s
E2E Tests291001803097m 38s

To view the full API test report, click here.
To view the full E2E test report, click here.
To view all test reports, visit the WooCommerce Test Reports Dashboard.

@Aljullu Aljullu force-pushed the fix/42221-wrong-template-title branch from 9fff6ed to fde3d84 Compare February 1, 2024 10:45
Copy link
Contributor

@gigitux gigitux left a comment

Choose a reason for hiding this comment

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

The fix LGTM, and I'm pre-approving this PR.

The only doubt that I have is that the current E2E test runs on TT4. So, even without your fix, the test doesn't fail.

Should we add a theme that overrides the Woo template to cover all the potential cases?

@Aljullu
Copy link
Contributor Author

Aljullu commented Feb 6, 2024

Thanks for the review, @gigitux!

The only doubt that I have is that the current E2E test runs on TT4. So, even without your fix, the test doesn't fail.

Heads-up that we have a specific workflow named Playwright E2E tests - BlockThemeWithTemplates that runs with a special theme with WooCommerce templates. All files ending with .block_theme_with_templates.spec.ts are run as part of that workflow. So the test modified in this PR is run with that theme, not with TT4.

I tried it again, and commenting out L61 causes the tests to fail. You need to run the tests like this:

pnpm run test:e2e:block-theme-with-templates ./tests/e2e/tests/templates/template-customization.block_theme_with_templates.spec.ts

@gigitux
Copy link
Contributor

gigitux commented Feb 6, 2024

Heads-up that we have a specific workflow named Playwright E2E tests - BlockThemeWithTemplates that runs with a special theme with WooCommerce templates. All files ending with .block_theme_with_templates.spec.ts are run as part of that workflow. So the test modified in this PR is run with that theme, not with TT4.

TIL! Cool! At the same time, should we run the tests with both use cases?

  • a theme that overrides the WooCommerce templates (like theme-with-woo-templates)
  • a theme that doesn't override the WooCommerce templates (like TT4)

This discussion is unrelated to this PR, so feel free to merge it! Great work! 🚀

@Aljullu
Copy link
Contributor Author

Aljullu commented Feb 6, 2024

TIL! Cool! At the same time, should we run the tests with both use cases?

Good question. We do have tests for a block theme that doesn't override WooCommerce templates here. I was on the fence on whether I should add the check I added in this PR to verify template title is shown correctly to these other tests.

In fact, I initially did but undid it before committing. 😄 The reason I leaned towards not adding it was that it has never been a bug in block themes without custom templates, so it felt that it was an unnecessary assertion that would pollute the test. But honestly, I still don't have a strong opinion on this, so happy to add it if you think otherwise.

@gigitux
Copy link
Contributor

gigitux commented Feb 6, 2024

The main thing is that with E2E tests, we should cover all the possible cases to have good coverage.

I don't know if it is possible, but when it makes sense (like in this case), we should run the same test on both configurations because we expect the behavior to be the same.

@Aljullu
Copy link
Contributor Author

Aljullu commented Feb 6, 2024

Makes sense. In ef5d508 I added the same check to the test that modifies the templates in block themes without custom templates.

@Aljullu Aljullu merged commit fa9d42b into trunk Feb 6, 2024
44 checks passed
@Aljullu Aljullu deleted the fix/42221-wrong-template-title branch February 6, 2024 12:33
@github-actions github-actions bot added this to the 8.7.0 milestone Feb 6, 2024
@github-actions github-actions bot added the needs: analysis Indicates if the PR requires a PR testing scrub session. label Feb 6, 2024
@nigeljamesstevenson nigeljamesstevenson added needs: external testing Indicates if the PR requires further testing conducted by testers external to the development team. and removed needs: analysis Indicates if the PR requires a PR testing scrub session. labels Feb 7, 2024
@nigeljamesstevenson nigeljamesstevenson added the status: analysis complete Indicates if a PR has been analysed by Solaris label Feb 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
focus: template Issue related to WooCommerce templates. needs: external testing Indicates if the PR requires further testing conducted by testers external to the development team. plugin: woocommerce Issues related to the WooCommerce Core plugin. status: analysis complete Indicates if a PR has been analysed by Solaris team: Kirigami & Origami
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BlockTemplatesController: The template name changes when the user saves the template
3 participants