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

best_selling_variants and top_grossing_variants are now calculated properly #718

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@tomash
Contributor

tomash commented Oct 26, 2011

Admin::OverviewController, best_selling_variants and top_grossing_variants functions.

In best_selling_variants. Previous code that selects line_items (and sums by quantity) for building up a list of top-selling products:

li = LineItem.includes(:order).where("orders.state = 'complete'").sum(:quantity, :group => :variant_id, :limit => 5)

...doesn't work as expected: yes it does return "sum of quantity" of 5 lineitems grouped by variant_id. But they are not the top-selling products. These can be achieved by adding a proper ORDER clause:

li = LineItem.includes(:order).where("orders.state = 'complete'").order("SUM(line_items.quantity) DESC").sum(:quantity, :group => :variant_id, :limit => 5)

It's similar in top_grossing_variants, with a slight twist: we can (we have to) get away with just a single query, although a pretty complex one (counting sums of price-by-quantity multiplies):

def top_grossing_variants
  total_sold_prices = LineItem.includes(:order).where("orders.state = 'complete'").order("SUM(line_items.quantity * line_items.price) DESC").sum("price * quantity", :group => :variant_id, :limit => 5)
  variants = total_sold_prices.map do |v|
    variant = Variant.find(v[0])
    [variant.name, v[1]]
  end

  variants.sort { |x,y| y[1] <=> x[1] }
end

This pull request fixes both methods.

The good thing: this issue, like the whole controller, seems to be pretty old code (dating from 2009), so I think you can cherry-pick this commit to older versions of spree (I think up to 0.30.x).

The bad thing: ordering by SUM will, sooner or later, become a performance hog. It's not currenly in my store (over 1k of Orders, around 3k of LineItems, almost 5k Products), but it will for amounts a few orders of magnitude bigger. So, for high-volume stores, this should be cached somewhere and recalculated on a regular basis (but not with every request to dashboard).

I haven't included any tests: it's a pretty simple and quick fix, and as far as I know the Dashboard is going to experience quite a makeover in terms of available analytics.

@schof

This comment has been minimized.

Show comment
Hide comment
@schof

schof Oct 27, 2011

Member

@LBRapid can you work up a view test for this one while you are working through the others?

Member

schof commented Oct 27, 2011

@LBRapid can you work up a view test for this one while you are working through the others?

@LBRapid

This comment has been minimized.

Show comment
Hide comment
@LBRapid

LBRapid Oct 27, 2011

Member

@schof Sure thing. I'm on it.

John Dyer
(410)-929-3423
http://twitter.com/LBRapid

On Wednesday, October 26, 2011 at 10:47 PM, Sean Schofield wrote:

@LBRapid can you work up a view test for this one while you are working through the others?

Reply to this email directly or view it on GitHub:
#718 (comment)

Member

LBRapid commented Oct 27, 2011

@schof Sure thing. I'm on it.

John Dyer
(410)-929-3423
http://twitter.com/LBRapid

On Wednesday, October 26, 2011 at 10:47 PM, Sean Schofield wrote:

@LBRapid can you work up a view test for this one while you are working through the others?

Reply to this email directly or view it on GitHub:
#718 (comment)

@schof

This comment has been minimized.

Show comment
Hide comment
@schof

schof Nov 7, 2011

Member

@LBRapid what happened to that test?

Member

schof commented Nov 7, 2011

@LBRapid what happened to that test?

@LBRapid

This comment has been minimized.

Show comment
Hide comment
@LBRapid

LBRapid Nov 9, 2011

Member

Specs are in the pull request at #744

Member

LBRapid commented Nov 9, 2011

Specs are in the pull request at #744

@schof

This comment has been minimized.

Show comment
Hide comment
@schof

schof Nov 10, 2011

Member

@tomash Do you mind creating a new pull request now that we are using namespace? I'm trying to do a bunch of things at once here and you are the expert on this fix.

Member

schof commented Nov 10, 2011

@tomash Do you mind creating a new pull request now that we are using namespace? I'm trying to do a bunch of things at once here and you are the expert on this fix.

@tomash

This comment has been minimized.

Show comment
Hide comment
@tomash

tomash Nov 10, 2011

Contributor

sure, give me a while...

Contributor

tomash commented Nov 10, 2011

sure, give me a while...

@tomash

This comment has been minimized.

Show comment
Hide comment
@tomash

tomash Nov 10, 2011

Contributor

done, #746

Contributor

tomash commented Nov 10, 2011

done, #746

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment