Add amount_remaining for Spree::StoreCreditEvent #1512

mtylty · 2016-10-10T13:26:35Z

This additional field will make sure that credit events will record the actual unused amount for a particular Spree::StoreCredit.

This also hides the user_total_amount in the backend because it makes no sense to display the total credit a user has at her/his disposal when visiting the store credit show route in the backend.

This should close #1473. It's a temporary fix waiting for a bigger refactor of Spree::StoreCreditEvent.

mtylty · 2016-10-10T13:30:54Z

The new backend will look like something like this:

gmacdougall · 2016-10-10T16:07:21Z

Thanks for taking this on Matteo. Is there anything that we could do to fill in the amount remaining on all of the existing store credits? We should have enough of a history to come up with something based upon the StoreCreditEvent.

If not, I would suggest we leave it NULL to show that we don't have that data.

mtylty · 2016-10-17T13:35:35Z

I've changed the migration to add NULL default and migrate all the old data.

About the data migration: I've manually tested the various cases and it seems to be working for me (it correctly recalculated all the remaining amounts for all the store credit events I had in the sandbox). Can't say if there are some edge cases I'm not considering... Please take a look at the comments on the migration and tell me if you think it could be right.

I noticed there is a DEPRECATION WARNING on display_remaining_amount when amount_remaining is nil saying I should give "0.00" as an argument... Maybe it's better to leave the default of the new column to 0.0?

This commit backports work from an open [Solidus PR][1] [1]: solidusio#1512 This migration adds the `amount_remaining` column to the `spree_store_credit_events` table, and iterates over existing Store Credit Events to calculate historical data for this column. This value is also displayed in the admin, instead of the User's total remaining store credit.

bbuchalter · 2017-05-26T17:25:24Z

I've been using this change successfully in my store for more than six months. I believe it's time to get this one merged. 👍

cc: @gmacdougall

This commit backports work from an open [Solidus PR][1] [1]: solidusio#1512 This migration adds the `amount_remaining` column to the `spree_store_credit_events` table, and iterates over existing Store Credit Events to calculate historical data for this column. This value is also displayed in the admin, instead of the User's total remaining store credit.

tvdeyen

LGTM, @gmacdougall could you please review as well?

tvdeyen · 2017-10-04T12:20:55Z

@mtylty would you mind to rebase this with latest master? Thanks

houndci-bot · 2017-11-17T15:30:55Z

core/db/migrate/20161006104057_add_amount_remaining_to_store_credit_events.rb

+
+      credit.store_credit_events.chronological.each do |event|
+        case event.action
+        when Spree::StoreCredit::ALLOCATION_ACTION,


Avoid when branches without a body.

I'm not sure... is this a false positive?

@mtylty I think hound is right. This when is missing its content, maybe it should just be credit_amount? Or are you intentionally leaving it without content, just for documentation?

@kennyadsl you're right I didn't notice the when doesn't actually have code in it. Thanks!

jhawthorn · 2017-11-29T16:36:46Z

core/db/migrate/20161006104057_add_amount_remaining_to_store_credit_events.rb

+  def up
+    add_column :spree_store_credit_events, :amount_remaining, :decimal, precision: 8, scale: 2, default: nil, null: true
+
+    Spree::StoreCredit.all.each do |credit|


We need to avoid referencing the Spree::StoreCredit model and the constants here.

Instead we can define our own "AddAmountRemainingToStoreCreditEvents::StoreCredit" model here and use that instead.

houndci-bot · 2018-03-22T11:11:14Z

core/db/migrate/20161006104057_add_amount_remaining_to_store_credit_events.rb

@@ -0,0 +1,38 @@
+class AddAmountRemainingToStoreCreditEvents < ActiveRecord::Migration[5.0]


Missing magic comment # frozen_string_literal: true.

mtylty · 2018-03-22T14:05:43Z

@kennyadsl this should be good to go, I made the requested changes.

I think it needs to be re-reviewed because I added a couple of changes that are probably worth talking about.

I talked this through with @kennyadsl and we decided to go for StoreCredit.includes(:store_credit_events).find_each to actually load the records to migrate so that, if you have a huge number of store credits, the server won't try to load all the records at once into memory.

kennyadsl

Thanks @mtylty

kennyadsl · 2018-03-28T15:35:57Z

@mtylty can you rebase now please?

This additional field will make sure that credit events will record the actual unused amount for a particular Spree::StoreCredit. This also hides the user_total_amount in the backend because it makes no sense to display the total credit a user has at her/his disposal when visiting the store credit show route in the backend.

This adds data migration capabilities to the migration that adds the amount_remaining column to Spree::StoreCreditEvent. The migration will re-calculate the amount_remaining based on previous store credit events and the initial credit amount.

mtylty · 2018-03-28T15:42:26Z

@kennyadsl done!

jhawthorn

Looks good to me.

My only concern is that the migration might be slow

stewart mentioned this pull request May 26, 2017

Backport StoreCredit#amount_remaining Solidus PR TommyJohnWear/solidus#1

Merged

tvdeyen assigned gmacdougall Oct 4, 2017

tvdeyen approved these changes Oct 4, 2017

View reviewed changes

mtylty force-pushed the nebulab/fix-store-credit-balance-history branch from 3d8045e to 1f92c97 Compare November 17, 2017 15:30

houndci-bot reviewed Nov 17, 2017

View reviewed changes

mtylty force-pushed the nebulab/fix-store-credit-balance-history branch 2 times, most recently from 11cce75 to ee1ee3c Compare November 28, 2017 17:36

jhawthorn reviewed Nov 29, 2017

View reviewed changes

kennyadsl assigned kennyadsl and unassigned gmacdougall Mar 7, 2018

mtylty force-pushed the nebulab/fix-store-credit-balance-history branch from ee1ee3c to 994519a Compare March 9, 2018 09:41

kennyadsl force-pushed the nebulab/fix-store-credit-balance-history branch from 994519a to ee8a77b Compare March 22, 2018 11:11

houndci-bot reviewed Mar 22, 2018

View reviewed changes

mtylty force-pushed the nebulab/fix-store-credit-balance-history branch from ee8a77b to 41682aa Compare March 22, 2018 13:33

kennyadsl approved these changes Mar 22, 2018

View reviewed changes

mtylty added 2 commits March 28, 2018 17:37

mtylty force-pushed the nebulab/fix-store-credit-balance-history branch from 41682aa to 38a4a5f Compare March 28, 2018 15:38

jhawthorn approved these changes Mar 28, 2018

View reviewed changes

kennyadsl merged commit b504f7e into solidusio:master Apr 11, 2018

kennyadsl deleted the nebulab/fix-store-credit-balance-history branch April 11, 2018 15:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add amount_remaining for Spree::StoreCreditEvent #1512

Add amount_remaining for Spree::StoreCreditEvent #1512

mtylty commented Oct 10, 2016 •

edited

Loading

mtylty commented Oct 10, 2016

gmacdougall commented Oct 10, 2016

mtylty commented Oct 17, 2016

bbuchalter commented May 26, 2017

tvdeyen left a comment

tvdeyen commented Oct 4, 2017

houndci-bot Nov 17, 2017

mtylty Nov 17, 2017

kennyadsl Nov 25, 2017

mtylty Nov 28, 2017

jhawthorn Nov 29, 2017

houndci-bot Mar 22, 2018

mtylty commented Mar 22, 2018 •

edited

Loading

kennyadsl left a comment

kennyadsl commented Mar 28, 2018

mtylty commented Mar 28, 2018

jhawthorn left a comment

		@@ -0,0 +1,38 @@
		class AddAmountRemainingToStoreCreditEvents < ActiveRecord::Migration[5.0]

Add amount_remaining for Spree::StoreCreditEvent #1512

Add amount_remaining for Spree::StoreCreditEvent #1512

Conversation

mtylty commented Oct 10, 2016 • edited Loading

mtylty commented Oct 10, 2016

gmacdougall commented Oct 10, 2016

mtylty commented Oct 17, 2016

bbuchalter commented May 26, 2017

tvdeyen left a comment

Choose a reason for hiding this comment

tvdeyen commented Oct 4, 2017

houndci-bot Nov 17, 2017

Choose a reason for hiding this comment

mtylty Nov 17, 2017

Choose a reason for hiding this comment

kennyadsl Nov 25, 2017

Choose a reason for hiding this comment

mtylty Nov 28, 2017

Choose a reason for hiding this comment

jhawthorn Nov 29, 2017

Choose a reason for hiding this comment

houndci-bot Mar 22, 2018

Choose a reason for hiding this comment

mtylty commented Mar 22, 2018 • edited Loading

kennyadsl left a comment

Choose a reason for hiding this comment

kennyadsl commented Mar 28, 2018

mtylty commented Mar 28, 2018

jhawthorn left a comment

Choose a reason for hiding this comment

mtylty commented Oct 10, 2016 •

edited

Loading

mtylty commented Mar 22, 2018 •

edited

Loading