Add post-purchase onboardings to Recommendation flows
- Add initial onboarding mechanisms
- changelog
Fixes #
Changes proposed in this Pull Request:
Other information:
- [ ] Have you written new tests for your changes, if applicable?
- [ ] Have you checked the E2E test CI results, and verified that your changes do not break them?
Jetpack product discussion
Does this pull request change what data or activity we track or use?
Testing instructions:
- Go to '..'
Are you an Automattician? You can now test your Pull Request on WordPress.com. On your sandbox, run bin/jetpack-downloader test jetpack add/post-purchase-recommendations-onboardings to get started. More details: p9dueE-5Nn-p2
Thank you for your PR!
When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
- :white_check_mark: Include a description of your PR changes.
- :white_check_mark: All commits were linted before commit.
- :white_check_mark: Add a "[Status]" label (In Progress, Needs Team Review, ...).
- :white_check_mark: Add testing instructions.
- :white_check_mark: Specify whether this PR includes any changes to data or privacy.
- :white_check_mark: Add changelog entries to affected projects
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation :robot:
The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available.
Once your PR is ready for review, check one last time that all required checks (other than "Required review") appearing at the bottom of this PR are passing or skipped. Then, add the "[Status] Needs Team review" label and ask someone from your team review the code. Once you’ve done so, switch to the "[Status] Needs Review" label; someone from Jetpack Crew will then review this PR and merge it to be included in the next Jetpack release.
Jetpack plugin:
- Next scheduled release: November 1, 2022.
- Scheduled code freeze: October 25, 2022.
I still plan to write unit tests supporting the new recommendations code to ensure it works as expected and does not break any existing functionalities. However, it should not prevent anyone from starting to review the PR!
But when you buy several single plans (ex. backup and then anti-spam) it only shows the spam onboarding in the summary. Would it be possible/preferable to show both backup and spam at the same time if you have both?
Thanks for pointing that out, I'll confirm it with design, but it indeed makes sense.
My last comment is that next time we have a change this big, it might be better to add the framework for the new functionality in one PR (i.e. adding the ability to add onboarding steps) and separate each new onboarding plan as a separate PR. I think that would make it a bit easier to review and look at in the future instead of one giant PR 😄
Yes, sorry for that. I'm actually torn between approaches to situations like this. Because on one hand, that way it's not possible to functionally test the mechanisms, but on the other - we end up with lots of code to view at once. Probably it would be great to leave a working MVP (based on single onboarding) as one PR, and add more onboardings as a separate one.
As a side note, I tried to still structure commits in an atomic way. You should be able to see MVP code by only selecting these 9 commits. And then the further commits focus on adding the next onboardings.
I agree it's not a perfect solution and will improve the PR structure in the next projects.
Yes, sorry for that.
No worries, I totally see the reasoning for doing it this way
Probably it would be great to leave a working MVP (based on single onboarding) as one PR, and add more onboardings as a separate one.
I think that would be a great solution
Hey @CodeyGuyDylan! Thanks again for the review. I've addressed all your feedback and I'll be adding unit tests to these changes, as a separate PR based on this one - not to further complicate the review process.
@CodeyGuyDylan thanks again for taking the time to provide me feedback on the code! I've addressed your comments and made the needed improvements. I hope everything works as expected now.
Thanks for checking out the PR @coder-karen! I've addressed all the unpolishments you've spotted.
@coder-karen thanks for taking a second look at the PR!
It looks like plan recommendations don't necessarily re-start when re-adding a plan, and they perhaps default to recommendations for other plans?
Thanks for explaining the steps you've taken. What was happening is that the code did not support activating new onboarding when there was any other one already active. Indeed, we want to show to the user the most recent onboarding, without waiting for them to finish the previous one - I've provided the change to allow for that now. It wasn't an issue, just an overseen case - thanks for spotting that!
Note that, after removing the plan, you need to access the Jetpack dashboard at least once before purchasing it again, in order to sync viewed onboardings. I don't think it is much of an issue, as if the user re-purchases the package so quickly, they probably do not need to see the onboarding again.