Add "Orders to fulfill" card to dashboard #293

AmandaRiu · 2018-08-07T04:18:19Z

Fixes #283

This PR adds the Orders to fulfill card to the Dashboard.

---

UI

This card should only be visible is the total count of orders to fulfill > 0. Otherwise it should be hidden.

This card makes use of pluralized strings to display the correct version of the string if there is only one order vs more than one order.

---

Navigation Changes

This feature required a new type of navigation, cross-TopLevelFragment. To keep this flexible, I've created a new interface called TopLevelFragmentRouter. It only contains one simple function showOrderList(...) at the moment, but can later accept a bunch of routing options. It also allows us to do stuff like this so we know the hosting activity can handle this feature:

(activity as? TopLevelFragmentRouter)?.showOrderList(CoreOrderStatus.PROCESSING.value)

I envision the TopLevelFragment design being replaced with the new Android Navigation components eventually, but for now, I've been trying to keep them generic and reusable whenever possible to avoid a bunch of custom routing scenarios. To this end, I've added a new property to TopLevelFragment named deferInit. This allows for loading a top level fragment and specifying that a portion of the initialization code be skipped so it can manually be executed later. Using this property I can leave all the code for locating and/or creating a TopLevelFragment generic and then in the case of the OrderListFragment pass along a status filter and call a method to finish loading.

Refreshing the Dashboard cards

The data for the dashboard cards are fetched upon initialization, and then refreshed under the following scenarios:

Manually via pull to refresh
When any of the OnOrderChanged events are consumed except when the event wouldn't cause the dashboard to fall out of sync with the orders view. So the FETCH_ORDER_NOTES event is ignored. Any OnOrderChanged event that has an error is also ignored.

If the DashboardFragment is currently in the background, we just set a flag so it knows to refresh when it comes to the foreground.

Dependencies

The FluxC PR must be merged before this PR can be merged.

cc: @nbradbury @aforcier

…Order To Fulfill Dashboard card feature

Includes the layout XML, custom class, and plural string support.

Updated tests as well

This makes the method much more flexible for future use with custom statuses and helps to clean up usage for the orders to fulfill dashboard card.

This is used to prevent the orders list fragment from loading the full list before we can set the status filter. This also gives us a generic way of deferring initialization of any of the top level fragments for similar cases in the future.

Since `onBackStackChanged()` is from the childFragmentManager, this method will never be called until we start opening child fragments on top of the DashboardFragment. The `isActive` property now uses both the childFragmentManager's backstack count as well as the hidden property to determine this fragments visibility.

The `isActive` property now uses both the childFragmentManager's backstack count as well as the hidden property to determine this fragments visibility.

Refresh if an OnOrderChanged event was received that wasn't either a fetch order count response, or a request for order notes (since those happen frequently).

nbradbury · 2018-08-08T15:58:52Z

WooCommerce/src/main/kotlin/com/woocommerce/android/ui/dashboard/DashboardContract.kt

+        fun refreshDashboard()
        fun setLoadingIndicator(active: Boolean)
        fun showStats(revenueStats: Map<String, Double>, salesStats: Map<String, Int>, granularity: StatsGranularity)
+        fun hideOrdersCard()


These two functions (and the view itself) seem mis-named to me. How about using UnfilledOrdersCard instead of simply OrdersCard?

Works for me. Fixed in 5b1863a

nbradbury · 2018-08-08T16:01:02Z

WooCommerce/src/main/res/layout/dashboard_fulfill_orders.xml

+        app:layout_constraintTop_toBottomOf="@+id/alertAction_title"/>
+
+    <!-- Action Button -->
+    <Button


Should this button have android:background="?android:attr/selectableItemBackground"?

Good catch, this should have a style attribute assigned to it. Fixed in 78fc331

nbradbury · 2018-08-08T16:02:37Z

The way the card suddenly appears is a little odd to me. How about fading it in instead?

nbradbury · 2018-08-08T16:47:07Z

WooCommerce/src/main/kotlin/com/woocommerce/android/ui/dashboard/DashboardFragment.kt

+            hideOrdersCard()
            loadDataPending = false
            setLoadingIndicator(true)
+            presenter.loadOrdersToFulfillCount()


Can these three lines be replaced with refreshDashboard()?

Fixed in 1217c0c

nbradbury · 2018-08-08T16:48:50Z

WooCommerce/src/main/kotlin/com/woocommerce/android/ui/dashboard/DashboardFragment.kt

        if (isActive) {
            setLoadingIndicator(true)
            presenter.loadStats(dashboard_stats.activeGranularity, forced = true)
+            presenter.loadOrdersToFulfillCount()


Do we need to add loadDataPending = false here?

👍 fixed in 1217c0c

nbradbury · 2018-08-08T16:51:30Z

WooCommerce/src/main/kotlin/com/woocommerce/android/ui/dashboard/DashboardPresenter.kt

+                dashboardView?.showOrdersCard(count)
+            } ?: dashboardView?.hideOrdersCard()
+        } ?: if (!event.isError && event.causeOfChange != WCOrderAction.FETCH_ORDER_NOTES) {
+            dashboardView?.refreshDashboard()


I'm not sure about this because it causes the dashboard to refresh even if nothing has changed. That seems wasteful since refreshing requires two non-trivial network requests.

But something may have changed. An example scenario:

User opens app, the dashboard loads and the orders to fulfill card displays 3

User clicks view orders

While the user is viewing the orders list, another order is submitted on their store

the user refreshes the orders list view and sees the order

the user switches back to the dashboard - the dashboard refreshes with the updated information because it was listening for that refresh event on orders.

I guess since the refresh is deferred until the next time the dashboard is accessed then this isn't as big a deal as I thought, but down the road it really would be nice if we only made network requests when we know they're necessary (ie: something has changed). In the meantime, though, can we also skip the refresh when the cause of change is POST_ORDER_NOTE?

it really would be nice if we only made network requests when we know they're necessary

That's the plan once notifications are done.

can we also skip the refresh when the cause of change is POST_ORDER_NOTE

Absolutely! 👍

Post order notes event is ignored as of 2038fad

nbradbury · 2018-08-08T17:42:31Z

WooCommerce/src/main/kotlin/com/woocommerce/android/ui/dashboard/DashboardFulfillOrdersCard.kt

+    }
+
+    fun updateOrdersCount(count: Int) {
+        val titleTxt = resources.getQuantityString(R.plurals.dashboard_fulfill_orders_title, count, count)


As Alex mentioned earlier, we can't use Android plurals due to a limitation in Glotpress. Also, this isn't handling the situation where there are more than 50 orders. Instead of saying 50+ it just says 50.

Updated in e0d40ab

…e the one implemented in WordPress-Android. This is because our translation service does not support pluralized strings.

AmandaRiu · 2018-08-08T22:56:51Z

@nbradbury Thanks for the thorough review! Ready for a second round 🥇

nbradbury · 2018-08-09T14:15:34Z

@AmandaRiu This all looks great, except that it still doesn't handle the case where there are more than 50 orders. In that situation it's supposed to say 50+, right?

…r fetching This is temporary until we have an api endpoint that will provide a total count directly.

AmandaRiu · 2018-08-10T02:57:08Z

@nbradbury Added support for displaying the plus sign. Ready for a 3rd round 🤦‍♀️

nbradbury · 2018-08-10T11:51:24Z

Added support for displaying the plus sign. Ready for a 3rd round

@AmandaRiu I'm not seeing this - did you forget to push the change?

AmandaRiu · 2018-08-10T14:57:28Z

@nbradbury it would seem I did! ✔️ Done

AmandaRiu · 2018-08-11T23:21:43Z

@nbradbury Yay! tests finally passed!

nbradbury · 2018-08-12T02:01:14Z

AmandaRiu added 12 commits August 2, 2018 19:18

Update to latest FluxC commit hash

acce241

Write fetching stats errors to WooLog

a2b2a10

Update to use the commit hash for the FluxC branch that supports the …

2344504

…Order To Fulfill Dashboard card feature

Create custom orders to fulfill dashboard card

45cc9aa

Includes the layout XML, custom class, and plural string support.

Add orders dashboard card to dashboard view

fcfb137

Add WCOrderStore to constructor to ensure it gets initialized for use

20c5e0a

Updated tests as well

Remove unnecessary null definition

2b758e2

Add support for displaying and populating orders dashboard card

3c3f8fd

Update the filter listener to pass back the filter string

cdd7bcf

This makes the method much more flexible for future use with custom statuses and helps to clean up usage for the orders to fulfill dashboard card.

Add hooks to open order list from MainActivity

92611d9

Add logic to open a filtered orders list

12d2366

AmandaRiu added the [Status] Not Ready for Review label Aug 7, 2018

AmandaRiu self-assigned this Aug 7, 2018

AmandaRiu added 4 commits August 7, 2018 16:00

Fix the way Orders List determines visibility

a6f1536

The `isActive` property now uses both the childFragmentManager's backstack count as well as the hidden property to determine this fragments visibility.

Add refresh dashboard cards logic

c76185c

Refresh if an OnOrderChanged event was received that wasn't either a fetch order count response, or a request for order notes (since those happen frequently).

Add unit tests for orders dashboard card

4680abc

AmandaRiu added this to the External open beta group milestone Aug 7, 2018

AmandaRiu added the MVLP label Aug 7, 2018

AmandaRiu added 3 commits August 7, 2018 17:20

Use the much safer Kotlin as? operator to avoid runtime exceptions

ca11163

Refactor to remove need for the dangerous Kotlin as operator

07c6356

Refactor to remove need for the dangerous Kotlin as operator

44d8028

AmandaRiu requested a review from nbradbury August 7, 2018 22:58

AmandaRiu removed their assignment Aug 7, 2018

AmandaRiu added status: do not merge Dependent on another PR, ready for review but not ready for merge. and removed [Status] Not Ready for Review labels Aug 7, 2018

Fix unit tests

f7f439a

nbradbury self-assigned this Aug 8, 2018

nbradbury reviewed Aug 8, 2018

View reviewed changes

AmandaRiu added 6 commits August 8, 2018 14:59

Remove pluralized strings and replaced with a utility method much lik…

e0d40ab

…e the one implemented in WordPress-Android. This is because our translation service does not support pluralized strings.

Ignore post order note events

2038fad

Add missing button style

78fc331

Rename dashboard orders card and related assets, variables, and methods.

5b1863a

Consolidate and optimize refresh code

1217c0c

Animate unfilled orders card visibility changes

c72770a

AmandaRiu removed the status: do not merge Dependent on another PR, ready for review but not ready for merge. label Aug 8, 2018

AmandaRiu added the [Status] Not Ready for Review label Aug 9, 2018

AmandaRiu added 2 commits August 9, 2018 21:50

Fixed a minor animation flicker

260f56d

Add support for displaying a "+" sign if more orders are available fo…

c35dde7

…r fetching This is temporary until we have an api endpoint that will provide a total count directly.

AmandaRiu removed the [Status] Not Ready for Review label Aug 10, 2018

AmandaRiu added 2 commits August 11, 2018 16:09

Fix DashboardPresenter tests

a050e18

Fix DashboardPresenter tests

9783776

nbradbury merged commit 5c5b264 into develop Aug 12, 2018

nbradbury deleted the issue/283_dash_orders branch August 12, 2018 02:01

astralbodies mentioned this pull request Aug 22, 2018

Dashboard: New Orders woocommerce/woocommerce-ios#35

Closed

Add "Orders to fulfill" card to dashboard #293

Add "Orders to fulfill" card to dashboard #293

Uh oh!

Conversation

AmandaRiu commented Aug 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

UI

Navigation Changes

Refreshing the Dashboard cards

Dependencies

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!

AmandaRiu Aug 8, 2018 • 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!

nbradbury commented Aug 8, 2018

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AmandaRiu commented Aug 8, 2018

Uh oh!

nbradbury commented Aug 9, 2018

Uh oh!

AmandaRiu commented Aug 10, 2018

Uh oh!

nbradbury commented Aug 10, 2018

Uh oh!

AmandaRiu commented Aug 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AmandaRiu commented Aug 11, 2018

Uh oh!

nbradbury commented Aug 12, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

AmandaRiu commented Aug 7, 2018 •

edited

Loading

AmandaRiu Aug 8, 2018 •

edited

Loading

AmandaRiu commented Aug 10, 2018 •

edited

Loading