Fix an issue where the All Domains flow wasn't taking into account sites with existing plan #22343

salimbraksa · 2024-01-08T17:02:50Z

Description

This PR resolves an issue where users purchasing a domain, linked to a site that already has a plan, through the All Domains flow get stuck in the Plans flow.

Test Instructions

N.B: Sandboxing is required to test domain checkout

Case 1: Site with Existing Plan

Log into the site that already has a plan
Navigate to Me > All Domains
Tap "+"
Select a domain
Tap "Choose Site" button
Expect the Checkout web view to appear
Purchase the domain to verify the checkout flow works.

Case 2: Site without plan ( Regression )

Log into the site that doesn't have a plan
Navigate to Me > All Domains
Tap "+"
Select a domain
Tap "Get Domain" button
Expect the Plans web view to appear
Select a plan, go verify the checkout flow works as expected.

Case 3: Domain with No-Site ( Regression )

Log into the site that already has a plan
Navigate to Me > All Domains
Tap "+"
Select a domain
Tap "Get Domain" button
Expect the Checkout web view to appear
Purchase the domain to verify the checkout flow works.

Regression Notes

Potential unintended areas of impact
Purchasing a domain through the All Domains flow.
What I did to test those areas of impact (or what existing automated tests I relied on)
Manual testing.
What automated tests I added (or what prevented me from doing so)
None.

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.

wpmobilebot · 2024-01-08T17:11:06Z

📲 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	pr22343-b8b430f
	Version	24.0
	Bundle ID	org.wordpress.alpha
	Commit	`b8b430f`
	App Center Build	WPiOS - One-Offs #8551

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

wpmobilebot · 2024-01-08T17:11:31Z

📲 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	pr22343-b8b430f
	Version	24.0
	Bundle ID	com.jetpack.alpha
	Commit	`b8b430f`
	App Center Build	jetpack-installable-builds #7578

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

…ion sites with existing plan

alpavanoglu · 2024-01-11T12:42:11Z

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


    // MARK: Helpers

+    private func purchaseDomain(for site: Blog?, on viewController: UIViewController, completion: @escaping (Result<Void, Swift.Error>) -> Void) {


I think a VM would be a better fit for these service calls but it doesn't have to be part of this PR.

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

hassaanelgarem · 2024-01-17T04:31:15Z

@salimbraksa Can you briefly explain how these changes fixed the issue? 🙏

salimbraksa · 2024-01-17T12:07:04Z

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

        self.domainPurchasedCallback = domainPurchasedCallback
        self.crashLogger = crashLogger
        self.analyticsSource = analyticsSource
+        self.domainRegistrationService = domainRegistrationService


This service was instantiated within the createCart method, but I've moved to init for better dependency injection, allowing easier unit testing.

salimbraksa · 2024-01-17T12:07:18Z

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

    func handlePurchaseDomainOnly(on viewController: UIViewController,
                                  onSuccess: @escaping () -> (),
                                  onFailure: @escaping () -> ()) {
+        // TODO: Refactor `handlePurchaseDomainOnly` to use `purchaseDomain` helper.


Will be addressed in another PR.

salimbraksa · 2024-01-17T12:07:23Z

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

    func addDomainToCartLinkedToCurrentSite(on viewController: UIViewController,
                         onSuccess: @escaping () -> (),
                         onFailure: @escaping () -> ()) {
+        // TODO: Refactor `addDomainToCartLinkedToCurrentSite` to use `purchaseDomain` helper.


Will be addressed in another PR.

salimbraksa · 2024-01-17T12:09:22Z

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

+                    self.domainAddedToCartAndLinkedToSiteCallback?(viewController, domain.domainName, site)
+                } else {
+                    self.presentCheckoutWebview(on: viewController, title: TextContent.checkoutTitle)
+                }


This is exactly the change that fixes the issue:

If site.canRegisterDomainWithPaidPlan is true, then we show the Plans web view.

Else, the Checkout web view is shown.

cc @hassaanelgarem

Thanks for clarifying 🙏

@staskus Do you confirm this logic is correct? 🙏

I haven't checked the rest of the PR but if site.canRegisterDomainWithPaidPlan it means that the site has a domain credit, therefore we just allow to make domain registration without any plan selection or checkout:

@staskus Re-reading SiteDomainsViewModel and SiteDomainsView logic, I understand that:

The Register domain screen is shown when site.canRegisterDomainWithPaidPlan is true.

The Plans web view is shown when site.canRegisterDomainWithPaidPlan is false and user doesn't have domains.

The checkout web view is shown when site.canRegisterDomainWithPaidPlan is false and user has domains.

Yes, sounds right!

salimbraksa · 2024-01-17T14:11:45Z

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

    let analyticsSource: String

+    // TODO: This can cause Core Data crashes. Pass `NSManagedObjectID` instead.
    var site: Blog?


FYI @hassaanelgarem I'll address this in a separate PR.

salimbraksa · 2024-01-18T02:33:48Z

Following Povilas' comment, it appears these changes don't resolve the issue. I'm converting this PR to a Draft and deprioritizing it to focus on the In-App Feedback Prompt UI instead.

…rDomainCoordinator

…github.com:wordpress-mobile/WordPress-iOS into task/22268-all-domains-existing-site-with-plan-bug

salimbraksa self-assigned this Jan 8, 2024

salimbraksa added [Type] Bug Jetpack App Domains labels Jan 8, 2024

salimbraksa added this to the 24.1 milestone Jan 8, 2024

salimbraksa requested review from alpavanoglu and hassaanelgarem January 8, 2024 17:13

salimbraksa marked this pull request as ready for review January 8, 2024 17:13

salimbraksa changed the title ~~Fix an issue where the All Domains flow wasn't taking into consideration sites with existing plan~~ Fix an issue where the All Domains flow wasn't taking into account sites with existing plan Jan 8, 2024

Fix an issue where the All Domains flow wasn't taking into considerat…

beda8a0

…ion sites with existing plan

salimbraksa force-pushed the task/22268-all-domains-existing-site-with-plan-bug branch from 467aca8 to beda8a0 Compare January 8, 2024 18:09

alpavanoglu reviewed Jan 11, 2024

View reviewed changes

salimbraksa commented Jan 17, 2024

View reviewed changes

Add a TODO to replace site attribute with siteID

0ba85e7

salimbraksa commented Jan 17, 2024

View reviewed changes

salimbraksa added 2 commits January 17, 2024 15:39

Show Domain Registration screen for blogs that have a domain credit

1ddee63

Add documentation for the purchaseDomain method

8923eaf

salimbraksa modified the milestones: 24.1, Pending Jan 17, 2024

salimbraksa marked this pull request as draft January 17, 2024 15:50

hassaanelgarem removed their request for review January 19, 2024 00:38

salimbraksa added 2 commits January 25, 2024 18:40

Implement private method responsible for presenting the Plans flow

d5e405b

Implement private method responsible for presenting the Plans flow

3eb3edb

salimbraksa added 2 commits January 25, 2024 18:49

Remove domainAddedToCartAndLinkedToSiteCallback property from Registe…

6398d0a

…rDomainCoordinator

Merge branch 'task/22268-all-domains-existing-site-with-plan-bug' of …

b8b430f

…github.com:wordpress-mobile/WordPress-iOS into task/22268-all-domains-existing-site-with-plan-bug

jkmassel closed this Jun 26, 2024

jkmassel deleted the task/22268-all-domains-existing-site-with-plan-bug branch July 26, 2024 18:21


		// MARK: Helpers

		private func purchaseDomain(for site: Blog?, on viewController: UIViewController, completion: @escaping (Result<Void, Swift.Error>) -> Void) {

Fix an issue where the All Domains flow wasn't taking into account sites with existing plan #22343

Fix an issue where the All Domains flow wasn't taking into account sites with existing plan #22343

Uh oh!

Conversation

salimbraksa commented Jan 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Test Instructions

Case 1: Site with Existing Plan

Case 2: Site without plan ( Regression )

Case 3: Domain with No-Site ( Regression )

Next

Regression Notes

PR submission checklist:

Uh oh!

wpmobilebot commented Jan 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wpmobilebot commented Jan 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hassaanelgarem commented Jan 17, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

salimbraksa Jan 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

salimbraksa commented Jan 18, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

salimbraksa commented Jan 8, 2024 •

edited

Loading

wpmobilebot commented Jan 8, 2024 •

edited

Loading

wpmobilebot commented Jan 8, 2024 •

edited

Loading

salimbraksa Jan 17, 2024 •

edited

Loading