4475 fix related keyphrase status #21941
Conversation
Pull Request Test Coverage Report for Build bde29a854458a44f50df09b5ee9194caa3447402Details
💛 - Coveralls |
mykola
added
changelog: non-user-facing
There was a problem hiding this comment.
Nice work, but please do take the opportunity to refactor and reduce this code further now that we are touching it 👍
... otherwise we would need to do it anyway as part of resolving technical debt later on 😅
In terms of solving the bug, this PR solves the case for related keyphrase analysis, but specifically for the SEO analysis in collections in Shopify, we decided we would use the same aggregation strategy. So, we can add a custom score aggregator for those assessors (packages/yoastseo/src/scoring/assessors/collectionPages/seoAssessor.js and packages/yoastseo/src/scoring/assessors/collectionPages/cornerstone/seoAssessor.js) as well 👍
| @@ -1157,16 +1149,15 @@ export default class AnalysisWebWorker { | |||
| * @param {Paper} paper The paper to analyze. | |||
| * @param {module:parsedPaper/structure.Node} tree The tree to analyze. | |||
| * | |||
| * @param {Object} analysisCombination Which assessors and score aggregator to use. | |||
| * @param {Object} analysisCombination Which assessors to use. | |||
There was a problem hiding this comment.
It would be great if we take this opportunity to remove the TreeAssessor references here. So, instead of an analysisCombination, just have assessor as the argument of this function.
Also, the fields that are created in setupTreeAnalysis (e.g., _contentTreeAssessor, _seoTreeAssessor, _relatedKeywordTreeAssessor) can be removed, along with all commented code.
| @@ -1243,11 +1234,10 @@ export default class AnalysisWebWorker { | |||
| synonyms: this._relatedKeywords[ key ].synonyms, | |||
| } ); | |||
|
|
|||
| // Which combination of (tree) assessors and score aggregator to use. | |||
| // Which combination of (tree) assessors to use. | |||
There was a problem hiding this comment.
... and we can also call the this.assess function with just a Paper and an Assessor.
| // Set the locale (we are more lenient for languages that have full analysis support). | ||
| analysisCombination.scoreAggregator.setLocale( this._configuration.locale ); | ||
| analysisCombination.oldAssessor.getScoreAggregator().setLocale( this._configuration.locale ); |
There was a problem hiding this comment.
It's not nice that this function is only available to the ReadabilityScoreAggregator. I would consider moving it to the abstract base class.
Context
Summary
This PR can be summarized in the following changelog entry:
Relevant technical choices:
RelatedKeywordScoreAggregatorwas added before the aggregate filter assessments without a score. A similar approach is used inReadabilityScoreAggregator. Also score aggregator is introduced as a field for Accessor classes By default is SEOScoreAggregator and redefined for RelatedKeywordAssessor and ContentAssessorTest instructions
Test instructions for the acceptance test before the PR gets merged
This PR can be acceptance tested by following these steps:
Relevant test scenarios
Test instructions for QA when the code is in the RC
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
Other environments
[shopify-seo], added test instructions for Shopify and attached theShopifylabel to this PR.Documentation
Quality assurance
Innovation
innovationlabel.Fixes #4475