Domain Management: Implement add new freestanding domain flow #21979

hassaanelgarem · 2023-11-06T07:09:24Z

Closes #21778
Closes #21780
WPKit PR: wordpress-mobile/WordPressKit-iOS#644

Description

This PR adds the first steps for adding a domain which is searching for a new domain. It also integrates with the already implemented "Choice" screen and hooks up the freestanding domain purchase flow.

Recording

Simulator.Screen.Recording.-.iPhone.14.Pro.-.2023-11-06.at.08.30.18.mp4

Testing Instructions

Setup the store sandbox
Navigate to the "Me" tab
Tap on "My Domains"
Tap on the + button
✅ Make sure that the search domains screen is displayed as a modal
Search for a domain
✅ Make sure that the search works fine
Select a .live domain
Tap on "Select Domain"
✅ Make sure the purchase choice screen is displayed
Tap on "Get Domain"
✅ Make sure the checkout page is displayed (P.S: There is a delay here, and ideally we should display an activity indicator. Check Domain choice screen enhancements #21978 )
Go through the checkout flow and purchase the domain
✅ Make sure the modal is dismissed after purchasing is successful and the domains list is updated

Other than the above flow, please testing the existing domain purchasing flows for regressions:

My Site -> Domains
Dashboard -> Free domain with plan card
Site creation

Regression Notes

Potential unintended areas of impact
Other places where a domain can be purchased:
What I did to test those areas of impact (or what existing automated tests I relied on)
Testing manually
What automated tests I added (or what prevented me from doing so)
N/A

PR submission checklist:

I have completed the Regression Notes.
I have considered adding unit tests for my changes.
I have considered adding accessibility improvements for my changes.
I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

UI Changes testing checklist:

Portrait and landscape orientations.
Light and dark modes.
Fonts: Larger, smaller and bold text.
High contrast.
VoiceOver.
Languages with large words or with letters/accents not frequently used in English.
Right-to-left languages. (Even if translation isn’t complete, formatting should still respect the right-to-left layout)
iPhone and iPad.
Multi-tasking: Split view and Slide over. (iPad)

… siteID

…iteID

# Conflicts: # WordPress/WordPress.xcodeproj/project.pbxproj

…ViewController`

peril-wordpress-mobile · 2023-11-06T07:09:29Z

	Warnings
⚠️	PR has more than 500 lines of code changing. Consider splitting into smaller PRs if possible.

Generated by 🚫 dangerJS

wpmobilebot · 2023-11-06T07:32:42Z

📲 You can test the changes from this Pull Request in Jetpack Alpha by scanning the QR code below to install the corresponding build.

	App Name	Jetpack Alpha
	Configuration	Release-Alpha
	Build Number	pr21979-ac5cbec
	Version	23.6
	Bundle ID	com.jetpack.alpha
	Commit	`ac5cbec`
	App Center Build	jetpack-installable-builds #6772

Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

wpmobilebot · 2023-11-06T08:05:28Z

📲 You can test the changes from this Pull Request in WordPress Alpha by scanning the QR code below to install the corresponding build.

	App Name	WordPress Alpha
	Configuration	Release-Alpha
	Build Number	pr21979-ac5cbec
	Version	23.6
	Bundle ID	org.wordpress.alpha
	Commit	`ac5cbec`
	App Center Build	WPiOS - One-Offs #7755

Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

...elated/Domains/Domain registration/RegisterDomainSuggestions/RegisterDomainCoordinator.swift

salimbraksa · 2023-11-06T21:19:31Z

...elated/Domains/Domain registration/RegisterDomainSuggestions/RegisterDomainCoordinator.swift

+
+    // MARK: Public Functions
+
+    func createCart(_ domain: FullyQuotedDomainSuggestion,


❓ What's the difference between the passed-in domain param and the property self.domain?

Good question!
No difference, really. It makes sense to remove this param and depend on the property for consistency.
Resolved in 42c5b70

...elated/Domains/Domain registration/RegisterDomainSuggestions/RegisterDomainCoordinator.swift

salimbraksa · 2023-11-06T21:28:49Z

...elated/Domains/Domain registration/RegisterDomainSuggestions/RegisterDomainCoordinator.swift

+        let canOpenNewURL = newURL.absoluteString.starts(with: Constants.checkoutWebAddress)
+
+        guard canOpenNewURL else {
+            onCancel()


[Suggestion] There is a critical bug in this implementation. Many times when I was testing, the default account authentication would fail and the Log In page shows up instead of the Checkout page. And since the Log In url doesn't start with Constants.checkoutWebAddress, the web view is dismissed immediately.

I'm still not sure exactly why the webview authentication fails, but sometimes I could reproduce it when using the proxy ( and other times happens without proxy ). Here is the Sentry issue associated with this error.

I'm not sure exactly how to handle this edge case. In SiteCreationPurchasingWebFlowController, I haven't thought too much about this issue because the Domain Purchasing during Site Creation project was just an experiment.

Let me know what you think! 🙇

Interesting! Any idea how I can reproduce this?

@hassaanelgarem I couldn't figure out a way to consistently reproduce that issue, but I remember it happened to many times during development ( using proxy ) and a fewer times in production ( without proxy ).

What do you say we start by logging this event on Sentry or Tracks instead of silently dismissing the Checkout screen?

We can iterate on this later if we see that this happens very frequently.

Also, when logging this event, we should also include the URL that caused the web view to be dismissed.

As discussed on Slack, we suspect this to be a proxy issue. I am hesitant to add error tracking for this, as this is not technically an error. It's only an error if the user is redirected to log in, but in other cases, it is an expected behavior. What do you think?

salimbraksa · 2023-11-07T15:08:53Z

...elated/Domains/Domain registration/RegisterDomainSuggestions/RegisterDomainCoordinator.swift

+
+        let webViewController = WebViewControllerFactory.controllerWithDefaultAccountAndSecureInteraction(
+            url: url,
+            source: "domains_register", // TODO: Update source


Don't forget to follow up on this @hassaanelgarem 👍

I created this issue to track this: #21996 👍

salimbraksa · 2023-11-07T15:20:17Z

WordPress/Classes/ViewRelated/Me/All Domains/Coordinators/AllDomainsAddDomainCoordinator.swift

+
+        let domainPurchasedCallback = { (domainViewController: UIViewController, domainName: String) in
+            allDomainsViewController.reloadDomains()
+        }


[Suggestion] @hassaanelgarem This closure is capturing allDomainsViewController. I'm not sure if that's going to cause a memory leak or not, what do you think of adding [weak allDomainsViewController] to be safe?

Feel free to ignore this suggestion if you think this is not going to a cause a memory leak.

Thanks for raising this! I don't think it's needed because allDomainsViewController doesn't have a strong reference to this closure. I've confirmed this using the Memory graph 👍

salimbraksa · 2023-11-07T15:40:12Z

...elated/Domains/Domain registration/RegisterDomainSuggestions/RegisterDomainCoordinator.swift

+                               shouldPush: true)
+    }
+
+    func presentWebViewForCurrentSite(on viewController: UIViewController) {


❓ I'm curious if we should have a single presentCheckoutWebView method that handles site vs no-site logic based on whether self.site is nil or not. Because right now, it is programmatically possible to:

Call presentWebViewForCurrentSite even though self.site is nil. Which would result in an error.

Call presentWebViewForNoSite even though self.site is not nil. This wouldn't result in an error, but it might be confusing. We are presenting the Checkout web view for no-site even though the coordinator has a site?

I'm wondering whether it's possible to have an API design where it's programmatically impossible to misuse this coordinator.

Update
Please reply to this comment instead.

salimbraksa · 2023-11-07T16:02:54Z

...elated/Domains/Domain registration/RegisterDomainSuggestions/RegisterDomainCoordinator.swift

+
+    func handleExistingSiteChoice(on viewController: UIViewController) {
+        print("handleExistingSiteChoice")
+    }


Regarding my earlier comment, I just saw these handleNoSiteChoice and handleExistingSiteChoice. So basically the coordinator has 5 public methods:

createCart

presentWebViewForNoSite

presentWebViewForCurrentSite

handleNoSiteChoice

handleExistingSiteChoice

In my opinion, it is not clear when to use which, and I think there is room for improvements here, for example:

Cart creation is an implementation detail and should be called internally. Similar to what you did in handleNoSiteChoice. So I think createCart method should be private.

presentWebViewForNoSite and presentWebViewForCurrentSite can also be private and called within handleNoSiteChoice and handleExistingSiteChoice.

Maybe handleNoSiteChoice and handleExistingSiteChoice can be merged into a single public method and handle site vs no-site internally depending on nullability of self.site.

I'm not suggesting to make any changes right now, because that would break the existing "My Site > Domains" implementation. But I just wanted to hear you thoughts.

Thanks for taking the time to make these thoughtful suggestions!

I do agree with most of this. And I have tried to address these suggestions in #21998, which builds on this PR, specifically in af5cf72. I did this to avoid causing conflicts.

I didn't merge handleNoSiteChoice and handleExistingSiteChoice because I think it would complicate things. But I applied your other suggestions.

Let me know what you think! Either here or in #21998 🙇

I didn't merge handleNoSiteChoice and handleExistingSiteChoice because I think it would complicate things.

To clarify, there is what I meant by merging those 2 methods:

Mark handleNoSiteChoice and handleExistingSiteChoice as private.

Add a new public method that decides whether to call handleExistingSiteChoice or handleNoSiteChoice based on self.site

func handleDomainPurchasing() { if let site { handleExistingSiteChoice(site) } else { handleNoSiteChoice() } } private func handleNoSiteChoice() { ... } private func handleExistingSiteChoice(_ site: Blog) { ... }

But I don't have a strong opinion on this, so feel free to decide whether you want make the change.

salimbraksa · 2023-11-07T16:26:09Z

@hassaanelgarem I've dropped a few more comments but the feature works as described in the test instructions. 👍

# Conflicts: # Podfile.lock

wpmobilebot · 2023-11-08T12:15:33Z

	1 Warning
⚠️	This PR is larger than 500 lines of changes. Please consider splitting it into smaller PRs for easier and faster reviews.

Generated by 🚫 Danger

salimbraksa

LGTM!

hassaanelgarem added 19 commits November 3, 2023 02:11

Update: correctly hook up add domain button to common function

44c2d94

Update: refactor domain suggestions to handle not passing a site

6cefb09

Update: introduce new domain selection type

195d449

Update: refactor RegisterDomainDetailsServiceProxy to accept nil as…

70d35a0

… siteID

Update: refactor RegisterDomainSuggestionsVC's helpers to handle no s…

6bcb7b9

…iteID

Add: Basic implementation of AllDomainsAddDomainCoordinator

d77f52d

Update: add public function to reload domains list after purchase

7144701

Update: add ability to customize title of suggestions view

cf8bf05

Update: Add support for custom title in checkout

536cdcd

Update: change string key to use convention

5d7359a

Add: helper func for starting checkout for no site domains

6b63115

Merge branch 'trunk' into task/21778-search-for-new-domain

dde989e

# Conflicts: # WordPress/WordPress.xcodeproj/project.pbxproj

Fix typo

5d17c8b

Refactor: introduce coordinator logic into `RegisterDomainSuggestions…

73095a4

…ViewController`

Update: move RegisterDomainCoordinator to its own file

085756f

Update: Move callbacks to coordinator

2534170

Update: hookup new flow to all domains screen

5d81e1f

Update: integrate right version of WPKit

61ae067

Refactor: remove duplicate code

d5ba657

hassaanelgarem added [Type] Task Jetpack App Domains labels Nov 6, 2023

hassaanelgarem added this to the 23.7 milestone Nov 6, 2023

hassaanelgarem requested review from salimbraksa and alpavanoglu November 6, 2023 07:09

hassaanelgarem self-assigned this Nov 6, 2023

Fix: lint errors

8d61f2c

hassaanelgarem mentioned this pull request Nov 6, 2023

Domain choice screen enhancements #21978

Closed

5 tasks

Update: fix build error

422c787

Tests: fix build error

012caac

hassaanelgarem mentioned this pull request Nov 6, 2023

[Domain Management] Domain Details web view #21964

Merged

4 tasks

salimbraksa reviewed Nov 6, 2023

View reviewed changes

...elated/Domains/Domain registration/RegisterDomainSuggestions/RegisterDomainCoordinator.swift Show resolved Hide resolved

salimbraksa reviewed Nov 6, 2023

View reviewed changes

...elated/Domains/Domain registration/RegisterDomainSuggestions/RegisterDomainCoordinator.swift Show resolved Hide resolved

salimbraksa reviewed Nov 6, 2023

View reviewed changes

hassaanelgarem added 4 commits November 7, 2023 01:32

Update: remove redundant domain params inside coordinator

42c5b70

Update: track error messages

243562a

Update: point to latest WPKit commit

0dc2c8a

Update: point WPKit to trunk

a580351

salimbraksa reviewed Nov 7, 2023

View reviewed changes

hassaanelgarem requested a review from salimbraksa November 8, 2023 01:10

hassaanelgarem added 2 commits November 8, 2023 14:09

Merge branch 'trunk' into task/21778-search-for-new-domain

b7af8bb

# Conflicts: # Podfile.lock

Update Podfile.lock

ac5cbec

salimbraksa approved these changes Nov 8, 2023

View reviewed changes

hassaanelgarem merged commit 0c8b031 into trunk Nov 8, 2023
23 checks passed

hassaanelgarem deleted the task/21778-search-for-new-domain branch November 8, 2023 16:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Domain Management: Implement add new freestanding domain flow #21979

Domain Management: Implement add new freestanding domain flow #21979

hassaanelgarem commented Nov 6, 2023

peril-wordpress-mobile bot commented Nov 6, 2023 •

edited

Loading

wpmobilebot commented Nov 6, 2023 •

edited

Loading

wpmobilebot commented Nov 6, 2023 •

edited

Loading

salimbraksa Nov 6, 2023

hassaanelgarem Nov 6, 2023

salimbraksa Nov 6, 2023 •

edited

Loading

hassaanelgarem Nov 6, 2023

salimbraksa Nov 7, 2023

hassaanelgarem Nov 7, 2023

salimbraksa Nov 7, 2023

hassaanelgarem Nov 7, 2023

salimbraksa Nov 7, 2023

hassaanelgarem Nov 7, 2023

salimbraksa Nov 7, 2023 •

edited

Loading

salimbraksa Nov 7, 2023 •

edited

Loading

hassaanelgarem Nov 8, 2023

salimbraksa Nov 8, 2023

salimbraksa commented Nov 7, 2023

wpmobilebot commented Nov 8, 2023

salimbraksa left a comment


		// MARK: Public Functions

		func createCart(_ domain: FullyQuotedDomainSuggestion,

Domain Management: Implement add new freestanding domain flow #21979

Domain Management: Implement add new freestanding domain flow #21979

Conversation

hassaanelgarem commented Nov 6, 2023

Description

Recording

Testing Instructions

Regression Notes

peril-wordpress-mobile bot commented Nov 6, 2023 • edited Loading

wpmobilebot commented Nov 6, 2023 • edited Loading

wpmobilebot commented Nov 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

salimbraksa Nov 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

salimbraksa Nov 7, 2023 • edited Loading

Choose a reason for hiding this comment

salimbraksa Nov 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

salimbraksa commented Nov 7, 2023

wpmobilebot commented Nov 8, 2023

salimbraksa left a comment

Choose a reason for hiding this comment

peril-wordpress-mobile bot commented Nov 6, 2023 •

edited

Loading

wpmobilebot commented Nov 6, 2023 •

edited

Loading

wpmobilebot commented Nov 6, 2023 •

edited

Loading

salimbraksa Nov 6, 2023 •

edited

Loading

salimbraksa Nov 7, 2023 •

edited

Loading

salimbraksa Nov 7, 2023 •

edited

Loading