Refactoring logic for displaying orders on the front-end account page

app/assets/javascripts/darkswarm/controllers/distributor_node_controller.js.coffee

@@ -1,9 +0,0 @@
-Darkswarm.controller "DistributorNodeCtrl", ($scope, HashNavigation, $anchorScroll) ->
-  $scope.toggle = ->
-    HashNavigation.toggle $scope.distributor.hash
-
-  $scope.open = ->
-    HashNavigation.active($scope.distributor.hash)
-
-  if $scope.open()
-    $anchorScroll()

app/assets/javascripts/darkswarm/controllers/shop_node_controller.js.coffee

@@ -0,0 +1,9 @@
+Darkswarm.controller "ShopNodeCtrl", ($scope, HashNavigation, $anchorScroll) ->
+  $scope.toggle = ->
+    HashNavigation.toggle $scope.shop.hash
+
+  $scope.open = ->
+    HashNavigation.active($scope.shop.hash)
+
+  if $scope.open()
+    $anchorScroll()

app/assets/javascripts/darkswarm/services/orders.js.coffee

@@ -1,22 +1,25 @@
-Darkswarm.factory 'Orders', (orders_by_distributor, currencyConfig, CurrentHub, Taxons, Dereferencer, visibleFilter, Matcher, Geo, $rootScope)->
+Darkswarm.factory 'Orders', (orders, shops, currencyConfig)->
   new class Orders
+    all: orders
+    changeable: []
+    shops: shops
+    shopsByID: {}
+    currencySymbol = currencyConfig.symbol
+
     constructor: ->
-      # Populate Orders.orders from json in page.
-      @orders_by_distributor = orders_by_distributor
-      @changeable_orders = []
-      @currency_symbol = currencyConfig.symbol
+      for shop in @shops
+        shop.orders = []
+        shop.balance = 0.0
+        @shopsByID[shop.id] = shop
 
-      for distributor in @orders_by_distributor
-        @findChangeableOrders(distributor.distributed_orders)
-        @updateRunningBalance(distributor.distributed_orders)
+      for order in @all by -1
+        shop = @shopsByID[order.shop_id]
+        shop.orders.unshift order
 
+        @changeable.unshift(order) if order.changes_allowed
 
-    updateRunningBalance: (orders) ->
-      for order, i in orders
-        balances = orders.slice(i,orders.length).map (o) -> parseFloat(o.outstanding_balance)
-        running_balance = balances.reduce (a,b) -> a+b
-        order.running_balance = running_balance.toFixed(2)
+        @updateRunningBalance(shop, order)
 
-    findChangeableOrders: (orders) ->
-      for order in orders when order.changes_allowed
-        @changeable_orders.push(order)
+    updateRunningBalance: (shop, order) ->
+      shop.balance += parseFloat(order.outstanding_balance)
+      order.runningBalance = shop.balance.toFixed(2)

app/controllers/spree/users_controller_decorator.rb

@@ -4,6 +4,16 @@ Spree::UsersController.class_eval do
   before_filter :enable_embedded_shopfront
   before_filter :set_credit_card, only: :show
 
+  # Override of spree_auth_devise default
+  # Ignores invoice orders, only order where state: 'complete'
+  def show
+    @orders = @user.orders.where(state: 'complete').order('completed_at desc')
+
+    if Spree::Config.accounts_distributor_id
+      @orders = @orders.where('distributor_id != ?', Spree::Config.accounts_distributor_id)
+    end
+  end
+
   private
 
   def set_credit_card

app/helpers/injection_helper.rb

@@ -64,9 +64,13 @@ module InjectionHelper
     render partial: "json/injection_ams", locals: {name: 'enterpriseAttributes', json: "#{@enterprise_attributes.to_json}"}
   end
 
-  def inject_orders_by_distributor
-    data_array = spree_current_user.orders_by_distributor
-    inject_json_ams "orders_by_distributor", data_array, Api::OrdersByDistributorSerializer
+  def inject_orders
+    inject_json_ams "orders", @orders.all, Api::OrderSerializer
+  end
+
+  def inject_shops
+    shops = Enterprise.where(id: @orders.pluck(:distributor_id).uniq)
+    inject_json_ams "shops", shops.all, Api::ShopForOrdersSerializer
   end
 
   def inject_saved_credit_cards

app/models/spree/user_decorator.rb

@@ -54,32 +54,6 @@ Spree.user_class.class_eval do
     owned_enterprises(:reload).size < enterprise_limit
   end
 
-  # Returns Enterprise IDs for distributors that the user has shopped at
-  def enterprises_ordered_from
-    enterprise_ids = orders.where(state: :complete).map(&:distributor_id).uniq
-    # Exclude the accounts distributor
-    if Spree::Config.accounts_distributor_id
-      enterprise_ids = enterprise_ids.keep_if { |a| a != Spree::Config.accounts_distributor_id }
-    end
-    enterprise_ids
-  end
-
-  # Returns orders and their associated payments for all distributors that have been ordered from
-  def complete_orders_by_distributor
-    Enterprise
-      .includes(distributed_orders: { payments: :payment_method })
-      .where(enterprises: { id: enterprises_ordered_from },
-             spree_orders: { state: 'complete', user_id: id })
-      .order('spree_orders.completed_at DESC')
-  end
-
-  def orders_by_distributor
-    # Remove uncompleted payments as these will not be reflected in order balance
-    data_array = complete_orders_by_distributor.to_a
-    remove_payments_in_checkout(data_array)
-    data_array.sort! { |a, b| b.distributed_orders.length <=> a.distributed_orders.length }
-  end
-
   private
 
   def limit_owned_enterprises

app/serializers/api/order_serializer.rb

@@ -1,13 +1,18 @@
 module Api
   class OrderSerializer < ActiveModel::Serializer
     attributes :number, :completed_at, :total, :state, :shipment_state, :payment_state
-    attributes :outstanding_balance, :payments, :path, :cancel_path, :changes_allowed, :changes_allowed_until
-    attributes :shop_name, :item_count
+    attributes :outstanding_balance, :payments, :path, :cancel_path
+    attributes :changes_allowed, :changes_allowed_until, :item_count
+    attributes :shop_id
 
     has_many :payments, serializer: Api::PaymentSerializer
 
-    def shop_name
-      object.distributor.andand.name
+    def payments
+      object.payments.joins(:payment_method).completed
+    end
+
+    def shop_id
+      object.distributor_id
     end
 
     def item_count

app/serializers/api/shop_for_orders_serializer.rb

@@ -0,0 +1,13 @@
+module Api
+  class ShopForOrdersSerializer < ActiveModel::Serializer
+    attributes :id, :name, :hash, :logo
+
+    def hash
+      object.to_param
+    end
+
+    def logo
+      object.logo(:small) if object.logo?
+    end
+  end
+end

app/views/spree/users/_fat.html.haml

@@ -9,16 +9,7 @@
         %th.order5.text-right= t :value
         %th.order6.text-right.show-for-large-up= t :outstanding_balance
         %th.order7.text-right= t :running_balance
-      %tbody.transaction-group{"ng-repeat" => "order in distributor.distributed_orders", "ng-class-odd"=>"'odd'", "ng-class-even"=>"'even'"}
-        %tr.order-row
-          %td.order1
-            %a{"ng-href" => "{{::order.path}}", "ng-bind" => "::('order' | t )+ ' ' + order.number"}
-          %td.order2{"ng-bind" => "::order.completed_at"}
-          %td.order3.show-for-large-up{"ng-bind" => "::'spree.payment_states.' + order.payment_state | t | capitalize"}
-          %td.order4.show-for-large-up{"ng-bind" => "::'spree.shipment_states.' + order.shipment_state | t | capitalize"}
-          %td.order5.text-right{"ng-class" => "{'credit' : order.total < 0, 'debit' : order.total > 0, 'paid' : order.total == 0}","ng-bind" => "::order.total | localizeCurrency"}
-          %td.order6.text-right.show-for-large-up{"ng-class" => "{'credit' : order.outstanding_balance < 0, 'debit' : order.outstanding_balance > 0, 'paid' : order.outstanding_balance == 0}", "ng-bind" => "::order.outstanding_balance | localizeCurrency"}
-          %td.order7.text-right{"ng-class" => "{'credit' : order.running_balance < 0, 'debit' : order.running_balance > 0, 'paid' : order.running_balance == 0}", "ng-bind" => "::order.running_balance | localizeCurrency"}
+      %tbody.transaction-group{"ng-repeat" => "order in shop.orders", "ng-class-odd"=>"'odd'", "ng-class-even"=>"'even'"}
         %tr.payment-row{"ng-repeat" => "payment in order.payments", "ng-class" => "{'invalid': payment.state != 'completed'}"}
           %td.order1{"ng-bind" => "::payment.payment_method"}
           %td.order2{"ng-bind" => "::payment.updated_at"}
@@ -29,3 +20,12 @@
           %td.order5.text-right{"ng-class" => "{'credit' : payment.amount > 0, 'debit' : payment.amount < 0, 'paid' : payment.amount == 0}","ng-bind" => "::payment.amount | localizeCurrency"}
           %td.order6.show-for-large-up
           %td.order7
+        %tr.order-row
+          %td.order1
+            %a{"ng-href" => "{{::order.path}}", "ng-bind" => "::('order' | t )+ ' ' + order.number"}
+          %td.order2{"ng-bind" => "::order.completed_at"}
+          %td.order3.show-for-large-up{"ng-bind" => "::'spree.payment_states.' + order.payment_state | t | capitalize"}
+          %td.order4.show-for-large-up{"ng-bind" => "::'spree.shipment_states.' + order.shipment_state | t | capitalize"}
+          %td.order5.text-right{"ng-class" => "{'credit' : order.total < 0, 'debit' : order.total > 0, 'paid' : order.total == 0}","ng-bind" => "::order.total | localizeCurrency"}
+          %td.order6.text-right.show-for-large-up{"ng-class" => "{'credit' : order.outstanding_balance < 0, 'debit' : order.outstanding_balance > 0, 'paid' : order.outstanding_balance == 0}", "ng-bind" => "::order.outstanding_balance | localizeCurrency"}
+          %td.order7.text-right{"ng-class" => "{'credit' : order.runningBalance < 0, 'debit' : order.runningBalance > 0, 'paid' : order.runningBalance == 0}", "ng-bind" => "::order.runningBalance | localizeCurrency"}

app/views/spree/users/_open_orders.html.haml

@@ -9,12 +9,12 @@
         %th.order5.text-right= t('.total')
         %th.order6.text-right.show-for-large-up= t('.edit')
         %th.order7.text-right= t('.cancel')
-      %tbody.transaction-group{"ng-repeat" => "order in Orders.changeable_orders", "ng-class-odd"=>"'odd'", "ng-class-even"=>"'even'"}
+      %tbody.transaction-group{"ng-repeat" => "order in Orders.changeable", "ng-class-odd"=>"'odd'", "ng-class-even"=>"'even'"}
         %tr.order-row
           %td.order1
             %a{"ng-href" => "{{::order.path}}", "ng-bind" => "::order.number"}
-          %td.order2{"ng-bind" => "::order.shop_name"}
-          %td.order3.show-for-large-up{"ng-bind" => "order.changes_allowed_until"}
+          %td.order2{"ng-bind" => "::Orders.shopsByID[order.shop_id].name"}
+          %td.order3.show-for-large-up{"ng-bind" => "::order.changes_allowed_until"}
           %td.order4.show-for-large-up{"ng-bind" => "::order.item_count"}
           %td.order5.text-right{"ng-class" => "{'credit' : order.total < 0, 'debit' : order.total > 0, 'paid' : order.total == 0}","ng-bind" => "::order.total | localizeCurrency"}
           %td.order6.text-right.show-for-large-up.brick

app/views/spree/users/_orders.html.haml

@@ -1,10 +1,10 @@
 %script{ type: "text/ng-template", id: "account/orders.html" }
   .orders{"ng-controller" => "OrdersCtrl", "ng-cloak" => true}
-    .my-open-orders{ ng: { show: 'Orders.changeable_orders.length > 0' } }
+    .my-open-orders{ ng: { show: 'Orders.changeable.length > 0' } }
       %h3= t(:open_orders)
       = render 'open_orders'
 
     .past-orders
       %h3= t(:past_orders)
       -# = render 'past_orders'
-    .message{"ng-if" => "Orders.changeable_orders.length == 0", "ng-bind" => "::'you_have_no_orders_yet' | t"}
+    .message{"ng-if" => "Orders.all.length == 0", "ng-bind" => "::'you_have_no_orders_yet' | t"}

app/views/spree/users/_skinny.html.haml

@@ -1,10 +1,10 @@
 .row.active_table_row.skinny-head.margin-top{"ng-click" => "toggle($event)", "ng-class" => "{'closed' : !open()}"}
   .columns.small-2
-    %img.margin-top.account-logo{"logo-fallback" => true, "ng-src" => "{{distributor.logo}}"}
+    %img.margin-top.account-logo{"logo-fallback" => true, "ng-src" => "{{shop.logo}}"}
   .columns.small-5
-    %h3.margin-top{"ng-bind" => "::distributor.name"}
+    %h3.margin-top{"ng-bind" => "::shop.name"}
   .columns.small-4.text-right
-    %h3.margin-top.distributor-balance{"ng-bind" => "::distributor.balance | formatBalance", "ng-class" => "{'credit' : distributor.balance < 0, 'debit' : distributor.balance > 0, 'paid' : distributor.balance == 0}" }
+    %h3.margin-top.distributor-balance{"ng-bind" => "::shop.balance | formatBalance", "ng-class" => "{'credit' : shop.balance < 0, 'debit' : shop.balance > 0, 'paid' : shop.balance == 0}" }
   .columns.small-1.text-right
     %h3.margin-top
       %i{"ng-class" => "{'ofn-i_005-caret-down' : !open(), 'ofn-i_006-caret-up' : open()}"}

app/views/spree/users/_transactions.html.haml

@@ -1,11 +1,11 @@
 %script{ type: "text/ng-template", id: "account/transactions.html" }
   .active_table.orders{"ng-controller" => "OrdersCtrl", "ng-cloak" => true}
     %h3.my-orders= t(:transaction_history)
-    %distributor.active_table_node.row.animate-repeat{"ng-if" => "Orders.orders_by_distributor.length > 0", "ng-repeat" => "(key, distributor) in Orders.orders_by_distributor",
-    "ng-controller" => "DistributorNodeCtrl",
-    "ng-class" => "{'closed' : !open(), 'open' : open(), 'inactive' : !distributor.active}",
-    id: "{{distributor.hash}}"}
+    %distributor.active_table_node.row.animate-repeat{"ng-if" => "Orders.shops.length > 0", "ng-repeat" => "shop in Orders.shops",
+    "ng-controller" => "ShopNodeCtrl",
+    "ng-class" => "{'closed' : !open(), 'open' : open(), 'inactive' : !shop.active}",
+    id: "{{shop.hash}}"}
       .small-12.columns
         = render partial: "spree/users/skinny"
         = render partial: "spree/users/fat"
-  .message{"ng-if" => "Orders.orders_by_distributor.length == 0", "ng-bind" => "::'you_have_no_orders_yet' | t"}
+  .message{"ng-if" => "Orders.shops.length == 0", "ng-bind" => "::'you_have_no_orders_yet' | t"}

app/views/spree/users/show.html.haml

@@ -1,5 +1,6 @@
 .darkswarm
-  = inject_orders_by_distributor
+  = inject_orders
+  = inject_shops
   = inject_saved_credit_cards
 
   :javascript

spec/controllers/spree/users_controller_spec.rb

@@ -0,0 +1,48 @@
+require 'spec_helper'
+
+describe Spree::UsersController do
+  include AuthenticationWorkflow
+
+  describe "show" do
+    let!(:u1) { create(:user) }
+    let!(:u2) { create(:user) }
+    let!(:distributor1) { create(:distributor_enterprise) }
+    let!(:distributor2) { create(:distributor_enterprise) }
+    let!(:d1o1) { create(:completed_order_with_totals, distributor: distributor1, user_id: u1.id) }
+    let!(:d1o2) { create(:completed_order_with_totals, distributor: distributor1, user_id: u1.id) }
+    let!(:d1_order_for_u2) { create(:completed_order_with_totals, distributor: distributor1, user_id: u2.id) }
+    let!(:d1o3) { create(:order, state: 'cart', distributor: distributor1, user_id: u1.id) }
+    let!(:d2o1) { create(:completed_order_with_totals, distributor: distributor2, user_id: u2.id) }
+    let!(:accounts_distributor) {create :distributor_enterprise}
+    let!(:order_account_invoice) { create(:order, distributor: accounts_distributor, state: 'complete', user: u1) }
+
+    let(:orders) { assigns(:orders) }
+    let(:shops) { Enterprise.where(id: orders.pluck(:distributor_id)) }
+
+    before do
+      Spree::Config.set({ accounts_distributor_id: accounts_distributor.id })
+      allow(controller).to receive(:spree_current_user) { u1 }
+    end
+
+    it "returns orders placed by the user at normal shops" do
+      spree_get :show
+
+      expect(orders).to eq [d1o1, d1o2]
+      expect(shops).to include distributor1
+
+      # Doesn't return orders belonging to the accounts distributor" do
+      expect(orders).to_not include order_account_invoice
+      expect(shops).to_not include accounts_distributor
+
+      # Doesn't return orders for irrelevant distributors" do
+      expect(orders).not_to include d2o1
+      expect(shops).not_to include distributor2
+
+      # Doesn't return other users' orders" do
+      expect(orders).not_to include d1_order_for_u2
+
+      # Doesn't return uncompleted orders" do
+      expect(orders).not_to include d1o3
+    end
+  end
+end

spec/features/consumer/account_spec.rb

@@ -40,6 +40,8 @@ feature %q{
         # No distributors allow changes to orders
         expect(page).to_not have_content I18n.t('spree.users.show.open_orders')
 
+        click_link I18n.t('spree.users.show.tabs.transactions')
+
         # It shows all hubs that have been ordered from with balance or credit
         expect(page).to have_content distributor1.name
         expect(page).to have_content distributor2.name

spec/models/spree/user_spec.rb

@@ -102,49 +102,4 @@ describe Spree.user_class do
       end
     end
   end
-
-  describe "retrieving orders for /account page" do
-    let!(:u1) { create(:user) }
-    let!(:u2) { create(:user) }
-    let!(:distributor1) { create(:distributor_enterprise) }
-    let!(:distributor2) { create(:distributor_enterprise) }
-    let!(:d1o1) { create(:completed_order_with_totals, distributor: distributor1, user_id: u1.id) }
-    let!(:d1o2) { create(:completed_order_with_totals, distributor: distributor1, user_id: u1.id) }
-    let!(:d1_order_for_u2) { create(:completed_order_with_totals, distributor: distributor1, user_id: u2.id) }
-    let!(:d1o3) { create(:order, state: 'cart', distributor: distributor1, user_id: u1.id) }
-    let!(:d2o1) { create(:completed_order_with_totals, distributor: distributor2, user_id: u2.id) }
-    let!(:accounts_distributor) {create :distributor_enterprise}
-    let!(:order_account_invoice) { create(:order, distributor: accounts_distributor, state: 'complete', user: u1) }
-
-    let!(:completed_payment) { create(:payment, order: d1o1, state: 'completed') }
-    let!(:payment) { create(:payment, order: d1o2, state: 'checkout') }
-
-    before do
-      Spree::Config.accounts_distributor_id = accounts_distributor.id
-    end
-
-    it "returns enterprises that the user has ordered from, excluding accounts distributor" do
-      expect(u1.enterprises_ordered_from).to eq [distributor1.id]
-    end
-
-    it "returns orders and payments for the user, organised by distributor" do
-      expect(u1.orders_by_distributor).to include distributor1
-      expect(u1.orders_by_distributor.first.distributed_orders).to include d1o1
-    end
-
-    it "doesn't return irrelevant distributors" do
-      expect(u1.orders_by_distributor).not_to include distributor2
-    end
-    it "doesn't return other users' orders" do
-      expect(u1.orders_by_distributor.first.distributed_orders).not_to include d1_order_for_u2
-    end
-
-    it "doesn't return uncompleted orders" do
-      expect(u1.orders_by_distributor.first.distributed_orders).not_to include d1o3
-    end
-
-    it "doesn't return payments that are still at checkout stage" do
-      expect(u1.orders_by_distributor.first.distributed_orders.map{|o| o.payments}.flatten).not_to include payment
-    end
-  end
 end

spec/serializers/order_serializer_spec.rb

@@ -4,6 +4,8 @@ describe Api::OrderSerializer do
   let(:serializer) { Api::OrderSerializer.new order }
   let(:order) { create(:completed_order_with_totals) }
 
+  let!(:completed_payment) { create(:payment, order: order, state: 'completed', amount: 43.21) }
+  let!(:payment) { create(:payment, order: order, state: 'checkout', amount: 123.45) }
 
   it "serializes an order" do
     expect(serializer.to_json).to match order.number.to_s
@@ -14,4 +16,8 @@ describe Api::OrderSerializer do
     expect(serializer.to_json).to match "balance_due"
   end
 
+  it "only serializes completed payments" do
+    expect(serializer.to_json).to match completed_payment.amount.to_s
+    expect(serializer.to_json).to_not match payment.amount.to_s
+  end
 end

spec/serializers/orders_by_distributor_serializer_spec.rb

@@ -1,28 +0,0 @@
-require 'spec_helper'
-
-describe Api::OrdersByDistributorSerializer do
-
-  # Banged lets ensure entered into test database
-  let!(:distributor1) { create(:distributor_enterprise) }
-  let!(:distributor2) { create(:distributor_enterprise) }
-  let!(:user) { create(:user)}
-  let!(:d1o1) { create(:completed_order_with_totals, distributor: distributor1, user_id: user.id, total: 10000)}
-  let!(:d1o2) { create(:completed_order_with_totals, distributor: distributor1, user_id: user.id, total: 5000)}
-  let!(:d2o1) { create(:completed_order_with_totals, distributor: distributor2, user_id: user.id)}
-
-  before do
-    @data = Enterprise.includes(:distributed_orders).where(enterprises: {id: user.enterprises_ordered_from }, spree_orders: {state: :complete, user_id: user.id}).to_a
-    @serializer = ActiveModel::ArraySerializer.new(@data, {each_serializer: Api::OrdersByDistributorSerializer})
-  end
-
-  it "serializes orders" do
-    expect(@serializer.to_json).to match "distributed_orders"
-  end
-
-  it "serializes the balance for each distributor" do
-    expect(@serializer.serializable_array[0].keys).to include :balance
-    # Would be good to test adding up balance properly but can't get a non-zero total from the factories...
-    expect(@serializer.serializable_array[0][:balance]).to eq "0.00"
-  end
-
-end