My Profile Network logic by koke · Pull Request #4732 · wordpress-mobile/WordPress-iOS

koke · 2016-01-25T17:55:41Z

This PR extends the network logic of My Profile from a simple "refresh when the view appears for the first time" to a more complex scenario:

Account settings should be refreshed when My Profile becomes visible and there's an internet connection
If the connection comes back, it should attempt the refresh automatically
After a polling interval (60 seconds) it refreshes the data, as long as My Profile is still visible
If there's a "recoverable" error (timeout, bad connection), it will retry the refresh up to three times
If the view disappears, any request should be canceled.
There should be only one request per account in progress.

It also adds a new notice message that will show:

If the request (including retries) is taking more than 5 seconds, a "We are having trouble loading your data"
If the request fails: "We had trouble loading your data"
If there's no connection: "You are offline"

Testing tips

I've done most of the testing with a device and the Network Link Conditioner, and the "Very Bad Network" preset. You can also tweak AccountSettingsService.Defaults to lower the timeout so the message appears early.

If you want to test the notice animation, all the logic is contained in ErrorAnimator and Animator. When I was developing it, I used a simple table view controller with a navbar button to toggle it, and slow animations. It seemed to behave properly with multiple toggles while in progress, and even animated rotation properly. But feel free to try and break it as hard as you can, it's my first animation code in years 😉

What's next

There are some things that I had in my notes that aren't handled yet, but I'll leave them here for reference:

The service should not attempt an API request if a previous one succeeded less than polling_interval seconds ago. I think currently reachability or the view reappearing would override this. Since we have no way to 'force-refresh' other than re-pushing the view, I'm not 100% sure if we really want that behaviour.
The first time My Profile loads and we have no AccountSettings data in the app, the rows shouldn't be tappable until there's some data.
When the refresh fails, it won't refresh again until the view is dismissed and presented again. We've talked about the notice view having a "Tap to retry".

Needs Review: @astralbodies

Converted the settings refresh to use observables. This allows us to do a few interesting things relatively easily: - Cancel refresh when observer unsubscribes - Show a message when the request is taking longer than usual - Share a refresh request by many view controllers (not implemented yet, but `.share()` should make it possible)

…ndling

Instead, expose the underlying label and modify its attributes directly

- Split multiple observables into their own properties. - Adds retry logic for recoverable errors. - Reuse existing remotes to avoid duplicating network observables. - Improved logging - Adds polling - Improves "Stalled" logic so it doesn't duplicate/cancel requests

Instead of creating an observable for each call to settings, store it as a property and add `.share()` so multiple subscriptions don't cause multiple netwokr requests.

When possible, avoid the overhead (and possible duplication of side effects) of creating observables by storing them instead of using computed properties. Also, add documentation for observable properties.

Also add locking as suggested in ReactiveX/RxSwift#422

Mostly a helper to retryWhen but improves readability

Refactored AccountSettingsService a bit so it only exposes `request`. Reverted the change where `remoteSettings` would initially emit a `.Refreshing` value as it breaks the `amb` in `request`

Use createColdObservable when possible as it's way more readable. The only reason to avoid it is in `testRequestOneNetworkErrorShouldRetry` because we want to return something different the second time it's subscribed.

We're already combining subscription in the service, and this was preventing a second settings request when My Profile was dismissed and presented again

koke · 2016-01-26T14:29:58Z

I'm not sure what to do about this. I could just document the restriction, add a precondition, and make sure we only call this on the main thread. Right now this method is only called from the AccountSettingsService initialiser, which is called on the main thread.

Other than it being easier, there's no good reason to restrict this to the main thread, but making this thread safe adds complexity for no immediate gain.

Thoughts?

I think this is one of those cases why services have been kept ephemeral to prevent issues like this.

I'd rather use the uniqueness of WordPressComApi which should come up with its own hashValue that is based upon a set of business logic conditions. This makes AccountSettingsRemote know too much about the api class I think. Then, in theory, two instances of WordPressComApi that are logically equivalent (same auth token, same username, etc) are interchangeable.

I can encapsulate the hashing inside the api object, IIRC this was mostly a convenience since hashValue is part of Hashable, which is a Swift thing, and WordPressComApi is Obj-C. So I think I did this instead of having to create a WordPressComApi swift extension just to implement Hashable. And also, because Hashable is Equatable which brings me to this:

in theory, two instances of WordPressComApi that are logically equivalent (same auth token, same username, etc) are interchangeable.

That would be nice, but WordPressComApi is a subclass of AFHTTPRequestOperationManager, so it's not just the auth token and username, but its own operation queue with in-progress operations, and all the other inherited AFNetworking state.

Which, by the way, was the reason why we wanted to reuse the same API object per account: so it's all the same operation queue, and we don't send too many concurrent requests to the same host.

That makes complete sense!

astralbodies · 2016-01-26T20:35:04Z

My Profile - iOS

Prior to this, MyProfileController was a class and owned the view controller. This had a couple problems: 1. We want to release the whole thing when UIKit decides it should deallocate the view controller, and for that we need the VC to own the controller. 2. Having the controller being a class led to a lot of retain cycles since the code relies on closures a lot. With the new code, the VC and controller are deallocated when the VC is popped, and the Observable subscriptions are properly disposed.

Since controller has value semantics, our local `controller` wasn't changed so its presenter was nil. the viewController.controller is a copy of the local one, and it's the one that has (and needs to have) a presenter.

astralbodies · 2016-02-01T19:51:52Z

Could we put some placeholder text for the right-hand label if no value is set?

astralbodies · 2016-02-01T19:55:25Z

One scenario I've found that could lose changes - user is online, goes in to edit a field, loses connection (or it goes lossy), field isn't updated on the main My Profile screen because remote request hasn't finished, then dismisses the view going back to Me. It seems like in this scenario the change is forever lost because background actions are canceled when the view is dismissed.

The impression I get as a user that when I tap Done after editing the field is my change has been made/no need to worry its lost. Coming back to My Profile and not seeing the change immediately could be puzzling. Losing the change entirely would definitely instill distrust. Any ideas on how to improve this?

update -

So it looks like this was only happening with wifi shut off, not a 100% loss network link conditioner turned on.

astralbodies · 2016-02-01T20:16:58Z

Just wondering if these class extension methods are something you came up with or if they're from a third party?

My own code, although pausable is inspired by the RxJS implementation. It is written in this style because I originally submitted it as a PR to RxSwift and followed their style.

astralbodies · 2016-02-01T20:32:39Z

With this design do you think it's still feasible to use AccountServiceRemote without RxSwift elsewhere in the app or is it tying it tightly to the RxSwift Observer pattern?

astralbodies · 2016-02-01T20:37:22Z

Does this mean we'll have Reachability instances for each instance of the AccountSettingsService? No big deal if so, just something to consider the more services we have hanging around with this new approach.

By default, it uses Reachability.internetConnection which is a class constant, so it should be just one instance

Ah yes, now I remember why we got away with doing host-based reachability instances.

astralbodies · 2016-02-01T21:33:38Z

Sorry for the smattering of comments .. code reviews on GitHub can be utterly painful when dealing with a stream of conversations.

koke · 2016-02-01T22:29:09Z

With this design do you think it's still feasible to use AccountServiceRemote without RxSwift elsewhere in the app or is it tying it tightly to the RxSwift Observer pattern?

AccountServiceRemote.getSettings(success:failure:) is still there, so it can also be used the old way

koke · 2016-02-01T22:30:54Z

Could we put some placeholder text for the right-hand label if no value is set?

I'd leave that to @rickybanister. I think we discussed it and found the error messages to be enough. In any case, I think it's out of scope for this PR.

koke · 2016-02-01T22:36:51Z

One scenario I've found that could lose changes - user is online, goes in to edit a field, loses connection (or it goes lossy), field isn't updated on the main My Profile screen because remote request hasn't finished, then dismisses the view going back to Me. It seems like in this scenario the change is forever lost because background actions are canceled when the view is dismissed.

The impression I get as a user that when I tap Done after editing the field is my change has been made/no need to worry its lost. Coming back to My Profile and not seeing the change immediately could be puzzling. Losing the change entirely would definitely instill distrust. Any ideas on how to improve this?

Definitely agree. I think the answer to this is that this PR updated only the My Profile screen, but didn't touch the other VCs. IMO, when you're editing a field and are offline you should see the same error message saying "You are offline, your changes won't be saved". We can't prevent you from tapping Save because there's no Save button, and since it's a navigation controller we can't stop you from going back, but at least we can warn you.

But besides this, if a change request fails for whatever reason, we need to let the user know. I think we discussed this at some point, but it's not implemented yet. It's another challenge, as you could be anywhere in the app (or even in the background) when the request fails, so it's not clear which part of the app would be responsible for showing the error.

astralbodies · 2016-02-02T14:59:37Z

Cool, I think this is good to merge for now!

…or-handling My Profile Network logic

koke added 30 commits January 18, 2016 16:39

Added notices UI for the connection errors

8a26666

Added link to WWDC video in the documentation

d50c207

Merge branch 'issue/fix-imports-7.3' into feature/my-profile-error-ha…

72479f2

…ndling

Remove PaddedLabel accessors

9336aed

Instead, expose the underlying label and modify its attributes directly

Adds reachability to refresh logic

2cbf62c

Merge branch 'develop' into feature/my-profile-error-handling

caa2cf6

Add Rx forwardIf operator

07e5e60

Don't emit an error for canceled requests

3abc714

Stop refreshing when VC disappears

f653973

Refactor settings remote to multicast

2c31f90

Instead of creating an observable for each call to settings, store it as a property and add `.share()` so multiple subscriptions don't cause multiple netwokr requests.

Refactor service to favor stored properties

e5b7a41

When possible, avoid the overhead (and possible duplication of side effects) of creating observables by storing them instead of using computed properties. Also, add documentation for observable properties.

Merge branch 'develop' into feature/my-profile-error-handling

68ed00e

Documentation and comments for settings remote

74d2937

Unit Tests for settings remote

eba03b9

Remove unused method

bbaf0e9

Unit tests for AccountSettingsService

e81a341

Rename forwardIf as pausable

391d36d

Also add locking as suggested in ReactiveX/RxSwift#422

Make remoteSettings emit .Refreshing initially

5f5a7af

Unit Tests for Rx pausable operator

c408f95

Adds retryIf Rx operator

8be6b28

Mostly a helper to retryWhen but improves readability

Fix Rx pausable test

c150eac

Make remoteSettings emit errors instead of .Failing

f5e028a

Unit test for .Stalled

fe46587

Refactored AccountSettingsService a bit so it only exposes `request`. Reverted the change where `remoteSettings` would initially emit a `.Refreshing` value as it breaks the `amb` in `request`

Updated documentation for request

4626660

Unit tests for settings reachability

5eae3de

Refactor unit tests for readability

b9af55c

Use createColdObservable when possible as it's way more readable. The only reason to avoid it is in `testRequestOneNetworkErrorShouldRetry` because we want to return something different the second time it's subscribed.

Renamed AccountSettingsRemoteTests

4f10c4a

Don't share remote's settings observable

63b76c1

We're already combining subscription in the service, and this was preventing a second settings request when My Profile was dismissed and presented again

koke reviewed Jan 26, 2016
View reviewed changes

koke added 2 commits January 26, 2016 15:45

Turn second initializer into a convenience initializer

ee71629

Fixed Animator example

5d20736

astralbodies self-assigned this Jan 26, 2016

koke added 3 commits January 26, 2016 22:55

Merge branch 'develop' into feature/my-profile-error-handling

8f2bcfb

Add ImmuTable.Empty helper

e59301d

koke added /Me and removed Not Ready For Review labels Jan 27, 2016

koke added 2 commits January 27, 2016 08:28

Add some more safeguards against a nil presenter

47d92db

Fix presenter assertion

3f20e45

Since controller has value semantics, our local `controller` wasn't changed so its presenter was nil. the viewController.controller is a copy of the local one, and it's the one that has (and needs to have) a presenter.

koke mentioned this pull request Jan 27, 2016

Account Settings, part 1 #4740

Merged

astralbodies reviewed Feb 1, 2016
View reviewed changes

koke added a commit that referenced this pull request Feb 2, 2016

Merge pull request #4732 from wordpress-mobile/feature/my-profile-err…

a0c04fd

…or-handling My Profile Network logic

koke merged commit a0c04fd into develop Feb 2, 2016

koke deleted the feature/my-profile-error-handling branch February 2, 2016 15:04

mokagio mentioned this pull request Jul 12, 2026

Adopt a8c-secrets in WordPress iOS #25772

Draft

Uh oh!

Conversation

koke commented Jan 25, 2016

Testing tips

What's next

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!

astralbodies commented Jan 26, 2016

Uh oh!

astralbodies commented Feb 1, 2016

Uh oh!

astralbodies commented Feb 1, 2016

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!

astralbodies commented Feb 1, 2016

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!

astralbodies commented Feb 1, 2016

Uh oh!

koke commented Feb 1, 2016

Uh oh!

koke commented Feb 1, 2016

Uh oh!

koke commented Feb 1, 2016

Uh oh!

astralbodies commented Feb 2, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants