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

21318 refactor seo data provision to script data #21339

Open
wants to merge 8 commits into
base: trunk
Choose a base branch
from

Conversation

thijsoo
Copy link
Contributor

@thijsoo thijsoo commented Apr 24, 2024

Context

  • This PR is part of a greater refactor of the way we handle our script data.

Summary

This PR can be summarized in the following changelog entry:

  • Changes the way we handle the SEO data for our metabox and sidebar.

Relevant technical choices:

  • The SEO data is split up in two parts. One for the Terms and one for the posts. They both end up filling the same domain object with the same naming that ends up in the script data. For the rest it is similar to the already refactored bits of script data.
  • There are some calls still to static methods of old classes. I chose to not refactor this as well to not increase the scope of this issue to much.

Test instructions

Test instructions for the acceptance test before the PR gets merged

This PR can be acceptance tested by following these steps:

Do the following for both posts and terms.

Keyword usage:

  • Create or edit a post /category and add the keyphrase the keyphrase to it. Note down the post id.
  • Create or edit a different post/category and add the keyphrase the keyphrase to it aswell.
  • Add the following to your browser console to see the value: wpseoScriptData.metabox.keyword_usage Make sure the first post is there. Also add wpseoScriptData.metabox.keyword_usage_post_types and make sure it says your keyphrase and your post type in there.

Meta description:

  • Now add wpseoScriptData.metabox.metadesc_template make sure the value is the same as your meta description.
  • This is not applicable for terms but is for posts: Also add wpseoScriptData.metabox.metaDescriptionDate and make sure it is either the current date or the date your post got changed the last time.
  • Edit the meta description and save. Make sure the wpseoScriptData.metabox.metadesc_template changes accordingly. and wpseoScriptData.metabox.metaDescriptionDate is updated to the current date

Meta title:

  • Now add wpseoScriptData.metabox.title_template make sure the value is the same as your meta title. With the replacement variables unparsed.
  • Also add wpseoScriptData.metabox.title_template_no_fallback and make sure it is the same.
  • Edit the meta title and save. Make sure the wpseoScriptData.metabox.title_template is not changed.

Social:

  • Enable premium
  • Go to ?page=wpseo_page_settings#/post-type/posts
  • Look at the Social media appearance values and note them down.
  • create a new post/category and check that the default values are the same as in the settings.

First content image:

  • Add two images to the content and save.
  • Add wpseoScriptData.metabox.first_content_image to the content and make sure they url is the same as you first image.

Relevant test scenarios

  • Changes should be tested with the browser console open
  • Changes should be tested on different posts/pages/taxonomies/custom post types/custom taxonomies
  • Changes should be tested on different editors (Default Block/Gutenberg/Classic/Elementor/other)
  • Changes should be tested on different browsers
  • Changes should be tested on multisite

Test instructions for QA when the code is in the RC

  • QA should use the same steps as above.

QA can test this PR by following these steps:

Impact check

This PR affects the following parts of the plugin, which may require extra testing:

UI changes

  • This PR changes the UI in the plugin. I have added the 'UI change' label to this PR.

Other environments

  • This PR also affects Shopify. I have added a changelog entry starting with [shopify-seo], added test instructions for Shopify and attached the Shopify label to this PR.

Documentation

  • I have written documentation for this change. For example, comments in the Relevant technical choices, comments in the code, documentation on Confluence / shared Google Drive / Yoast developer portal, or other.

Quality assurance

  • I have tested this code to the best of my abilities.
  • During testing, I had activated all plugins that Yoast SEO provides integrations for.
  • I have added unit tests to verify the code works as intended.
  • If any part of the code is behind a feature flag, my test instructions also cover cases where the feature flag is switched off.
  • I have written this PR in accordance with my team's definition of done.
  • I have checked that the base branch is correctly set.

Innovation

  • No innovation project is applicable for this PR.
  • This PR falls under an innovation project. I have attached the innovation label.
  • I have added my hours to the WBSO document.

Fixes #

@thijsoo thijsoo added the changelog: non-user-facing Needs to be included in the 'Non-userfacing' category in the changelog label Apr 24, 2024
@thijsoo thijsoo linked an issue Apr 24, 2024 that may be closed by this pull request
@thijsoo thijsoo marked this pull request as ready for review April 25, 2024 10:43
@coveralls
Copy link

Pull Request Test Coverage Report for Build 6ebbfba6f3acb97e17911989012bcf63226e2de5

Details

  • 182 of 203 (89.66%) changed or added relevant lines in 18 files are covered.
  • 810 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+1.1%) to 53.487%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/editors/application/seo/post-seo-information-repository.php 8 10 80.0%
src/editors/framework/seo/posts/keywords-data-provider.php 14 16 87.5%
src/editors/framework/seo/posts/meta-title-data-provider.php 12 14 85.71%
src/editors/framework/seo/posts/meta-description-data-provider.php 11 14 78.57%
src/editors/framework/seo/terms/social-data-provider.php 21 24 87.5%
src/editors/framework/seo/posts/social-data-provider.php 15 24 62.5%
Files with Coverage Reduction New Missed Lines %
src/generated/container.php 810 0.16%
Totals Coverage Status
Change from base Build 0bc542ae2a401e45ef1640d3b77f8be2369045a3: 1.1%
Covered Lines: 29462
Relevant Lines: 55394

💛 - Coveralls

Copy link
Member

@igorschoester igorschoester left a comment

Choose a reason for hiding this comment

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

CR 🏗️

Looking nice! 🥳
Mostly naming questions and then some PHPStorm type declaration stuff
Missing test coverage though? Tests that you added also don't cover the constructors

Comment on lines 38 to 47
/**
* The term.
*
* @param WP_Post $post The post.
*
* @return void
*/
public function set_post( WP_Post $post ) {
$this->post = $post;
}
Copy link
Member

Choose a reason for hiding this comment

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

I was wondering why you would cache the post/term like this inside the repository.
Wouldn't it be nicer to just pass it to the get_seo_data method?

}

/**
* The term.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* The term.
* The post.

Copy link
Member

Choose a reason for hiding this comment

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

We call them keyphrases in the UI nowadays. Though I'm pretty sure we have more occurences of keyword in the code still. I do think we started moving towards the term keyphrase? Maybe ask team Lingo.

Of course influencing all the naming here 😅

Copy link
Member

Choose a reason for hiding this comment

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

In the decoupling issue, @vraja-pro named it SEO description or just description inside the SEO context here.
The meta part refers to the output tag on the frontend.

In other words, my vote would go to referring to description and title within this SEO domain context instead of prefixing them with meta-

Copy link
Member

Choose a reason for hiding this comment

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

Same keyword vs keyphrase comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog: non-user-facing Needs to be included in the 'Non-userfacing' category in the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor seo data provision to script data
3 participants