Issue/9739 domain contact form refactoring #9892

khaykov · 2019-05-21T07:52:48Z

This PR is a refactoring of a domain contact form.

I extracted the majority of the logic into ViewModel and added tests.
It's my first time working on ViewModel this big so I would appreciate insight on.. anything, really!
I have been staring on this code for too long, so there is a good chance I missed something.

The PR is giant, and I'm not really sure how to make it smaller since there is a lot of things going behind the scene. Good thing half of it are tests!

To test:

Install the wasabi build.

Switch to a site on a business plan with unclaimed domain credit (let me know if you need help with this).
Navigate to Plugins screen, and from there to details of any plugin (like Woocommerce).
Tap on Install button.
In the dialog tap on "REGISTER DOMAIN".
Pick a domain (preferably .blog).
Tap on "CHOOSE DOMAIN" and navigate to contact form details.
If you had contact information stored at WordPress.com it should be prefilled.
All the fields apart from State and Organisation are required, and their emptiness is validated locally.
The validity of input fields content is validated on backend one by one (eg. if you have two fields with malformed you will have to make two registration attempts to get errors for both of them).
Make sure ToS link is clickable.
Every time you change the country States are loaded from remote (some countries, like Germany, do not have states).
Try to register the domain.
PS: Follow domain testing guideline and make sure if you purchase a domain to cancel it.

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

planarvoid

Great job on the refactoring! 💯 I think it makes it so much more readable! Consider my suggestions and let me know what you think 👍

WordPress/src/main/java/org/wordpress/android/ui/domains/DomainRegistrationDetailsFragment.kt

planarvoid · 2019-05-23T12:51:19Z

WordPress/src/main/java/org/wordpress/android/ui/domains/DomainRegistrationDetailsFragment.kt

-                hideFormProgressIndicator()
-            }
+    private fun setupObservers() {
+        viewModel.formProgressIndicatorVisible.observe(this,


I'd consider having all the UI state fields (not the events/dialogs) in one object. I think when you have this many fields, it might be more readable to have one object (UiModel?). It's easier to test because some of the fields might depend on each other and one object fully represents all the possible states of the UI (for example you can have a logic while building the UiModel that checks that only one progress indicator is visible). I'm not saying that there is anything wrong with your approach though 👍. Let me know what you think. Posting a change can be done like liveData.postValue(liveData.value.copy(updatedField = newValue)).

This makes sense! I tried this approach, and when updating tests I realized that I'm not really sure how to test the sequence of updates to UI state. For example, currently I can test that progress indicator was shown, then dismissed, and is currently invisible like this:

verify(formProgressIndicatorObserver, times(1)).onChanged(true) verify(formProgressIndicatorObserver, times(1)).onChanged(false) Assertions.assertThat(viewModel.formProgressIndicatorVisible.value).isEqualTo(false)

Do you know how to achieve similar coverage using UI state object (or is it even necessary?)

If your UI state object is a data class, you can do something like verify(observer, times(1)).onChanged(stateObject.copy(progressIndicator = true) (the times(1) is not required btw). You have to build the complete UI State object with all the fields though.

What I usually do when I want to test a sequence of objects is that I do something like

val results = mutableListOf<UiState>() viewModel.uiState.observeForever { if (it != null) results.add(it) }

This way the results contain all the emitted UI states and you can assert anything you want.

Awesome, thank you! I took your approach (storing observer results) and it's way better - lets us test a snapshot of whole UI state at each step.

yep, that's exactly what I thought we should be doing :-)

WordPress/src/main/java/org/wordpress/android/ui/domains/DomainRegistrationDetailsFragment.kt

planarvoid · 2019-05-23T13:55:58Z

WordPress/src/main/java/org/wordpress/android/ui/domains/DomainRegistrationDetailsFragment.kt

+                domain_privacy_options_radiogroup.setOnCheckedChangeListener { _, _ ->
+                    viewModel.togglePrivacyProtection(domain_privacy_on_radio_button.isChecked)
+                }
+                viewModel.privacyProtectionState.removeObserver(this)


would it be maybe better to always check the current value in the observer and only update the field if the value has changed? one other option we can consider is removing the on text changed/onclick/onchecked listener before updating the value but I think the first approach is better. The thing is that I think stopping the observer is not the best approach. The LiveData advantage are the continuous updates (even though they might not be necessary in this case). It's possible we might get into a state where the LiveData doesn't match the actual Ui State.

Yes, it is absolutely better! And it works really well with 2-way data binding. Without it we have a problem with the data flow loop (?) - we toggle a radio button, listener assigned to it calls ViewModel, which notifies LiveData. Observer reacts to LiveData and toggles the radio button, which triggers a listener and calls ViewModel, etc. :) I replaced OnCheckedChangeListener with normal click listener, so it works as intended, but I still remove observer for the domainContactDetails data to avoid this (not sure how to avoid doing this).

What do you think about this approach:

val currentModel = contactFormToDomainContactModel() if (currentModel != domainContactModel) { populateContactForm(domainContactModel!!) }

I'm not 100% sure this will always work but if it doesn't, we just have to make sure the fields are the same (because they will all go through the equals method). It should work because we'll be using the same method to collect the current data and updating the viewModel.domainContactDetails field.

Ahh, got it! Yes, this will work :)

planarvoid · 2019-05-24T07:07:19Z

WordPress/src/main/java/org/wordpress/android/ui/domains/DomainRegistrationDetailsFragment.kt

+            builder.setTitle(R.string.domain_registration_state_picker_dialog_title)
+            builder.setItems(states.map { it.name }.toTypedArray()) { _, which ->
+                if (targetFragment != null && targetFragment is OnStateSelectedListener) {
+                    (targetFragment as OnStateSelectedListener).onStateSelected(states[which])


I'm doing something very similar right now and I think it makes sense to create the ViewModel with the Activity context and fetch it here and set the field directly. What do you think? This way you can avoid fragment-fragment communication and you don't need the listeners.

This is super clever! I checked your implementation, and it works really well (I see tons of other uses for this too!) but it needs an adapter or something a bit more dynamic than the default implementation of the list in AlerDialog - I'm not sure how to update its content (from LiveData observer) after it is shown. Do you think it's ok to access LiveData value directly (without observer) in this case? This will make things a bit easier.

I think your approach for the data is good 👍 . The only thing I was wondering about is the callback. Instead of casting the targetFragment and going through the fragment to the viewModel you can just inject the ViewModel.Factory and call directly the same ViewModel which handles the Fragment.

.../test/java/org/wordpress/android/viewmodel/domains/DomainRegistrationDetailsViewModelTest.kt

planarvoid · 2019-06-03T12:25:07Z

really good changes @khaykov 👍 let me know what you think about my suggestions!

loremattei · 2019-06-03T13:05:34Z

Hey! I'm moving this to 12.7 since 12.6 has been cut. If you want it to land 12.6, please feel free to move it back, target the release branch and ping me to build a new beta.

…observer.

…teners.

khaykov · 2019-06-05T05:48:21Z

Thanks for the suggestions, @planarvoid ! I learned tons of stuff from them 🙇

planarvoid · 2019-06-05T12:58:37Z

WordPress/src/main/java/org/wordpress/android/ui/domains/DomainRegistrationDetailsFragment.kt

-        builder.show()
+        val dialogFragment = CountryPickerDialogFragment.newInstance(countries.toCollection(ArrayList()))
+        dialogFragment.setTargetFragment(this, 0)
+        dialogFragment.show(fragmentManager, CountryPickerDialogFragment.TAG)
    }

    private fun showFormProgressIndicator() {


NIT: this is a detail but I think it would be fine to have only one method with a boolean parameter that would set the view to visible/gone. The parameter could be named so there is no confusion setFormProgressIndicatorVisibility(visible = ...)

planarvoid · 2019-06-05T13:03:00Z

.../test/java/org/wordpress/android/viewmodel/domains/DomainRegistrationDetailsViewModelTest.kt

+
+        var preloadStep = 0
+
+        Assertions.assertThat(uiStateResults.size).isEqualTo(5)


NIT: I'd do a static import here :-)

planarvoid · 2019-06-05T13:03:48Z

.../test/java/org/wordpress/android/viewmodel/domains/DomainRegistrationDetailsViewModelTest.kt

+        Assertions.assertThat(uiStateResults.size).isEqualTo(5)
+
+        // initial state
+        Assertions.assertThat(uiStateResults[preloadStep].isFormProgressIndicatorVisible).isEqualTo(false)


I think it would be more readable to have the uiStateResults[preloadStep] extracted in a variable.

Good idea 👍 I can get rid of the majority of comments that way.

planarvoid

Everything looks pretty awesome 💯 great job. I've left a few comments that are mostly about code style so I'm approving the code right now.

planarvoid · 2019-06-05T13:16:02Z

The domain registration seems to work well so feel free to once you implement or ignore my comments 👍

nbradbury · 2019-06-05T17:06:56Z

Just a friendly reminder to please make sure to manually merge develop into this branch before you merge this PR, as develop now contains the AndroidX migration. The automatic merge can succeed but still break the build. Better to find out now before it breaks develop. Thanks!

…id into issue/9739-domain-contact-form-refactoring # Conflicts: # WordPress/src/main/java/org/wordpress/android/modules/ApplicationModule.java # WordPress/src/main/java/org/wordpress/android/ui/domains/DomainRegistrationDetailsFragment.kt

peril-wordpress-mobile · 2019-06-05T20:24:34Z

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

Generated by 🚫 dangerJS

… UI states for better readability.

khaykov · 2019-06-06T06:18:20Z

@planarvoid I implemented your suggestions, so feel free to merge if they look ok!

planarvoid

All looks good, thanks for the changes!

khaykov added 9 commits May 20, 2019 09:40

Adding ViewModel.

a69741e

ViewModel tests.

3886279

Added ViewModel to fragment.

0838f6c

Updated layout and resources.

1ca6e5c

Changed state selector visibility logic.

4cf6a19

Updated tests.

2c01961

Added test for same country selected.

ee8aa7f

Fixed wrong field for country code validation. Added more tests.

d02cf8d

ktlint fixes, added more comments

f9badc1

khaykov added this to the 12.6 milestone May 21, 2019

khaykov added Domain Credit [Status] Needs Code Review labels May 21, 2019

wordpress-mobile deleted a comment from peril-wordpress-mobile bot May 21, 2019

Fixed wrong state picked dialog title.

0b9ae72

wordpress-mobile deleted a comment from peril-wordpress-mobile bot May 21, 2019

khaykov requested a review from planarvoid May 21, 2019 20:39

wordpress-mobile deleted a comment from peril-wordpress-mobile bot May 21, 2019

planarvoid reviewed May 24, 2019

View reviewed changes

khaykov added 4 commits May 29, 2019 16:55

Moved site and domainProductDetails to onViewCreated

5d91dae

Removed generic type.

262d8c5

Updated way privacy protection radio button is handled.

2f25ffe

Updated tests.

fc78ddb

wordpress-mobile deleted a comment from peril-wordpress-mobile bot May 30, 2019

Removed more types from observers.

c6f503b

wordpress-mobile deleted a comment from peril-wordpress-mobile bot May 30, 2019

loremattei modified the milestones: 12.6 ❄️, 12.7 Jun 3, 2019

Checking if the viewmodel data matched form data instead of removing …

b3c4005

…observer.

khaykov added 3 commits June 4, 2019 14:12

Calling ViewModel directly from dialog fragments instead of using lis…

3be7431

…teners.

Switched to using UI state.

e6e8d92

updated tests

fa70c9a

wordpress-mobile deleted a comment from peril-wordpress-mobile bot Jun 5, 2019

planarvoid reviewed Jun 5, 2019

View reviewed changes

planarvoid approved these changes Jun 5, 2019

View reviewed changes

khaykov added 2 commits June 5, 2019 14:13

Simplified progress indication toggle methods.

9c545bc

Simplifying test using static import for Assertions and variables for…

5e29939

… UI states for better readability.

planarvoid approved these changes Jun 6, 2019

View reviewed changes

planarvoid merged commit 4fab9e7 into develop Jun 6, 2019

planarvoid deleted the issue/9739-domain-contact-form-refactoring branch June 6, 2019 09:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue/9739 domain contact form refactoring #9892

Issue/9739 domain contact form refactoring #9892

khaykov commented May 21, 2019

planarvoid left a comment

planarvoid May 23, 2019

khaykov May 30, 2019

planarvoid Jun 3, 2019

khaykov Jun 5, 2019

planarvoid Jun 5, 2019

planarvoid May 23, 2019

khaykov May 30, 2019

planarvoid Jun 3, 2019

khaykov Jun 5, 2019

planarvoid May 24, 2019

khaykov May 30, 2019

planarvoid Jun 3, 2019

planarvoid commented Jun 3, 2019

loremattei commented Jun 3, 2019

khaykov commented Jun 5, 2019

planarvoid Jun 5, 2019 •

edited

Loading

planarvoid Jun 5, 2019

planarvoid Jun 5, 2019

khaykov Jun 5, 2019

planarvoid left a comment

planarvoid commented Jun 5, 2019

nbradbury commented Jun 5, 2019

peril-wordpress-mobile bot commented Jun 5, 2019

khaykov commented Jun 6, 2019

planarvoid left a comment


		var preloadStep = 0

		Assertions.assertThat(uiStateResults.size).isEqualTo(5)

Issue/9739 domain contact form refactoring #9892

Issue/9739 domain contact form refactoring #9892

Conversation

khaykov commented May 21, 2019

planarvoid left a 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

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

planarvoid commented Jun 3, 2019

loremattei commented Jun 3, 2019

khaykov commented Jun 5, 2019

planarvoid Jun 5, 2019 • 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

planarvoid left a comment

Choose a reason for hiding this comment

planarvoid commented Jun 5, 2019

nbradbury commented Jun 5, 2019

peril-wordpress-mobile bot commented Jun 5, 2019

khaykov commented Jun 6, 2019

planarvoid left a comment

Choose a reason for hiding this comment

planarvoid Jun 5, 2019 •

edited

Loading