Dynamic Dashboard: Reviews Card Reload Functionality #12752

hafizrahman · 2024-05-16T06:41:31Z

Part of: #12742

Description

This PR includes:

initial logic for fetching reviews (and products), and displaying it on the UI.
functionality for viewing all reviews

The main purpose of this PR is to get more eyes on the reviews and products fetching logic, and make sure the logic is sound, before going forward with more functionality. Because of that, some functionalities are missing and will be added in subsequent PR:

The card doesn't load notifications yet, which is needed to determine balloon color (read or not read). The loading logic should be similar to products loading, so should be relatively simple to add once the products loading is confirmed.
Filtering is not done yet. Still discussing in Slack what's the best way forward.
Tapping each review is not added yet.
Empty and error states are not added yet.

Testing instructions

Run app, use a test site with at least 1 review on it.
Enable the "Most recent reviews" card in the dashboard if you haven't,
wait and ensure it loads and shows three reviews, with the correct subject, review content, and star.
Try the "View all reviews" and ensure it opens the Reviews list screen,
Go back to Dashboard and ensure the card still shows 3 cards as before.

Screenshots

… and add it to the data.

Conflicts: WooCommerce/Classes/ViewRelated/Dashboard/DashboardViewModel.swift WooCommerce/Classes/ViewRelated/Dashboard/Reviews/ReviewsDashboardCard.swift

WooCommerce/Classes/ViewRelated/Dashboard/Reviews/ReviewsDashboardCard.swift

wpmobilebot · 2024-05-16T06:49:34Z

📲 You can test the changes from this Pull Request in WooCommerce iOS by scanning the QR code below to install the corresponding build.

	App Name	WooCommerce iOS
	Build Number	pr12752-40bb44a
	Version	18.6
	Bundle ID	com.automattic.alpha.woocommerce
	Commit	`40bb44a`
	App Center Build	WooCommerce - Prototype Builds #9134

Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

hafizrahman · 2024-05-16T09:27:47Z

WooCommerce/Classes/ViewRelated/Dashboard/Reviews/ReviewsDashboardCardViewModel.swift

+    func updateReviewsResults() async {
+        let reviews = productReviewsResultsController.fetchedObjects.prefix(Constants.numberOfItems)
+        let reviewProductIDs = reviews.map { $0.productID }
+
+        // Load products that matches the review product IDs
+        if reviewProductIDs.isNotEmpty {
+            await loadReviewProducts(for: reviewProductIDs)
+        }
+
+        data = reviews
+            .map { review in
+                let product = reviewProducts.first { $0.productID == review.productID }
+                // TODO: also fetch notification
+                return ReviewViewModel(review: review, product: product, notification: nil)
+            }
+    }


I'd like to get more opinion on this area.

The fetch result for reviews only includes data for product ID related to the reviews, while the UI requires the product name to be displayed. Thus, another call loadReviewProducts() is needed to get the product names.

I'm not 100% sure if this is the best way to do this. It feels to me that it's going through many steps, but seems inevitable if we want to use storage as single source of truth:

remote call for reviews

handle db update for reviews

remote call for products

handle db update for products

compare local reviews and local products to build data

Thanks for explaining, Hafiz!

I think you can try fetching from storage before sending a remote call for products.

You can use the following predicate to fetch multiple products from storage.

NSPredicate(format: "siteID == %lld AND productID IN %@", siteID, arrayOfProductIDs)

What do you think?

I updated the logic in 4ad2e89

Now it will fetch products from storage first and only do remote call if they're not found.

selanthiraiyan

I left a comment about reading from storage before sending a request to retrieve products. Please let me know your thoughts.

WooCommerce/Classes/ViewRelated/Dashboard/DashboardViewModel.swift

WooCommerce/Classes/ViewRelated/Dashboard/Reviews/ReviewsDashboardCard.swift

selanthiraiyan · 2024-05-16T11:57:12Z

WooCommerce/Classes/ViewRelated/Dashboard/Reviews/ReviewsDashboardCardViewModel.swift

+    func updateReviewsResults() async {
+        let reviews = productReviewsResultsController.fetchedObjects.prefix(Constants.numberOfItems)
+        let reviewProductIDs = reviews.map { $0.productID }
+
+        // Load products that matches the review product IDs
+        if reviewProductIDs.isNotEmpty {
+            await loadReviewProducts(for: reviewProductIDs)
+        }
+
+        data = reviews
+            .map { review in
+                let product = reviewProducts.first { $0.productID == review.productID }
+                // TODO: also fetch notification
+                return ReviewViewModel(review: review, product: product, notification: nil)
+            }
+    }


Thanks for explaining, Hafiz!

I think you can try fetching from storage before sending a remote call for products.

You can use the following predicate to fetch multiple products from storage.

NSPredicate(format: "siteID == %lld AND productID IN %@", siteID, arrayOfProductIDs)

What do you think?

WooCommerce/Classes/ViewRelated/Dashboard/Reviews/ReviewsDashboardCard.swift

…ded.

WooCommerce/Classes/ViewRelated/Dashboard/Reviews/ReviewsDashboardCard.swift

selanthiraiyan

Thanks for the updates to read from storage, @hafizrahman! 👍

I left a few comments/questions about the logic. Please let me know your thoughts.

WooCommerce/Classes/ViewRelated/Dashboard/Reviews/ReviewsDashboardCard.swift

WooCommerce/Classes/ViewRelated/Dashboard/Reviews/ReviewsDashboardCardViewModel.swift

1. reviewProducts is now a computed var to simplify fetchProducts() 2. productsResultsController now initialized first and updated later when productIDs exist with updateProductsResultsControllerPredicate() 3. reviewProductIDs is now simply passed as function parameters and not kept as class variable 4. Update logic for checking existing products before needing to fetch 5. Code organization with MARK for Storage-related code

Conflicts: WooCommerce/Classes/ViewRelated/Dashboard/DashboardViewModel.swift

selanthiraiyan

Thanks for the updates, Hafiz!

This looks good to me. I left a few non-blocking suggestions. 🚀

WooCommerce/Classes/ViewRelated/Dashboard/Reviews/ReviewsDashboardCardViewModel.swift

selanthiraiyan · 2024-05-17T12:05:52Z

WooCommerce/Classes/ViewRelated/Dashboard/Reviews/ReviewsDashboardCardViewModel.swift

+
+    func updateProductsResultsControllerPredicate(with productIDs: [Int64]) {
+        let predicates = NSCompoundPredicate(andPredicateWithSubpredicates: [sitePredicate(),
+                                                                            NSPredicate(format: "productID IN %@", productIDs)])


Super nit: Indentation fix needed.

I think this looks odd but perhaps correct because it's an array as the parameter.

Either using enter manually or Editor > Structure > Re-indent results the same like above:

Screen.Recording.2024-05-20.at.10.25.40.mov

selanthiraiyan · 2024-05-17T12:07:44Z

WooCommerce/Classes/ViewRelated/Dashboard/Reviews/ReviewsDashboardCardViewModel.swift

+
+        self.productsResultsController = ResultsController<StorageProduct>(storageManager: storageManager,
+                                                                           matching: nil,
+                                                                           sortedBy: [])


nit: We could mention the fetchLimit: Constants.numberOfItems here as well.

Good thinking. I had assumed that when the predicate is updated and it's requesting up to 3 product IDs, it would always return up to 3 items anyway since they're distinct. But I agree it's safer to limit it here too. Added in 37fc482

selanthiraiyan · 2024-05-17T12:18:25Z

WooCommerce/Classes/ViewRelated/Dashboard/Reviews/ReviewsDashboardCardViewModel.swift

+            stores.dispatch(ProductAction.retrieveProducts(siteID: siteID,
+                                                           productIDs: reviewProductIDs
+                                                          ) { result in
+                continuation.resume(with: result)


Non blocking suggestion:
We could replace this with the following to skip returning stuff from this function.

switch result { case .success: continuation.resume() case .failure(let error): continuation.resume(throwing: error) }

The same applies to func loadReviews() async throws -> [ProductReview] method as well.

Thanks, I added the updates for these functions on 40bb44a

selanthiraiyan · 2024-05-17T12:22:28Z

WooCommerce/Classes/ViewRelated/Dashboard/Reviews/ReviewsDashboardCardViewModel.swift

+            // Ignoring the result from remote as we're using storage as the single source of truth
+            _ = try await retrieveProducts(for: reviewProductIDs)
+        } catch {
+            syncingError = error


Suggestion: I believe that we can display the reviews even if retrieving products fails. Could we ignore the error and just display the reviews without product info?

If we think about it further, we could even load the products silently in the background and update the UI. What do you think?

This is a neat suggestion and I can try adding it on the next PR that deals with loading states. I do notice that it can take a while waiting for products and notifications to be fetched, and showing partial result first can be more useful to users. 👍🏼

…oardCardViewModel.swift let/var fix Co-authored-by: Sharma Elanthiraiyan <sharma.elanthiraiyan@automattic.com>

hafizrahman added 9 commits May 15, 2024 19:05

Add functionality to open all reviews from card.

40184d4

Add initial reloadData function for Reviews card.

ff9c78b

Add db loading and display the result.

37193af

Strip HTML from the review text.

b61d592

Use the existing ReviewViewModel for data.

2eea667

Add functionality to fetch products after getting review product IDs,…

a47f3bb

… and add it to the data.

Update visibility handling.

616b88b

Merge branch 'trunk'

42da0b8

Conflicts: WooCommerce/Classes/ViewRelated/Dashboard/DashboardViewModel.swift WooCommerce/Classes/ViewRelated/Dashboard/Reviews/ReviewsDashboardCard.swift

Make sure to only get the latest three reviews.

12d09c8

hafizrahman added the type: task An internally driven task. label May 16, 2024

hafizrahman added this to the 18.7 milestone May 16, 2024

hafizrahman marked this pull request as ready for review May 16, 2024 06:42

hafizrahman commented May 16, 2024

View reviewed changes

WooCommerce/Classes/ViewRelated/Dashboard/Reviews/ReviewsDashboardCard.swift Outdated Show resolved Hide resolved

hafizrahman requested review from selanthiraiyan and itsmeichigo May 16, 2024 06:47

hafizrahman added 4 commits May 16, 2024 13:50

shorten name.

12708f7

Simplify siteID predicate.

cb58f27

Set prefix with constant.

4e8571f

Add shimmer/redact to review items too when sync is happening.

2b2151b

hafizrahman commented May 16, 2024

View reviewed changes

selanthiraiyan self-assigned this May 16, 2024

Delete dummy data

3246160

selanthiraiyan reviewed May 16, 2024

View reviewed changes

hafizrahman added 2 commits May 16, 2024 19:02

Update incorrect dummy data usage.

34094ec

Update func name.

4fa99a1

hafizrahman commented May 16, 2024

View reviewed changes

WooCommerce/Classes/ViewRelated/Dashboard/Reviews/ReviewsDashboardCard.swift Show resolved Hide resolved

hafizrahman added 3 commits May 17, 2024 09:02

Update logic to get related products locally first, then fetch if nee…

4ad2e89

…ded.

Fix viewModel not being @ObservedObject

23839f6

Simplify ForEach since we don't need index, just check last item.

3d9286c

Update the way reviewText snippet is shown in the swiftUI way.

9aec7bc

hafizrahman requested a review from selanthiraiyan May 17, 2024 03:34

More color fix and updates.

382f342

hafizrahman commented May 17, 2024

View reviewed changes

WooCommerce/Classes/ViewRelated/Dashboard/Reviews/ReviewsDashboardCard.swift Outdated Show resolved Hide resolved

selanthiraiyan reviewed May 17, 2024

View reviewed changes

itsmeichigo modified the milestones: 18.7, 18.8 May 17, 2024

hafizrahman added 2 commits May 17, 2024 16:41

Merge branch 'trunk'

702678f

Conflicts: WooCommerce/Classes/ViewRelated/Dashboard/DashboardViewModel.swift

hafizrahman requested a review from selanthiraiyan May 17, 2024 09:52

Update review text to use AttributedString.

ae19a09

selanthiraiyan approved these changes May 17, 2024

View reviewed changes

hafizrahman and others added 3 commits May 20, 2024 10:23

Update WooCommerce/Classes/ViewRelated/Dashboard/Reviews/ReviewsDashb…

6cb627b

…oardCardViewModel.swift let/var fix Co-authored-by: Sharma Elanthiraiyan <sharma.elanthiraiyan@automattic.com>

Add fetch limit to the products results controller initializer

37fc482

Refactor remote fetch functions that do not need to return results.

40bb44a

hafizrahman enabled auto-merge May 20, 2024 03:46

hafizrahman merged commit c262c13 into trunk May 20, 2024
22 checks passed

hafizrahman deleted the feat/12742-review-card-functionality branch May 20, 2024 04:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dynamic Dashboard: Reviews Card Reload Functionality #12752

Dynamic Dashboard: Reviews Card Reload Functionality #12752

hafizrahman commented May 16, 2024 •

edited

Loading

wpmobilebot commented May 16, 2024 •

edited

Loading

hafizrahman May 16, 2024 •

edited

Loading

selanthiraiyan May 16, 2024

hafizrahman May 17, 2024

selanthiraiyan left a comment

selanthiraiyan May 16, 2024

selanthiraiyan left a comment

selanthiraiyan left a comment

selanthiraiyan May 17, 2024

hafizrahman May 20, 2024

selanthiraiyan May 17, 2024

hafizrahman May 20, 2024

selanthiraiyan May 17, 2024

hafizrahman May 20, 2024

selanthiraiyan May 17, 2024

hafizrahman May 20, 2024

Dynamic Dashboard: Reviews Card Reload Functionality #12752

Dynamic Dashboard: Reviews Card Reload Functionality #12752

Conversation

hafizrahman commented May 16, 2024 • edited Loading

Description

Testing instructions

Screenshots

wpmobilebot commented May 16, 2024 • edited Loading

hafizrahman May 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

selanthiraiyan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

selanthiraiyan left a comment

Choose a reason for hiding this comment

selanthiraiyan 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

hafizrahman commented May 16, 2024 •

edited

Loading

wpmobilebot commented May 16, 2024 •

edited

Loading

hafizrahman May 16, 2024 •

edited

Loading