Update RRM SettingsEdit to handle missing ID error. #10222
Conversation
|
Build files for d65092d are ready:
|
There was a problem hiding this comment.
Thanks @juniovitorino, this is looking good, but could use a couple of tweaks. Please see my comments.
Additionally, it would be good to provide a bit more detail to the QA Brief - specifically, how we can reproduce the error in E2E testing. Please feel free to drop me a line if you could use some help with this.
assets/js/modules/reader-revenue-manager/components/settings/SettingsEdit.js
Outdated
Show resolved
Hide resolved
assets/js/modules/reader-revenue-manager/components/settings/SettingsEdit.stories.js
Outdated
Show resolved
Hide resolved
assets/js/modules/reader-revenue-manager/components/settings/SettingsEdit.js
Outdated
Show resolved
Hide resolved
assets/js/modules/reader-revenue-manager/components/settings/SettingsEdit.js
Outdated
Show resolved
Hide resolved
@techanvil I chose to go with your recommendations; they all seemed reasonable to me. Since you know the codebase much better than I do, I’ll learn from your suggestions. Also, could you share an example of a QA Brief to give me a better idea of how to write one? |
juniovitorino
force-pushed
the
enhancement/10131-handle-error-if-the-saved-product-id-is-no-longer-available
branch
from
8a1a319 to
1277172
Compare
assets/js/modules/reader-revenue-manager/components/settings/SettingsEdit.js
Outdated
Show resolved
Hide resolved
assets/js/modules/reader-revenue-manager/components/settings/SettingsEdit.stories.js
Outdated
Show resolved
Hide resolved
assets/js/modules/reader-revenue-manager/components/settings/SettingsEdit.stories.js
Outdated
Show resolved
Hide resolved
Thanks @juniovitorino! I've left a couple of additional comments, as you've probably noticed. For example QA Briefs, pretty much all issues which have progressed to Code Review and beyond should have one, so you can browse issues in GitHub/ZenHub to get a broad range of examples to help understand how we write them. For a more focused set of examples I'd suggest browsing issues in the RRM Phase 2 epic. And, the most relevant issues to this one are #10065, #9952 and #9954. In fact, #10065 is the issue which actually integrates the ability for the user to set the However we are often in similar situations, so we can use JS snippets to help model scenarios to short-circuit the testing process. In this particular case, I would suggest that the QAB should take the following approach:
googlesitekit.data.select('modules/reader-revenue-manager').getProductIDs();
googlesitekit.data.dispatch('modules/reader-revenue-manager').setProductID( /* Use a product ID from the actual list output from the command above */ "CAownIG4DA:basic-product-id");
googlesitekit.data.dispatch('modules/reader-revenue-manager').saveSettings();
googlesitekit.data.dispatch('modules/reader-revenue-manager').setProductID( /* Use a product ID that doesn't exist in the list output above */ "CAownIG4DA:some-non-existent-product-id");
googlesitekit.data.dispatch('modules/reader-revenue-manager').saveSettings();
It would of course be preferable to test this in a fully integrated manner, which would involve the user setting the I hope that helps, please let me know if you have any further questions. I'm not sure how much exposure you've had to creating publications in the Publisher Center, if you've not done this yet the RRM Phase 1 bug bash instructions has got some useful info on it, although it doesn't get into setting the product IDs which is what Phase 2 tackles. You can add product IDs to a paywall or contribution CTA at the point of creating the subscription plan or contribution amounts. Again, please drop me a line if you want to discuss any of this further, I'd be happy to jump on a call if that would help. |
|
Size Change: +115 B (+0.01%) Total Size: 2.08 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Hey @juniovitorino, please note the VRT suite is currently failing in CI for this PR.
In fact so is the E2E suite, but these are known flaky test failures so you don't need to worry about them for this PR.
You can see which tests are currently known to be flaky by searching for "flaky" in ZenHub for example:
Back to the VRT suite - most of the failures should be fixed by updating the product IDs in the story file as per my other comment.
However, at a minimum you'll need to add the VRT reference images for the new scenario you added for the MissingProductID story.
I don't know if you've seen it yet, but you can download the VRT report for the failing run from the Summary page for the run on GitHub:
Unzip and view the HTML report and you can filter to just show the failed tests.
You'll want to add the new images (or update where relevant, probably not for this PR but in future). You can do this by generating them locally with npm run test:visualtest and then npm run test:visualapprove to copy them into tests/backstop/reference. Or, a shortcut can be to copy relevant images directly from the downloaded report into tests/backstop/reference, that's what I often do.
There's more info on the VRT suite in our documentation, please drop me a line if you've any questions!
There was a problem hiding this comment.
Please update the remaining product IDs in this file :)
The calls to setProductId() need updating, and so does the MissingProductID story.
|
@techanvil Thanks so much for the comprehensive explanation about VisualTest. I’ve finished the requested changes and checked the failing VRT again. It’s failing in other modules, and I don’t think my changes are related to these failures, but if they are, please let me know. |
juniovitorino
force-pushed
the
enhancement/10131-handle-error-if-the-saved-product-id-is-no-longer-available
branch
from
6300a52 to
c782cbb
Compare
There was a problem hiding this comment.
Hi @juniovitorino, thanks for the update. However, it looks like you missed a couple of details from my previous review.
Essentially, I would not expect the changes in this PR to affect existing stories in Storybook. As things stand it's added the missing product ID error to a number of additional stories.
As per these points in my review:
Back to the VRT suite - most of the failures should be fixed by updating the product IDs in the story file as per my other comment.
The calls to
setProductId()need updating, and so does theMissingProductIDstory.
You need to update the calls to setProductID() as well as setProductIDs() so the selected product ID matches one of the provided range.
Once this is done, you should find the existing stories are no longer affected and you can revert the changes to their VRT images.
Incidentally, I'm not sure if you have seen this but Storybook is published as GitHub pages, this is useful for quickly referring back to the different versions:
develop: https://google.github.io/site-kit-wp/storybook/develop/?path=/story/accountchooser--account-choosermain: https://google.github.io/site-kit-wp/storybook/main/?path=/story/accountchooser--account-chooser- PR (removed when the PR is merged): https://google.github.io/site-kit-wp/storybook/pull/10222/?path=/story/accountchooser--account-chooser
As to the VRTs which are still failing, these are indeed unrelated to this PR; it looks like they should be fixed now on develop.
| @@ -65,6 +75,11 @@ PendingVerification.args = { | |||
| registry | |||
| .dispatch( MODULES_READER_REVENUE_MANAGER ) | |||
| .selectPublication( publications[ 1 ] ); | |||
|
|
|||
| registry.dispatch( MODULES_READER_REVENUE_MANAGER ).setProductID( 1 ); | |||
There was a problem hiding this comment.
Please update setProductID() here to provide a product ID matching one that is provided to setProductIDs() below, and apply this to the other stories in the file where relevant.
…ettingsEdit.js Co-authored-by: Tom Rees-Herdman <[email protected]>
…ettingsEdit.stories.js Co-authored-by: Tom Rees-Herdman <[email protected]>
…ettingsEdit.js Co-authored-by: Tom Rees-Herdman <[email protected]>
…ettingsEdit.js Co-authored-by: Tom Rees-Herdman <[email protected]>
juniovitorino
force-pushed
the
enhancement/10131-handle-error-if-the-saved-product-id-is-no-longer-available
branch
from
c782cbb to
6d54e5f
Compare
juniovitorino
force-pushed
the
enhancement/10131-handle-error-if-the-saved-product-id-is-no-longer-available
branch
from
6d54e5f to
a3f5b8c
Compare
There was a problem hiding this comment.
Hey @juniovitorino, thanks for updating the PR, however it's still not quite there.
- There are still a number of calls to
setProductID()with a number rather than a string. - You've updated the stories for the Sign in With Google
SettingsEditcomponent, restoring the incorrect versions with Portuguese copy for the button label. I'm not sure how this has happened, was it a Git error, or do you actually see Portuguese copy for it when you view Storybook? - A minor point, but I notice you force pushed to this branch. In future, please try to avoid this once a PR for the branch has started to go through code review, as it can make it harder to track changes since the last review.
Summary
Addresses issue:
Relevant technical choices
PR Author Checklist
Do not alter or remove anything below. The following sections will be managed by moderators only.
Code Reviewer Checklist
Merge Reviewer Checklist