Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Display "Origin" column in Orders table in Orders Analytics #46424
Display "Origin" column in Orders table in Orders Analytics #46424
Changes from 19 commits
a9d90f3
1a48761
70a490b
d54ba36
ffdef81
2e26c5f
494e684
46c52cd
aaca142
286f3bd
0ee9a7a
494b36c
f5a1557
ddda7cd
ff5f4c7
2ab21c6
930c23b
b98ddd1
d0911a0
9851886
f3099a7
e1589b7
e496387
d03e47f
9c82c3f
a84b2fe
73361f4
58ad4b2
713b653
cc3d0e0
ad770ca
c787b0c
63eeaa6
79eda32
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
📜
I took quite a bit of time to analyse this approach. I was inclined to use
wc_get_orders
instead of direct SQL. The advantages of usingwc_get_orders
are that we don't have to worry aboutHPOS
compatibility, we have theWC_Order
object giving us more information to work with, and our code is a bit cleaner.I also reached out to @vedanshujain and asked his opinion. He said both approaches are acceptable.
I did a simple benchmark test using
microtime
, and I can see using direct SQL is slightly faster thanwc_get_orders
. In the context of analytics, I am inclined towards faster results than cleaner code. Let's move forward with this approach and we can tweak it if required at a later stage.What are your thoughts?
cc: @vedanshujain
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, I thought about using
wc_get_orders
too (reference: https://github.com/woocommerce/woocommerce/wiki/wc_get_orders-and-WC_Order_Query), and I was also thinking about HPOS compatibility.However, there were two things that I thought about, that I ended up using direct SQL.
One, I'm not sure if
wc_get_orders
might give us too much data ("fat" orders) and cause us issues in production. In our development environment, it's usually pretty simple small order objects; but I'm concerned that in the real world, merchants might be using some plugins or extensions that stores a lot of metadata into an order object. If we usewc_get_orders
, it would be the first time we retrieve orders object in this Orders Analytics page. Even if the users choose page size 25 (the smallest page size), I'm concerned that it might cause performance issue or "out of memory" issue etc. In other words, I would like to be safe, and use direct SQL to retrieve just the data we need quickly.Two, I see that we already have existing prior art of using direct SQL in populating other
extended_info
properties (product, customer, and coupon), so I just follow the existing way.Just sharing an info that I found in https://www.php.net/manual/en/function.microtime.php: "For performance measurements, using hrtime() is recommended."
But no worry, I guess using
microtime
is fine as a rough gauge.Related to my comments above about "fat" orders. Using direct SQL is slightly faster in our development environment. I'm thinking that in the real world merchant's WooCommerce store production environment, it may be significantly faster compared to using
wc_get_orders
.