[Analytics Hub] Exit Analytics Hub and display an error if date range selection fails

[rachelmcr]

Part of: #8213

Description

When selecting a date range fails, we now pop out of the Analytics Hub back to the My Store view and display a notice about the error.

We expect this should only happen in rare cases, when AnalyticsHubTimeRangeSelection fails to unwrap the current and previous time ranges.

Changes

• In the analytics hub view model, we now catch AnalyticsHubTimeRangeSelection.TimeRangeGeneratorError errors. We set an error notice and set errorSelectingTimeRange to true.
• In the analytics hub view, we use the system notice presenter to enqueue the error notice when the hosting controller disappears, and pop the hosting controller when errorSelectingTimeRange is true.

Testing

You can force a failed date range selection by temporarily editing one of the methods that unwraps the time range to always throw an error:

woocommerce-ios/WooCommerce/Classes/ViewRelated/Dashboard/Analytics Hub/Time Range/AnalyticsHubTimeRangeSelection.swift

Lines 68 to 73 in 0af536a

	func unwrapCurrentTimeRange() throws -> AnalyticsHubTimeRange {
	guard let currentTimeRange = currentTimeRange else {
	throw TimeRangeGeneratorError.selectedRangeGenerationFailed
	}
	return currentTimeRange
	}

Then:

1. Build and run the app.
2. Tap the "See more" button.
3. Confirm that you are popped out of the Analytics Hub and an error notice appears on the My Store screen.

Screenshots

DateRangeSelectionError.mp4

Submitter Checklist

Update release notes:

• I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

[wpmobilebot]

You can test the changes from this Pull Request by:

• Clicking here or scanning the QR code below to access App Center
• Then installing the build number pr8298-81e9c33 on your iPhone

If you need access to App Center, please ask a maintainer to add you.

[ealeksandrov]

It works well in my testing!
Just a few small considerations/ideas for improvement:

1. In the trunk we have additional refresh action which error may be unhandled.
2. Do you think we can connect dismiss + notice actions directly, so it's more obvious without relying on UIKit lifecycle (viewWillDisappear)? For example: dismiss(with notice: Notice?) on HostingVC can trigger both actions with clear intent.
3. errorSelectingTimeRange feels like duplicated information when we already have notice set and error thrown. Can we trigger notice+dismiss in the catch clause?

[ealeksandrov]

Xcode shows a warning here for each navigation:

Accessing StateObject's object without being installed on a View. This will create a new instance each time.

[rachelmcr]

Thanks for the review @ealeksandrov! I took a look at your suggestions; let me know what you think.

In the trunk we have additional refresh action which error may be unhandled.

Thanks for the heads up! I merged trunk to take a look at the refresh action and handled the error with the changes outlined below.

Do you think we can connect dismiss + notice actions directly, so it's more obvious without relying on UIKit lifecycle (viewWillDisappear)? For example: dismiss(with notice: Notice?) on HostingVC can trigger both actions with clear intent.

I like the idea of making the intent clearer like this, but if we don't rely on the UIKit lifecycle it will change the timing of the notice. Using viewWillDisappear ensures that the notice doesn't appear before the view is dismissed. Otherwise, the notice shows up while the view is still visible:

Simulator.Screen.Recording.-.iPhone.14.Pro.-.2022-12-05.at.15.32.38.mp4

I prefer the timing that it has now, but I've updated the dismiss action in 7aa749f so it's more clearly connected to the notice. (This also resolves the Xcode warning you observed.) This way it still uses the UIKit lifecycle for the notice timing, but it also makes it clearer that we're both dismissing the view and handling the notice.

errorSelectingTimeRange feels like duplicated information when we already have notice set and error thrown. Can we trigger notice+dismiss in the catch clause?

That's reasonable. It feels a little odd to me to call UI code directly in the view model, so I've updated the catch clause in 228f144 to just set the notice (removing errorSelectingTimeRange). Then, the view handles the dismiss action with the provided notice when the notice is set. (I've moved that to a separate .onReceive modifier to handle both the initial sync and refresh actions.)

[ealeksandrov]

Thanks for all changes and the demo, I agree that current implementation looks better!
One last nit: can we rename notice property and its comment to describe that setting it will dismiss the view? Something like "dismissNotice"?