Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/6619 limit stats refresh #2526

Merged
merged 21 commits into from
Oct 6, 2022

Conversation

JorgeMucientes
Copy link
Contributor

@JorgeMucientes JorgeMucientes commented Sep 17, 2022

Part of: #6618

Description

wc-analytics endpoint is one of the slowest endpoints we hit from the app, and is one that we hit a lot. We fetch site stats and top performers every time we open My Store tab and when we change between tabs. This translates into the "slow/endless loading" perception of the app. This PR alongside changes applied in this other PR in woocommerce-android aim to reduce that loading "slowness" for top performer products data with the following changes:

  • Migrate top performers cache to Room to easily expose an observable
  • Simplify top performers cached model to avoid having to parse multiple JSON or query different SQL tables to load top performers from cache
  • Invalidate top performers cache when a new order is placed
  • Invalidate top performers cache after 1h. Maybe this is to often? please share your thoughts

Finally, I update all broken unit tests, remove some that did not apply anymore and added a couple more.

Testing instructions

This can be tested alongside the changes from WooCommerce Android. Please follow the test instructions from this PR.

Copy link
Member

@hichamboushaba hichamboushaba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @JorgeMucientes for working on this, I left some questions about blocking functions that need to be answered before approving this.

In the meantime I'll test the functionality in WCAndroid.

private var mapper: WCProductLeaderboardsMapper = mock()
private val topPerformersDao: TopPerformerProductsDao = mock()

private var storeUnderTest = WCLeaderboardsStore(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

very np 😄, I noticed some tests call createStoreUnderTest, and some don't.

This is a little confusing, I think the reason is that sometimes we want to setup the mocks before creating the instance.

A small suggestion, but you don't have to implement it: remove the @before from setup, and add a mocks builder argument to it, then call it always, and make sure it calls createStoreUnderTest (similar to this from the WC app), this would make things consistent, always start with setup, but without thinking about a second instantiation of storeUnderTest

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice approach! I like it. I've applied it as I wanted to see it in action. In the end for this specific case it ended up being a bit more verbose, as I had to add an additional setup() call to all tests, when only one test needed to override the current instance of storeUnderTest

But I'll keep the changes as I think is consistent and I think is a cleaner way to defer SUT creation to override mocks.

Thanks for the tip 🙏🏼


@Transaction
@Query("SELECT * FROM TopPerformerProducts WHERE granularity = :granularity AND siteId = :siteId")
abstract fun getTopPerformerProductsFor(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be a suspendable function to make sure it's using the correct background thread?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, good catch!


@Dao
abstract class TopPerformerProductsDao {
@Transaction
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason to mark this and the following functions as transactions? 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not in these cases. Removed, good catch!
Those transactions are leftovers when my initial approach was to have 2 different tables for top performers' data, one for aggregated top performer data and one other for the product details. Similar to the approach we had prior to the refactor, although product details were stores as json string. Then I noticed I didn't need any of that.


@Transaction
@Query("SELECT * FROM TopPerformerProducts WHERE siteId = :siteId")
abstract fun getTopPerformerProductsForSite(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above.

Copy link
Member

@hichamboushaba hichamboushaba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks for the fixes @JorgeMucientes.

I'll leave merging this to you, to be able to coordinate between the two PRs.

@JorgeMucientes JorgeMucientes merged commit 6459816 into trunk Oct 6, 2022
@JorgeMucientes JorgeMucientes deleted the feature/6619-limit-stats-refresh branch October 6, 2022 14:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants