-
Notifications
You must be signed in to change notification settings - Fork 37
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
Conversation
There was a problem hiding this 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( |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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( |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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? 🙂
There was a problem hiding this comment.
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( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above.
There was a problem hiding this 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.
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: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.