Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Address AT issues after domain registration #10101

Conversation

khaykov
Copy link
Member

@khaykov khaykov commented Jun 24, 2019

Fixes #10100

This PR can be reviewed right now, but merged after Flux-C counterpart is merged, and correct commit has is reflected in this branch.

Update:
After more testing with more than 50 domains, I realized that there is not 100% sure way to do AT after fresh domain registration without some sort of problem. Most common ones are:

  • Primary domain switches to "wpcomstaging".
  • AT get's stuck even on empty site.
  • AT errors out for some reason.

Additional issue: when you install a plugin during AT there is some sort of server-side delay, between completion of AT and when the plugin is marked as installed. This often times happen when you perform AT on freshly registered domains (

PR adds a delay/retry logic for domain registration and fetching plugin details.

In addition, this PR removes the "Congratulations" screen after the successful domain registration during AT. Instead, we immediately prompt used to start Automated Transfer after the domain is registered.

Sometimes domain is not ready for AT, so we notify used about this.
Sometimes everything works!

To test:

  1. On a Simple site without a Custom Domain and with a Domain Credit, perform an AT by installing a plugin and registering a domain.
  2. When domain is registered, notice that AT prompt is visible.
  3. Installing a plugin will either work and you will see an error message, that site is not yet ready.
  4. When AT is complete, notice that "INSTALL" button of plugin changes to "INSTALLED".

PS: viewDomainRegistrationActivity in ActivityLauncher is left for compatibility with develop branch.

Update release notes:

  • If there are user facing changes, I have added an item to RELEASE-NOTES.txt.

@automattic-ian
Copy link

A 10 second delay should be plenty for smaller simple sites with not much content, but please bear in mind there are some pretty common scenarios when a 10 second delay, may not be enough.

I've seen larger sites (ex those with thousands of posts/pages/media library items) take much longer, in some instances I have seen a delay of more than 2 minutes.

If possible I would recommend, adding a check to change the delay based on post count, and increase it a bit if the site is quite large.

@khaykov khaykov added this to the 12.7 ❄️ milestone Jun 25, 2019
@khaykov
Copy link
Member Author

khaykov commented Jun 25, 2019

Thanks, @automattic-ian ! That's a great insight. If time varies like this, I think I'll try some other approach instead of just a flat delay.

Removed "Congrats" screen after domain registration.
Start plugin installation imidiately after domain registration is complete.
@khaykov khaykov changed the title Add a 10 seconds delay between completion of AT and fetching of plugin directory. Address delayed plugin directory updated during AT on fresh domain Jun 26, 2019
@khaykov khaykov requested review from 0nko, nbradbury and planarvoid June 26, 2019 05:16
@khaykov khaykov changed the title Address delayed plugin directory updated during AT on fresh domain Address AT issues after domain registration Jun 26, 2019
override fun onCreate(savedInstanceState: Bundle?) {
super.onCreate(savedInstanceState)

setContentView(R.layout.activity_domain_suggestions_activity)

domainRegistrationPurpose = intent.getSerializableExtra(DOMAIN_REGISTRATION_PURPOSE_KEY)
as DomainRegistrationPurpose
Copy link
Contributor

@planarvoid planarvoid Jun 27, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is correct. This will work if the DOMAIN_REGISTRATION_PURPOSE_KEY is in the intent, if it isn't, it will try to cast null to DomainRegistrationPurpose and it will fail because DomainRegistrationPurpose is not nullable. I think we have to cast it to DomainRegistrationPurpose? or use as? DomainRegistrationPurpose. This assumption is based on https://kotlinlang.org/docs/reference/typecasts.html#unsafe-cast-operator

Do you really need to keep the org.wordpress.android.ui.ActivityLauncher#viewDomainRegistrationActivity? It is unused so the DomainRegistrationPurpose should always be there. You can change var domainRegistrationPurpose: DomainRegistrationPurpose? = null to lateinit var domainRegistrationPurpose: DomainRegistrationPurpose so it doesn't have to be nullable.

Copy link
Member Author

@khaykov khaykov Jun 27, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! I added a null guard using as? and left it nullable for now, since it will be called using viewDomainRegistrationActivity from develop branch, and I want to reduce conflicts when merging this back into it.

I'll change it to lateinit in next domain related PR 👍

import kotlin.coroutines.resume

@UseExperimental(InternalCoroutinesApi::class)
class NoDelayCoroutineDispatcher : CoroutineDispatcher(), Delay {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good idea 👍

@planarvoid planarvoid self-assigned this Jun 27, 2019
@khaykov
Copy link
Member Author

khaykov commented Jun 28, 2019

Changed error message and moved it into a dialog from toast 👍

Copy link
Contributor

@planarvoid planarvoid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good 👍 thanks for the changes 💯

khaykov added 2 commits June 28, 2019 07:39
…Android into issue/10100-accounting-for-server-side-delay-during-some-requests

# Conflicts:
#	build.gradle
@khaykov khaykov merged commit 2ee4b63 into release/12.7 Jun 28, 2019
@khaykov khaykov deleted the issue/10100-accounting-for-server-side-delay-during-some-requests branch June 28, 2019 14:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants