From 40d284812a3b99136b267654f7f2884001935112 Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Mon, 22 Feb 2021 13:12:04 +0000
Subject: [PATCH 1/9] Extract itemized contents to testable method

---
 app/controllers/spree/paypal_controller.rb | 60 ++++++++++++----------
 1 file changed, 32 insertions(+), 28 deletions(-)

diff --git a/app/controllers/spree/paypal_controller.rb b/app/controllers/spree/paypal_controller.rb
index f5ef6872e6..9460e649b6 100644
--- a/app/controllers/spree/paypal_controller.rb
+++ b/app/controllers/spree/paypal_controller.rb
@@ -13,33 +13,9 @@ module Spree
 
     def express
       order = current_order || raise(ActiveRecord::RecordNotFound)
-      items = order.line_items.map(&method(:line_item))
-
-      tax_adjustments = order.adjustments.tax.additional
-      shipping_adjustments = order.adjustments.shipping
-
-      order.adjustments.eligible.each do |adjustment|
-        next if (tax_adjustments + shipping_adjustments).include?(adjustment)
-
-        items << {
-          Name: adjustment.label,
-          Quantity: 1,
-          Amount: {
-            currencyID: order.currency,
-            value: adjustment.amount
-          }
-        }
-      end
-
-      # Because PayPal doesn't accept $0 items at all.
-      # See https://github.com/spree-contrib/better_spree_paypal_express/issues/10
-      # "It can be a positive or negative value but not zero."
-      items.reject! do |item|
-        item[:Amount][:value].zero?
-      end
 
       pp_request = provider.build_set_express_checkout(
-        express_checkout_request_details(order, items)
+        express_checkout_request_details(order)
       )
 
       begin
@@ -111,7 +87,7 @@ module Spree
       }
     end
 
-    def express_checkout_request_details(order, items)
+    def express_checkout_request_details(order)
       {
         SetExpressCheckoutRequestDetails: {
           InvoiceID: order.number,
@@ -124,7 +100,7 @@ module Spree
           LandingPage: payment_method.preferred_landing_page.presence || "Billing",
           cppheaderimage: payment_method.preferred_logourl.presence || "",
           NoShipping: 1,
-          PaymentDetails: [payment_details(items)]
+          PaymentDetails: [payment_details(order)]
         }
       }
     end
@@ -171,7 +147,8 @@ module Spree
       payment_method.provider
     end
 
-    def payment_details(items)
+    def payment_details(order)
+      items = itemized_contents(order)
       item_sum = items.sum { |i| i[:Quantity] * i[:Amount][:value] }
 
       tax_adjustments = current_order.adjustments.tax.additional
@@ -212,6 +189,33 @@ module Spree
       end
     end
 
+    def itemized_contents(order)
+      items = order.line_items.map(&method(:line_item))
+
+      tax_adjustments = order.adjustments.tax.additional
+      shipping_adjustments = order.adjustments.shipping
+
+      order.adjustments.eligible.each do |adjustment|
+        next if (tax_adjustments + shipping_adjustments).include?(adjustment)
+
+        items << {
+          Name: adjustment.label,
+          Quantity: 1,
+          Amount: {
+            currencyID: order.currency,
+            value: adjustment.amount
+          }
+        }
+      end
+
+      # Because PayPal doesn't accept $0 items at all.
+      # See https://github.com/spree-contrib/better_spree_paypal_express/issues/10
+      # "It can be a positive or negative value but not zero."
+      items.reject! do |item|
+        item[:Amount][:value].zero?
+      end
+    end
+
     def address_options
       return {} unless address_required?
 

From eaf9305a77ff2e4e69c694cd984a238999310a1c Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Mon, 22 Feb 2021 13:37:13 +0000
Subject: [PATCH 2/9] Simplify adjustments summing

---
 app/controllers/spree/paypal_controller.rb | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/app/controllers/spree/paypal_controller.rb b/app/controllers/spree/paypal_controller.rb
index 9460e649b6..bb37a6653f 100644
--- a/app/controllers/spree/paypal_controller.rb
+++ b/app/controllers/spree/paypal_controller.rb
@@ -150,9 +150,7 @@ module Spree
     def payment_details(order)
       items = itemized_contents(order)
       item_sum = items.sum { |i| i[:Quantity] * i[:Amount][:value] }
-
-      tax_adjustments = current_order.adjustments.tax.additional
-      tax_adjustments_total = tax_adjustments.sum(:amount)
+      tax_adjustments_total = current_order.all_adjustments.tax.additional.sum(:amount)
 
       if item_sum.zero?
         # Paypal does not support no items or a zero dollar ItemTotal

From 8de5ac4680c87c376a6452a1f838e958ebf34ab0 Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Mon, 22 Feb 2021 13:22:27 +0000
Subject: [PATCH 3/9] Extract itemized contents to service

---
 app/controllers/spree/paypal_controller.rb |  43 +-------
 app/services/paypal_items_builder.rb       |  53 +++++++++
 spec/services/paypal_items_builder_spec.rb | 121 +++++++++++++++++++++
 3 files changed, 176 insertions(+), 41 deletions(-)
 create mode 100644 app/services/paypal_items_builder.rb
 create mode 100644 spec/services/paypal_items_builder_spec.rb

diff --git a/app/controllers/spree/paypal_controller.rb b/app/controllers/spree/paypal_controller.rb
index bb37a6653f..e139d53908 100644
--- a/app/controllers/spree/paypal_controller.rb
+++ b/app/controllers/spree/paypal_controller.rb
@@ -74,19 +74,6 @@ module Spree
 
     private
 
-    def line_item(item)
-      {
-        Name: item.product.name,
-        Number: item.variant.sku,
-        Quantity: item.quantity,
-        Amount: {
-          currencyID: item.order.currency,
-          value: item.price
-        },
-        ItemCategory: "Physical"
-      }
-    end
-
     def express_checkout_request_details(order)
       {
         SetExpressCheckoutRequestDetails: {
@@ -148,7 +135,8 @@ module Spree
     end
 
     def payment_details(order)
-      items = itemized_contents(order)
+      items = PaypalItemsBuilder.new(order).call
+
       item_sum = items.sum { |i| i[:Quantity] * i[:Amount][:value] }
       tax_adjustments_total = current_order.all_adjustments.tax.additional.sum(:amount)
 
@@ -187,33 +175,6 @@ module Spree
       end
     end
 
-    def itemized_contents(order)
-      items = order.line_items.map(&method(:line_item))
-
-      tax_adjustments = order.adjustments.tax.additional
-      shipping_adjustments = order.adjustments.shipping
-
-      order.adjustments.eligible.each do |adjustment|
-        next if (tax_adjustments + shipping_adjustments).include?(adjustment)
-
-        items << {
-          Name: adjustment.label,
-          Quantity: 1,
-          Amount: {
-            currencyID: order.currency,
-            value: adjustment.amount
-          }
-        }
-      end
-
-      # Because PayPal doesn't accept $0 items at all.
-      # See https://github.com/spree-contrib/better_spree_paypal_express/issues/10
-      # "It can be a positive or negative value but not zero."
-      items.reject! do |item|
-        item[:Amount][:value].zero?
-      end
-    end
-
     def address_options
       return {} unless address_required?
 
diff --git a/app/services/paypal_items_builder.rb b/app/services/paypal_items_builder.rb
new file mode 100644
index 0000000000..d80ac041c4
--- /dev/null
+++ b/app/services/paypal_items_builder.rb
@@ -0,0 +1,53 @@
+# frozen_string_literal: true
+
+class PaypalItemsBuilder
+  def initialize(order)
+    @order = order
+  end
+
+  def call
+    items = order.line_items.map(&method(:line_item))
+
+    tax_adjustments = order.adjustments.tax.additional
+    shipping_adjustments = order.adjustments.shipping
+
+    order.adjustments.eligible.each do |adjustment|
+      next if (tax_adjustments + shipping_adjustments).include?(adjustment)
+
+      items << {
+        Name: adjustment.label,
+        Quantity: 1,
+        Amount: {
+          currencyID: order.currency,
+          value: adjustment.amount
+        }
+      }
+    end
+
+    # Because PayPal doesn't accept $0 items at all.
+    # See https://github.com/spree-contrib/better_spree_paypal_express/issues/10
+    # "It can be a positive or negative value but not zero."
+    items.reject! do |item|
+      item[:Amount][:value].zero?
+    end
+
+    items
+  end
+
+  private
+
+  attr_reader :order
+
+  def line_item(item)
+    {
+      Name: item.product.name,
+      Number: item.variant.sku,
+      Quantity: item.quantity,
+      Amount: {
+        currencyID: item.order.currency,
+        value: item.price
+      },
+      ItemCategory: "Physical"
+    }
+  end
+end
diff --git a/spec/services/paypal_items_builder_spec.rb b/spec/services/paypal_items_builder_spec.rb
new file mode 100644
index 0000000000..c6eb0b6d08
--- /dev/null
+++ b/spec/services/paypal_items_builder_spec.rb
@@ -0,0 +1,121 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe PaypalItemsBuilder do
+  let(:order) { create(:completed_order_with_fees) }
+  let(:service) { described_class.new(order) }
+  let(:items) { described_class.new(order).call }
+
+  it "lists line items" do
+    line_item = order.line_items.first
+
+    expect(items.first[:Name]).to eq line_item.variant.name
+    expect(items.first[:Number]).to eq line_item.variant.sku
+    expect(items.first[:Quantity]).to eq line_item.quantity
+    expect(items.first[:Amount]).to eq(currencyID: order.currency,
+                                       value: line_item.price)
+    expect(items.first[:ItemCategory]).to eq "Physical"
+  end
+
+  context "listing adjustments" do
+    let!(:admin_adjustment) {
+      create(:adjustment, label: "Admin Adjustment", order: order, adjustable: order,
+                          amount: 12, source: nil, originator: nil, state: "closed")
+    }
+    let!(:ineligible_adjustment) {
+      create(:adjustment, label: "Ineligible Adjustment", order: order, adjustable: order,
+                          amount: 34, eligible: false, state: "closed")
+    }
+    let!(:zone) { create(:zone_with_member) }
+    let!(:included_tax_rate) {
+      create(:tax_rate, amount: 12, included_in_price: true, zone: zone,
+                        calculator: ::Calculator::DefaultTax.new)
+    }
+    let!(:additional_tax_rate) {
+      create(:tax_rate, amount: 34, included_in_price: false, zone: zone,
+                        calculator: ::Calculator::DefaultTax.new)
+    }
+    let!(:included_tax_adjustment) {
+      create(:adjustment, label: "Included Tax Adjustment", order: order,
+                          adjustable: order.line_items.first, amount: 56,
+                          originator: included_tax_rate, included: true, state: "closed")
+    }
+    let!(:additional_tax_adjustment) {
+      create(:adjustment, label: "Additional Tax Adjustment", order: order, adjustable: order,
+                          amount: 78, originator: additional_tax_rate, state: "closed")
+    }
+    let!(:enterprise_fee) { create(:enterprise_fee) }
+    let!(:line_item_enterprise_fee) {
+      create(:adjustment, label: "Line Item Fee", order: order, adjustable: order,
+                          amount: 91, originator: enterprise_fee, source: order.line_items.first,
+                          state: "closed")
+    }
+    let!(:order_enterprise_fee) {
+      create(:adjustment, label: "Order Fee", order: order, adjustable: order,
+                          amount: 23, originator: enterprise_fee, state: "closed")
+    }
+
+    it "should add up to the order total, minus any additional tax and the shipping cost" do
+      items_total = items.sum { |i| i[:Quantity] * i[:Amount][:value] }
+      order_tax_total = order.all_adjustments.tax.additional.sum(:amount)
+
+      expect(items_total).to eq(order.total - order_tax_total - order.ship_total)
+    end
+
+    it "lists the payment fee adjustment" do
+      payment_fee = items.find{ |i| i[:Name] == I18n.t('payment_method_fee') }
+
+      expect(payment_fee[:Quantity]).to eq 1
+      expect(payment_fee[:Amount]).to eq(currencyID: order.currency,
+                                         value: order.all_adjustments.payment_fee.first.amount)
+    end
+
+    it "lists admin adjustments" do
+      admin_item = items.find{ |i| i[:Name] == admin_adjustment.label }
+
+      expect(order.all_adjustments.admin.count).to eq 1
+      expect(admin_item[:Quantity]).to eq 1
+      expect(admin_item[:Amount]).to eq(currencyID: order.currency,
+                                        value: order.all_adjustments.admin.first.amount)
+    end
+
+    it "lists enterprise fee adjustments" do
+      line_item_fee = items.find{ |i| i[:Name] == line_item_enterprise_fee.label }
+      order_fee = items.find{ |i| i[:Name] == order_enterprise_fee.label }
+
+      expect(order.all_adjustments.enterprise_fee.count).to eq 2
+
+      expect(line_item_fee[:Quantity]).to eq 1
+      expect(line_item_fee[:Amount]).to eq(currencyID: order.currency,
+                                           value: line_item_enterprise_fee.amount)
+      expect(order_fee[:Quantity]).to eq 1
+      expect(order_fee[:Amount]).to eq(currencyID: order.currency,
+                                       value: order_enterprise_fee.amount)
+    end
+
+    it "does not list tax adjustments" do
+      tax_adjustment_items = items.select do |i|
+        i[:Name].in? [additional_tax_adjustment.label, included_tax_adjustment.label]
+      end
+
+      expect(order.all_adjustments.tax.inclusive.count).to eq 1
+      expect(order.all_adjustments.tax.additional.count).to eq 1
+      expect(tax_adjustment_items.count).to be_zero
+    end
+
+    it "does not list the shipping fee" do
+      shipping_fee_item = items.find{ |i| i[:Name] == I18n.t('shipping') }
+
+      expect(order.all_adjustments.shipping.count).to eq 1
+      expect(shipping_fee_item).to be_nil
+    end
+
+    it "does not list ineligible adjustments" do
+      ineligible_item = items.detect{ |i| i[:Name] == ineligible_adjustment.label }
+
+      expect(order.adjustments.where(eligible: false).count).to eq 1
+      expect(ineligible_item).to be_nil
+    end
+  end
+end

From 3ac16432c70c5584eb2dc92346c6259429700684 Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Mon, 22 Feb 2021 13:30:57 +0000
Subject: [PATCH 4/9] Refactor data representation methods

---
 app/services/paypal_items_builder.rb | 24 ++++++++++++++----------
 1 file changed, 14 insertions(+), 10 deletions(-)

diff --git a/app/services/paypal_items_builder.rb b/app/services/paypal_items_builder.rb
index d80ac041c4..120bc7ccd1 100644
--- a/app/services/paypal_items_builder.rb
+++ b/app/services/paypal_items_builder.rb
@@ -6,7 +6,7 @@ class PaypalItemsBuilder
   end
 
   def call
-    items = order.line_items.map(&method(:line_item))
+    items = order.line_items.map(&method(:line_item_data))
 
     tax_adjustments = order.adjustments.tax.additional
     shipping_adjustments = order.adjustments.shipping
@@ -14,14 +14,7 @@ class PaypalItemsBuilder
     order.adjustments.eligible.each do |adjustment|
       next if (tax_adjustments + shipping_adjustments).include?(adjustment)
 
-      items << {
-        Name: adjustment.label,
-        Quantity: 1,
-        Amount: {
-          currencyID: order.currency,
-          value: adjustment.amount
-        }
-      }
+      items << adjustment_data(adjustment)
     end
 
     # Because PayPal doesn't accept $0 items at all.
@@ -38,7 +31,7 @@ class PaypalItemsBuilder
 
   attr_reader :order
 
-  def line_item(item)
+  def line_item_data(item)
     {
       Name: item.product.name,
       Number: item.variant.sku,
@@ -50,4 +43,15 @@ class PaypalItemsBuilder
       ItemCategory: "Physical"
     }
   end
+
+  def adjustment_data(adjustment)
+    {
+      Name: adjustment.label,
+      Quantity: 1,
+      Amount: {
+        currencyID: order.currency,
+        value: adjustment.amount
+      }
+    }
+  end
 end

From 20f4a5359e6e9fc3b5f633f13fbfb8d5852ba4c6 Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Mon, 22 Feb 2021 15:07:43 +0000
Subject: [PATCH 5/9] Exclude all tax adjustments in item building

Included taxes are ignored here, and the additional tax total is is handled separately.
---
 app/services/paypal_items_builder.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/services/paypal_items_builder.rb b/app/services/paypal_items_builder.rb
index 120bc7ccd1..6eca8669f7 100644
--- a/app/services/paypal_items_builder.rb
+++ b/app/services/paypal_items_builder.rb
@@ -8,7 +8,7 @@ class PaypalItemsBuilder
   def call
     items = order.line_items.map(&method(:line_item_data))
 
-    tax_adjustments = order.adjustments.tax.additional
+    tax_adjustments = order.adjustments.tax
     shipping_adjustments = order.adjustments.shipping
 
     order.adjustments.eligible.each do |adjustment|

From ecf43325270dd027df01674fd184a5ae5ca045fb Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Mon, 22 Feb 2021 16:00:22 +0000
Subject: [PATCH 6/9] Use #all_adjustments scope

Some of the way these objects are returned by different scopes will be changing soon. This ensures we should get the same results.
---
 app/services/paypal_items_builder.rb | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/app/services/paypal_items_builder.rb b/app/services/paypal_items_builder.rb
index 6eca8669f7..548d86cfd7 100644
--- a/app/services/paypal_items_builder.rb
+++ b/app/services/paypal_items_builder.rb
@@ -8,10 +8,10 @@ class PaypalItemsBuilder
   def call
     items = order.line_items.map(&method(:line_item_data))
 
-    tax_adjustments = order.adjustments.tax
-    shipping_adjustments = order.adjustments.shipping
+    tax_adjustments = order.all_adjustments.tax
+    shipping_adjustments = order.all_adjustments.shipping
 
-    order.adjustments.eligible.each do |adjustment|
+    order.all_adjustments.eligible.each do |adjustment|
       next if (tax_adjustments + shipping_adjustments).include?(adjustment)
 
       items << adjustment_data(adjustment)

From d517e5adf6b0e04cb8ca59842f5250bac8c972b0 Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Mon, 22 Feb 2021 16:01:24 +0000
Subject: [PATCH 7/9] Simplify matching

---
 app/services/paypal_items_builder.rb | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/app/services/paypal_items_builder.rb b/app/services/paypal_items_builder.rb
index 548d86cfd7..771e21dac8 100644
--- a/app/services/paypal_items_builder.rb
+++ b/app/services/paypal_items_builder.rb
@@ -8,11 +8,10 @@ class PaypalItemsBuilder
   def call
     items = order.line_items.map(&method(:line_item_data))
 
-    tax_adjustments = order.all_adjustments.tax
-    shipping_adjustments = order.all_adjustments.shipping
+    shipping_and_tax_adjustments = order.all_adjustments.shipping + order.all_adjustments.tax
 
     order.all_adjustments.eligible.each do |adjustment|
-      next if (tax_adjustments + shipping_adjustments).include?(adjustment)
+      next if shipping_and_tax_adjustments.include?(adjustment)
 
       items << adjustment_data(adjustment)
     end

From 02b36363775d4ca2ae8a4de32a76107f7e62a4f1 Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Wed, 24 Feb 2021 12:34:45 +0000
Subject: [PATCH 8/9] Extract relevant adjustments to comment-method

---
 app/services/paypal_items_builder.rb | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/app/services/paypal_items_builder.rb b/app/services/paypal_items_builder.rb
index 771e21dac8..dadfb04283 100644
--- a/app/services/paypal_items_builder.rb
+++ b/app/services/paypal_items_builder.rb
@@ -8,11 +8,7 @@ class PaypalItemsBuilder
   def call
     items = order.line_items.map(&method(:line_item_data))
 
-    shipping_and_tax_adjustments = order.all_adjustments.shipping + order.all_adjustments.tax
-
-    order.all_adjustments.eligible.each do |adjustment|
-      next if shipping_and_tax_adjustments.include?(adjustment)
-
+    relevant_adjustments.each do |adjustment|
       items << adjustment_data(adjustment)
     end
 
@@ -30,6 +26,11 @@ class PaypalItemsBuilder
 
   attr_reader :order
 
+  def relevant_adjustments
+    # Tax total and shipping costs are added separately, so they're not included here.
+    order.all_adjustments.eligible - order.all_adjustments.tax - order.all_adjustments.shipping
+  end
+
   def line_item_data(item)
     {
       Name: item.product.name,

From faf7e3c02be90d3ffeda964359a0e10e658bcb0d Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Wed, 24 Feb 2021 12:35:53 +0000
Subject: [PATCH 9/9] Simplify filtering items with zero price

---
 app/services/paypal_items_builder.rb | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/app/services/paypal_items_builder.rb b/app/services/paypal_items_builder.rb
index dadfb04283..b0497c3372 100644
--- a/app/services/paypal_items_builder.rb
+++ b/app/services/paypal_items_builder.rb
@@ -15,11 +15,9 @@ class PaypalItemsBuilder
     # Because PayPal doesn't accept $0 items at all.
     # See https://github.com/spree-contrib/better_spree_paypal_express/issues/10
     # "It can be a positive or negative value but not zero."
-    items.reject! do |item|
+    items.reject do |item|
       item[:Amount][:value].zero?
     end
-
-    items
   end
 
   private