Customer balance frontoffice #6643

sauloperez · 2021-01-11T17:39:39Z

What? Why?

This reuses the new implementation of customer balance, refactoring it slightly so it fits better for this use case. Fortunately, we didn't have to change the JS implementation. We only updated the server-side data fetching.

The git history should give you all the specific details on how I accomplished this.

What should we test?

The following feature spec covers this

openfoodnetwork/spec/features/consumer/account_spec.rb

Lines 35 to 77 in 334e270

    
           it "shows all hubs that have been ordered from with balance or credit" do 
        
             # Single test to avoid re-rendering page 
        
             visit "/account" 
        
             # No distributors allow changes to orders 
        
             expect(page).to have_no_content I18n.t('spree.users.orders.open_orders') 
        
             expect(page).to have_content I18n.t('spree.users.orders.past_orders') 
        
             # Lists all other orders 
        
             expect(page).to have_content d1o1.number.to_s 
        
             expect(page).to have_content d1o2.number.to_s 
        
             expect(page).to have_link(distributor1.name, 
        
                                       href: "#{distributor1.permalink}/shop", count: 2) 
        
             expect(page).to have_content d2o1.number.to_s 
        
             expect(page).to have_link(distributor2.name, 
        
                                       href: "#{distributor2.permalink}/shop", count: 1) 
        
             expect(page).to have_content credit_order.number.to_s 
        
             expect(page).to have_link(distributor_credit.name, 
        
                                       href: "#{distributor_credit.permalink}/shop", count: 1) 
        
             # Viewing transaction history 
        
             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_link(distributor1.name, 
        
                                       href: "#{distributor1.permalink}/shop", count: 1) 
        
             expect(page).to have_content distributor2.name 
        
             expect(page).to have_link(distributor2.name, 
        
                                       href: "#{distributor2.permalink}/shop", count: 1) 
        
             expect(page).not_to have_content distributor_without_orders.name 
        
             expect(page).to have_content distributor1.name + "\n" + "Balance due" 
        
             expect(page).to have_content distributor_credit.name + "\nCredit" 
        
             # It reveals table of orders for distributors when clicked 
        
             expand_active_table_node distributor1.name 
        
             expect(page).to have_link "Order " + d1o1.number, href: "/orders/#{d1o1.number}" 
        
             expand_active_table_node distributor2.name 
        
             expect(page).not_to have_content "Order " + d1o1.number.to_s 
        
           end

but we should confirm so and trust it.

Specifically, we need to check that the numbers match between back-office and front-office and balances in /accounts are right. Remember to do so with the feature toggle on and off.

Release notes

Use new order balance calculation in the front-office user account page but hidden under a feature toggle.
Changelog Category: User facing changes

app/controllers/spree/users_controller.rb

spec/serializers/api/order_serializer_spec.rb

app/queries/customers_with_balance.rb

andrewpbrett

This looks great - easy to understand and change if necessary in the future.

I only focused on reading/assessing the code, and didn't do any manual testing to confirm that the balances are still the same; I think this will be adequately covered by Filipe or another tester later in the pipe.

andrewpbrett · 2021-01-19T18:17:13Z

app/models/spree/order.rb

+    # All the states of a complete order but that shouldn't count towards the balance. Those that
+    # the customer won't enjoy.


I'm not sure I know what you mean by the customer won't enjoy them? Not a big deal, obviously :)

then it needs to be rephrased. It's a way to say that is a state that belongs after the order is placed but in which the customer won't have the products on their hand

I'd appreciate it if you guys could help me find a perfect name for these two constants and the scope below. Naming is hard, you know.

andrewpbrett · 2021-01-19T18:19:15Z

app/queries/outstanding_balance.rb

@@ -0,0 +1,40 @@
+# frozen_string_literal: true


I like this pattern of pulling queries into their own classes/files.

That's an abstraction I'd like to introduce. I've seen it in the Rails community for a long time (see 4 in https://codeclimate.com/blog/7-ways-to-decompose-fat-activerecord-models/) and although I used it before, I've been using it a lot lately, and IMO it has lots of pros.

some examples:

https://medium.flatstack.com/query-object-in-ruby-on-rails-56ea434365f0

https://thoughtbot.com/blog/active-record-eager-loading-with-query-objects-and-decorators

https://thoughtbot.com/upcase/videos/advanced-query-optimization

app/controllers/spree/users_controller.rb

Matt-Yorkley · 2021-01-20T11:35:25Z

app/controllers/spree/users_controller.rb

+
+      customers = spree_current_user.customers
+      @shops = Enterprise
+        .where(id: @orders.pluck(:distributor_id).uniq | customers.pluck(:enterprise_id))


I'm wondering if distinct could be used here instead of uniq, if we're going for performance?

I plan to create a separate issue for this whole data fetch (among other improvements I found along the way). Stay tuned.

Matt-Yorkley · 2021-01-20T11:44:36Z

app/models/spree/order.rb

+    # the customer won't enjoy.
+    NON_FULFILLED_STATES = %w(canceled returned).freeze
+
+    scope :in_incomplete_state, -> { where(state: PRIOR_TO_COMPLETION_STATES) }


Could we go for a simpler name here? We could have FULFILLED_STATES and UNFULFILLED_STATES and the scope could be unfulfilled?

Also, you've only used this scope in one place where you explicitly use the negation of it (with not), so maybe the opposite scope is more useful, eg: fulfilled?

"Fulfilled" probably doesn't convey the right idea though. Naming things is hard... maybe finalized / unfinalized is better than fulfilled / unfulfilled?

FINALIZED_STATES = %w(complete canceled returned).freeze UNFINALIZED_STATES = %w(cart address delivery payment).freeze scope :finalized, -> { where(state: FINALIZED_STATES) } scope :unfinalized, -> { where(state: UNFINALIZED_STATES) }

~~yeah, that looks better. Thanks!~~ we missed that FINALIZED_STATES should NOT include complete. I'll see what I can do.

I think finalized states should include complete though.

The scope you're trying to get (below) is "all the states that are not cart, address, delivery, payment", which is: complete, canceled, returned.

not in_incomplete_state == not unfinalized == finalized (including complete) 😅

Ah, I see you want a scope that just includes only canceled or returned in some cases. Maybe that's a third scope?

I think semantically, if we have a scope called "finalized", it has to include the completed state.

I'll find another name then. Yes, we don't want two opposite scopes. They are not semantically opposite.

Matt-Yorkley · 2021-01-20T11:59:12Z

app/queries/complete_orders_with_balance.rb

+
+  def sorted_complete_orders
+    @user.orders
+      .where.not(Spree::Order.in_incomplete_state.where_values_hash)


If we go with the finalized / unfinalized scopes, this whole line can can just be .finalized, right?

I'll try removing the double negation once I address the scope naming but where_values_hash is the way to get the WHERE from the underlying AR Relation object to pass them to #not. Apparently, there's no way to make #not and #merge work together.

Data fetching is a controller action responsibility. We shouldn't couple the controller with it too much but it should trigger it, not the view-layer.

Instead of relying on Spree::Order#outstanding_balance we make us of the result set `balance_value` computed column. So, we ask PostgreSQL to compute it instead of Ruby and then serialize it from that computed column. That's a bit faster to compute that way and let's reuse logic. We hide this new implementation under this features' toggle so it's only used when enabled. We want hit the old behaviour by default.

We don't care about the conversion from hash to JSON (that's an ActiveModel::Serializer responsibility that is thoroughly tested) but our logic so we can skip that step which only slows down tests. It consistently reduced ~1.5s on my machine but it's still too slow to wait ~8.5s to get feedback from them.

It improves the overall readability of the code and as a result, things became easier to manage already.

This duplicates the scenarios tested for CustomersWithBalance.

This comment was related to the feature we removed in openfoodfoundation#3609.

sauloperez · 2021-01-20T17:24:06Z

Alright @Matt-Yorkley . I think I made sense of it all in the last commit.

We rely now on the exhaustive list of states an order can be in after checkout. What made this all a bit more messy is that I made up the "checkout" order state, likely mixing it from the payment model states. This simplifies things quite a bit and gives meaningful names to things.

Matt-Yorkley · 2021-01-20T17:56:51Z

app/queries/outstanding_balance.rb

@@ -9,6 +9,10 @@
 #
 # See CompleteOrdersWithBalance or CustomersWithBalance as examples.
 class OutstandingBalance
+  # All the states of a finished order but that shouldn't count towards the balance (the customer
+  # didn't get the order for whatever reason). Note it does not include complete
+  FINALIZED_NON_SUCCESSFUL_STATES = %w(canceled returned).freeze


I'm wondering about awaiting_return and returned now that you've added them in the scope in Spree::Order. Are they relevant here? 🤔

For awaiting_return the customer is still in posession of the product, so until returned, we still need to consider it in the balance.

sauloperez · 2021-01-21T09:16:44Z

So I take it as you approve @Matt-Yorkley . I want to hear from @filipefurtad0

filipefurtad0 · 2021-01-27T15:46:25Z

Hey @sauloperez

Feature toggle comes really handy for testing this 👍

I've checked a specific customer, which has orders on different hubs, before and after toggling this feature, for the customer and hubs - this needs to be done for both "sides" of the order.

I went stepwise and checked all the cases in here. Everything looks fine, but something strikes me as being introduced by this PR, which I'm not sure it is intended:

-> this PR seems to introduce the balance of cancelled orders, in the customer balance. This happens for both for the admin as well as for the customer, so the values match nicely.

So, let's say:

an order completed, and the payment of £15.40 is captured -> then the customer balance is zero.
if we cancel the order:

Then the customer balance is zero as well - as reported on #6366.

Now, let's toggle the feature for the customer only (right side, below):

We can see that the value of the order appears on the balance, on the customer's side. If we now toggle it for the hub as well:

We can see it appears for the hub as well. So there is agreement on both, but I'm not sure this is expected / desired.

Since #6366 is well covered with specs, I focused here on our two missing test cases which relate to:

when adding new payments - add test case to /admin/customers_spec.rb to cover customer ballance, when adding new payments #6732
All good, changes are reflected in both customer and admin side.
partial refunds or a placed order - Add test case to cover partial refunds, while updating customer balance #6733
This works fine as well.

So, this looks good to go 👍 Customer balance matched between (admin) backoffice and (user) account, all the test cases.
But I'm unsure about the fact that cancelled orders now appear on the customer balance - adding the feedback needed label

sauloperez · 2021-01-27T16:37:58Z

But I'm unsure about the fact that cancelled orders now appear on the customer balance

this is what I've been going after with this epic (apart from the perf. issue), and it was introduced in the former PR. But I'll leave it up to @RachL to approve.

Essentially, what we add that makes the balance calculations right is

(...)
CASE WHEN state IN ('canceled', 'returned') THEN payment_total
     WHEN state IS NOT NULL THEN payment_total - total # any other finalized state
ELSE 0 END
  AS balance
(...)

RachL · 2021-01-27T17:30:11Z

@filipefurtad0 @sauloperez is it correct if I sum up in those terms:

before when we cancelled an order with a captured payment, the balance was 0
now the balance is a credit

I thing this is a pretty good improvement? Or am I missing something?

filipefurtad0 · 2021-01-27T18:33:33Z

it was introduced in the former PR

I think I've missed it before, but yeah:

this is a pretty good improvement?

I think so as well!
I was just surprised to see it and felt I should double check 👍

Thanks for feedback!

RachL · 2021-01-27T18:36:50Z

You were right to do so. It's a crazy topic :D

sauloperez · 2021-01-27T18:55:47Z

You were right to do so. It's a crazy topic :D

Don't tell me 😂

sauloperez self-assigned this Jan 11, 2021

sauloperez force-pushed the customer-balance-frontoffice branch 3 times, most recently from aeaec4a to 41a03ef Compare January 13, 2021 15:28

openfoodfoundation deleted a comment from hound bot Jan 13, 2021

sauloperez force-pushed the customer-balance-frontoffice branch from 41a03ef to c8f050c Compare January 13, 2021 15:38

hound bot reviewed Jan 13, 2021

View reviewed changes

app/controllers/spree/users_controller.rb Outdated Show resolved Hide resolved

hound bot reviewed Jan 13, 2021

View reviewed changes

spec/serializers/api/order_serializer_spec.rb Outdated Show resolved Hide resolved

sauloperez mentioned this pull request Jan 13, 2021

GA toggle #6657

Closed

sauloperez force-pushed the customer-balance-frontoffice branch from 4247051 to cc908f7 Compare January 14, 2021 09:48

hound bot reviewed Jan 14, 2021

View reviewed changes

app/queries/customers_with_balance.rb Outdated Show resolved Hide resolved

sauloperez force-pushed the customer-balance-frontoffice branch 5 times, most recently from 9eb1110 to 6e915d1 Compare January 19, 2021 10:49

sauloperez marked this pull request as ready for review January 19, 2021 11:00

sauloperez requested a review from Matt-Yorkley January 19, 2021 12:30

sauloperez force-pushed the customer-balance-frontoffice branch from 6e915d1 to 23cdc86 Compare January 19, 2021 12:30

andrewpbrett approved these changes Jan 19, 2021

View reviewed changes

Matt-Yorkley reviewed Jan 20, 2021

View reviewed changes

app/controllers/spree/users_controller.rb Outdated Show resolved Hide resolved

Matt-Yorkley reviewed Jan 20, 2021

View reviewed changes

sauloperez added 6 commits January 20, 2021 18:23

Move data fetching from injection helper to action

caf1c9e

Data fetching is a controller action responsibility. We shouldn't couple the controller with it too much but it should trigger it, not the view-layer.

Extract outstanding balance SQL CASE/WHEN

681a009

Remove old commented out code

a124f93

Hide new data fetching implementation under toggle

e8ef4ac

sauloperez added 6 commits January 20, 2021 18:23

Move query object to app/queries/ and doc it

d18e79a

Extract query object out of UsersController

7838630

It improves the overall readability of the code and as a result, things became easier to manage already.

Test OutstandingBalance

db23428

This duplicates the scenarios tested for CustomersWithBalance.

Ensure the query the class depends on is called

9bb49bb

Fix long line

996761d

Remove outdated comment

d9c065a

This comment was related to the feature we removed in openfoodfoundation#3609.

sauloperez force-pushed the customer-balance-frontoffice branch from cf89864 to 391c78b Compare January 20, 2021 17:23

sauloperez force-pushed the customer-balance-frontoffice branch from 391c78b to cc9e3fe Compare January 20, 2021 17:34

Matt-Yorkley reviewed Jan 20, 2021

View reviewed changes

filipefurtad0 self-assigned this Jan 27, 2021

filipefurtad0 added the pr-staged-uk staging.openfoodnetwork.org.uk label Jan 27, 2021

filipefurtad0 added feedback-needed and removed pr-staged-uk staging.openfoodnetwork.org.uk labels Jan 27, 2021

filipefurtad0 removed the feedback-needed label Jan 27, 2021

sauloperez merged commit c3897dd into openfoodfoundation:master Jan 27, 2021

sauloperez deleted the customer-balance-frontoffice branch January 27, 2021 18:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Customer balance frontoffice #6643

Customer balance frontoffice #6643

sauloperez commented Jan 11, 2021 •

edited

Loading

andrewpbrett left a comment

andrewpbrett Jan 19, 2021

sauloperez Jan 20, 2021

sauloperez Jan 20, 2021

andrewpbrett Jan 19, 2021

sauloperez Jan 20, 2021

Matt-Yorkley Jan 20, 2021 •

edited

Loading

sauloperez Jan 20, 2021 •

edited

Loading

Matt-Yorkley Jan 20, 2021 •

edited

Loading

sauloperez Jan 20, 2021 •

edited

Loading

Matt-Yorkley Jan 20, 2021

Matt-Yorkley Jan 20, 2021 •

edited

Loading

Matt-Yorkley Jan 20, 2021 •

edited

Loading

Matt-Yorkley Jan 20, 2021

sauloperez Jan 20, 2021 •

edited

Loading

Matt-Yorkley Jan 20, 2021

Matt-Yorkley Jan 20, 2021 •

edited

Loading

sauloperez Jan 20, 2021 •

edited

Loading

sauloperez commented Jan 20, 2021

Matt-Yorkley Jan 20, 2021 •

edited

Loading

sauloperez Jan 20, 2021

sauloperez commented Jan 21, 2021

filipefurtad0 commented Jan 27, 2021

sauloperez commented Jan 27, 2021 •

edited

Loading

RachL commented Jan 27, 2021

filipefurtad0 commented Jan 27, 2021 •

edited

Loading

RachL commented Jan 27, 2021

sauloperez commented Jan 27, 2021

	it "shows all hubs that have been ordered from with balance or credit" do
	# Single test to avoid re-rendering page
	visit "/account"

	# No distributors allow changes to orders
	expect(page).to have_no_content I18n.t('spree.users.orders.open_orders')

	expect(page).to have_content I18n.t('spree.users.orders.past_orders')

	# Lists all other orders
	expect(page).to have_content d1o1.number.to_s
	expect(page).to have_content d1o2.number.to_s
	expect(page).to have_link(distributor1.name,
	href: "#{distributor1.permalink}/shop", count: 2)
	expect(page).to have_content d2o1.number.to_s
	expect(page).to have_link(distributor2.name,
	href: "#{distributor2.permalink}/shop", count: 1)
	expect(page).to have_content credit_order.number.to_s
	expect(page).to have_link(distributor_credit.name,
	href: "#{distributor_credit.permalink}/shop", count: 1)

	# Viewing transaction history
	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_link(distributor1.name,
	href: "#{distributor1.permalink}/shop", count: 1)
	expect(page).to have_content distributor2.name
	expect(page).to have_link(distributor2.name,
	href: "#{distributor2.permalink}/shop", count: 1)
	expect(page).not_to have_content distributor_without_orders.name

	expect(page).to have_content distributor1.name + "\n" + "Balance due"
	expect(page).to have_content distributor_credit.name + "\nCredit"

	# It reveals table of orders for distributors when clicked
	expand_active_table_node distributor1.name
	expect(page).to have_link "Order " + d1o1.number, href: "/orders/#{d1o1.number}"

	expand_active_table_node distributor2.name
	expect(page).not_to have_content "Order " + d1o1.number.to_s
	end

		# All the states of a complete order but that shouldn't count towards the balance. Those that
		# the customer won't enjoy.

Customer balance frontoffice #6643

Customer balance frontoffice #6643

Conversation

sauloperez commented Jan 11, 2021 • edited Loading

What? Why?

What should we test?

Release notes

andrewpbrett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Matt-Yorkley Jan 20, 2021 • edited Loading

Choose a reason for hiding this comment

sauloperez Jan 20, 2021 • edited Loading

Choose a reason for hiding this comment

Matt-Yorkley Jan 20, 2021 • edited Loading

Choose a reason for hiding this comment

sauloperez Jan 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Matt-Yorkley Jan 20, 2021 • edited Loading

Choose a reason for hiding this comment

Matt-Yorkley Jan 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sauloperez Jan 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Matt-Yorkley Jan 20, 2021 • edited Loading

Choose a reason for hiding this comment

sauloperez Jan 20, 2021 • edited Loading

Choose a reason for hiding this comment

sauloperez commented Jan 20, 2021

Matt-Yorkley Jan 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sauloperez commented Jan 21, 2021

filipefurtad0 commented Jan 27, 2021

sauloperez commented Jan 27, 2021 • edited Loading

RachL commented Jan 27, 2021

filipefurtad0 commented Jan 27, 2021 • edited Loading

RachL commented Jan 27, 2021

sauloperez commented Jan 27, 2021

sauloperez commented Jan 11, 2021 •

edited

Loading

Matt-Yorkley Jan 20, 2021 •

edited

Loading

sauloperez Jan 20, 2021 •

edited

Loading

Matt-Yorkley Jan 20, 2021 •

edited

Loading

sauloperez Jan 20, 2021 •

edited

Loading

Matt-Yorkley Jan 20, 2021 •

edited

Loading

Matt-Yorkley Jan 20, 2021 •

edited

Loading

sauloperez Jan 20, 2021 •

edited

Loading

Matt-Yorkley Jan 20, 2021 •

edited

Loading

sauloperez Jan 20, 2021 •

edited

Loading

Matt-Yorkley Jan 20, 2021 •

edited

Loading

sauloperez commented Jan 27, 2021 •

edited

Loading

filipefurtad0 commented Jan 27, 2021 •

edited

Loading