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

Fix access of Spree namespaces #13

Open
wants to merge 5 commits into
base: rails-6-support
Choose a base branch
from

Conversation

mayankdedhia
Copy link

Add a fix to access the Spree namespace

@mayankdedhia mayankdedhia changed the title Fix access of Spree namespace in reports Fix access of Spree namespaces Aug 19, 2020
@orders = @search.result

supplier_earnings_map = @orders.map(&:supplier_earnings_map)
grouped_suppliers_map = supplier_earnings_map.flatten.group_by(&:name).values
grouped_suppliers_map = supplier_earnings_map.flatten.group_by { |s| s[:name] }.values
Copy link

Choose a reason for hiding this comment

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

here in case that the suppliers aren't present, maybe we should use:

supplier_earnings_map.flatten.group_by { |s| s[:name] }&.values

Copy link
Author

Choose a reason for hiding this comment

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

Let me test that first and then change it

Copy link

@jtapia jtapia left a comment

Choose a reason for hiding this comment

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

Besides my comment, LGTM

get :earnings
post :earnings
get :earnings
post :earnings
Copy link
Member

Choose a reason for hiding this comment

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

Interesting, is the current report sending the form as a post request? this should be GET so it can be bookmarkable

@@ -1,6 +1,6 @@
# frozen_string_literal: true

require 'csv'
require "csv"
Copy link
Member

Choose a reason for hiding this comment

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

we try to use double quotes only whenever we need to interpolate variables, leaving the rest as single quotes

Copy link
Author

Choose a reason for hiding this comment

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

That is my editor playing with me

def supplier_total_sales
params[:q] = search_params

@search = ::Spree::Order.complete.not_canceled.ransack(params[:q])
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 any default params? like only last 30 days, I can see some performance degradation over time with more and more data being computed

Copy link
Author

Choose a reason for hiding this comment

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

Makes sense to add a range, will update that

@jtapia jtapia self-requested a review September 7, 2020 16:42
def supplier_total(user_or_supplier)
def supplier_total_sales(user_or_supplier)
supplier = user_or_supplier.is_a?(::Spree::Supplier) ? user_or_supplier : user_or_supplier.supplier
shipments = self.shipments.by_supplier(supplier)
Copy link

Choose a reason for hiding this comment

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

optional:
you can avoid using self

suppliers.map do |s|
{
name: s.name,
total_sales: self.supplier_total_sales(s),
Copy link

Choose a reason for hiding this comment

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

optional:
same here


context 'when passed a supplier' do
it 'returns the total commission earned for the order for a given supplier' do
context "when passed a supplier" do
Copy link

Choose a reason for hiding this comment

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

Same here as @softr8 has recommended, I would suggest to keep the single quotes

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.

3 participants