Domain: Refactor to use a common domain selection view #22254
Conversation
Dashboard Card is used instead
Support all the previous tracking events for an older VC and new track events for new VC. It preserves the previous behavior.
All the functionality is now part of DomainSelectionViewController
| @@ -97,12 +115,46 @@ final class WebAddressWizardContent: CollapsableHeaderViewController { | |||
| } | |||
| } | |||
|
|
|||
| // MARK: - Transfer Footer Views | |||
There was a problem hiding this comment.
Copied from RegisterDomainSuggestionsViewController
| self?.displayActionableNotice(title: Strings.errorTitle, actionTitle: Strings.errorDismiss) | ||
| } | ||
|
|
||
| switch domainSelectionType { |
There was a problem hiding this comment.
Copied from RegisterDomainSuggestionsViewController
| @@ -715,3 +844,198 @@ extension WebAddressWizardContent: UITableViewDelegate { | |||
| performSearchIfNeeded(query: retryQuery) | |||
| } | |||
| } | |||
|
|
|||
| // MARK: - Transfer Footer Setup | |||
There was a problem hiding this comment.
Copied from RegisterDomainSuggestionsViewController
| // MARK: - Support | ||
|
|
||
| private extension DomainSelectionViewController { | ||
| func includeSupportButtonIfNeeded() { |
There was a problem hiding this comment.
Copied from RegisterDomainSuggestionsViewController
| // MARK: - Routing | ||
|
|
||
| private extension DomainSelectionViewController { | ||
| private func pushRegisterDomainDetailsViewController() { |
There was a problem hiding this comment.
Copied from RegisterDomainSuggestionsViewController
| } | ||
| } | ||
|
|
||
| func track(_ event: WPAnalyticsEvent, properties: [AnyHashable: Any] = [:], blog: Blog? = nil) { |
There was a problem hiding this comment.
Copied from RegisterDomainSuggestionsViewController
|
| App Name |  WordPress Alpha |
|
| Configuration | Release-Alpha | |
| Build Number | pr22254-1b8ecaf | |
| Version | 23.9 | |
| Bundle ID | org.wordpress.alpha | |
| Commit | 1b8ecaf | |
| App Center Build | WPiOS - One-Offs #8209 |
|
| App Name |  Jetpack Alpha |
|
| Configuration | Release-Alpha | |
| Build Number | pr22254-1b8ecaf | |
| Version | 23.9 | |
| Bundle ID | com.jetpack.alpha | |
| Commit | 1b8ecaf | |
| App Center Build | jetpack-installable-builds #7232 |
SafeArea gets added twice both from SwiftUI and UIKit side if not ignored
Generated by 🚫 dangerJS |
DomainSelectionViewController UI supports both configurations even if feature flag is disabled. It's no longer needed.
There was a problem hiding this comment.
Hey @staskus, thanks for tackling this refactor! 🏅 I took a first pass at reviewing and focused on testing today without looking much at the code. I split testing into two scenarios:
- user on the Free plan
- user on paid annual plans who already have a domain
I haven't tested users on the Free plan who already have a domain, users on paid plans who haven't claimed their free domain for a year yet, or users on monthly paid plans (and so are not eligible for a free domain). I also haven't finished a checkout to actually confirm the whole flow yet.
Free plan scenarios
I created a new account and a new site
- Site creation with free WP.com subdomain
I tested these flows up until the checkout:
- Free to Paid Dashboard Card
- Register Dashboard Card
- All Domains (Just buy a domain)
- All Domains (Existing WP.com site)
- Site Domains
Register Dashboard Card
Is this referring to this dashboard card (pcdRpT-21K-p2) we implemented to sell domains without a plan? If so, since it was removed in #21597, so I'm not sure why it's in the list of entry points (but I might be misunderstanding this).
[not a blocker, possibly out of scope] Users in the Dashboard -> More -> Domains flow are offered a domain for their existing site in this scenario. Should we only offer domains with a paid plan purchase? The user has a site, so matching the above "Existing WordPress.com site" where a domain is offered with a plan (which brings benefits to their site) seems intuitive.
Paid plan scenarios
Note that here the user has a paid domain, so additional domains are either re-directs or not associated with any site.
I tested these flows up until the checkout:
- Free to Paid Dashboard Card (not applicable)
- Register Dashboard Card (not sure if applicable; see above)
- All Domains (Just buy a domain): The checkout screen shows an odd error message: "I tried and failed to create products from signup"
- All Domains (Existing WP.com site): The checkout screen re-directs to the WP.com login screen
- Site Domains: The checkout screen opens briefly and closes
 |
 |
|---|
…-new-domain-selection
|
Thanks for the review!
I meant this card, sorry for not clarifying. It's shown when the user has a plan but hasn't claimed the domain. 
Mmm.. Strange. It works for me. Were you proxied? This refactoring only tackles the domain selection view. I didn't change any other flows so it shouldn't be affected.
If I understand the question correctly, this is what we're doing with #22261 project. Offering domains with a paid plan purchase, not only domains with redirects? |
Free plan who already have a domainSimulator.Screen.Recording.-.iPhone.15.Plus.-.2023-12-21.at.11.00.38.mp4As I understand one of your previous questions was regarding this flow, when the user already has a domain. I agree we should always offer a chance to purchase a plan for such a user. This refactor is not changing any behavior previously developed. And #22261 is adding another entry point. Users on paid plans who haven't claimed their free domain for a year yetSimulator.Screen.Recording.-.iPhone.15.-.2023-12-21.at.11.14.41.mp4claim.your.free.domain.mp4I also noticed that "All Domains" flow is underdeveloped for this case, however, it's not related to any changes we're making at the moment: Users on monthly paid plans (and so are not eligible for a free domain)Thanks, I never tested this flow before now. For such users, we don't show any cards, because they have a plan but are not eligible for domains as you say. I wonder if there's a way for us to prompt such users to switch from month plan to year plan and get a free domain 🤔 Simulator.Screen.Recording.-.iPhone.15.-.2023-12-21.at.13.10.46.mp4 |
|
I fixed additional cases I caught now when testing. Not directly related to changes I made in this PR but necessary to make flows work better:
|
Generated by 🚫 Danger |
|
@guarani, yes the plugins flow is as you described. It works. Simulator.Screen.Recording.-.iPhone.15.-.2023-12-21.at.17.17.10.mp4 |
|
Thanks for the replies @staskus! It seems to be working as expected. I reviewed the code as best I could, the changes are pretty big, so I tried to track most flows through the code instead of looking at all the changes one by one. Register Dashboard Card
It makes total sense now. I'd forgotten about that card! I tested the flow to the "Register domain" screen, and it worked as expected.
You're right, I got mixed up there. All Domains
Site DomainsThis is working as expected. Other scenarios
I probably was proxied, but I re-tested proxied and un-proxied today, and it works now. I don't know what the issue was.
I think this should be possible and upgrade credit will be applied, so perhaps we only would need to add the prompt. It's not in scope here, though. |
staskus
deleted the
task/replace-outdated-domain-selection-with-new-domain-selection
branch
Fixes #22246
Recently we created a new modern domain selection for Site Creation flow, but we continued using the previously created domain selection in all the other flows. The views not only differ in UI but also in how many domains we fetch and how we sort them.
The goal of this PR is to have a single implementation of domain selection for all the flows.
Solution
I first identified different entry points and their requirements:
Then turned
WebAddressWizardContentintoDomainSelectionViewControllerand started moving functionality one by one fromRegisterDomainSuggestionsViewControllertoDomainSelectionViewControllerso it would support all the cases for the site creation as well as all the previous functionality. I didn't want to change anything outside domain selection view controllers and keep everything else working in same way.In the end
DomainSelectionViewControllergrew and now includes the business logic within it. One step I haven't done is move awayswitchstatements into different configuration files using composition. For now, I feel it's more convenient having all the logic in one place.Testing
Domain.Selection.flow.mov
Regression Notes
Breaking domain selection in all the previous flows.
Manual testing
PR submission checklist:
RELEASE-NOTES.txtif necessary.UI Changes testing checklist: