Add a store credit reasons Admin UI #2798
Conversation
| @@ -0,0 +1,18 @@ | |||
| class RenameSpreeStoreCreditUpdateReasonsTable < ActiveRecord::Migration[5.1] | |||
There was a problem hiding this comment.
Style/FrozenStringLiteralComment: Missing magic comment # frozen_string_literal: true.
| @@ -173,12 +173,12 @@ | |||
| describe "#update_amount" do | |||
| let(:original_amount) { 100.0 } | |||
| let!(:store_credit) { create(:store_credit, user: user, amount: original_amount) } | |||
| let!(:update_reason) { create(:store_credit_update_reason) } | |||
| let!(:store_credit_reason) { create(:store_credit_reason) } | |||
There was a problem hiding this comment.
Layout/ExtraSpacing: Unnecessary spacing detected.
| @@ -16,7 +16,7 @@ | |||
|
|
|||
| let!(:b_credit_category) { create(:store_credit_category, name: "B category") } | |||
| let!(:a_credit_category) { create(:store_credit_category, name: "A category") } | |||
| let!(:update_reason) { create(:store_credit_update_reason) } | |||
| let!(:store_credit_reason) { create(:store_credit_reason) } | |||
There was a problem hiding this comment.
Layout/ExtraSpacing: Unnecessary spacing detected.
There was a problem hiding this comment.
I think it's a good change, I have just a doubt that the StoreCreditUpdateReason resource meant to represent the reason of the update and not the reason of the store credit itself. Could it make sense?
I left some comment about code but I'm a bit concerned about backward compatibility. Can you please give it a thought as well?
| attr_accessor :action, :action_amount, :action_originator, :action_authorization_code, :update_reason | ||
| belongs_to :store_credit_reason, class_name: 'Spree::StoreCreditReason', inverse_of: :store_credits | ||
|
|
||
| attr_accessor :action, :action_amount, :action_originator, :action_authorization_code, :store_credit_reason |
There was a problem hiding this comment.
I don't think store_credit_reason here is needed anymore
|
|
||
| validates_presence_of :update_reason, if: :action_requires_reason? | ||
| validates_presence_of :store_credit_reason_id, if: :action_requires_reason? |
There was a problem hiding this comment.
I think we can use store_credit_reason here
| def load_update_reasons | ||
| @update_reasons = Spree::StoreCreditUpdateReason.all.order(:name) | ||
| def load_reasons | ||
| @store_credit_reasons = Spree::StoreCreditReason.all.order(:name) |
There was a problem hiding this comment.
maybe we should use the active scope here?
| subject { event.valid? } | ||
|
|
||
| context "adjustment event" do | ||
| context "has an update reason" do | ||
| context "has an store credit reason" do |
| let(:event) { build(:store_credit_adjustment_event) } | ||
|
|
||
| it "returns true" do | ||
| expect(subject).to eq true | ||
| end | ||
| end | ||
|
|
||
| context "doesn't have an update reason" do | ||
| let(:event) { build(:store_credit_adjustment_event, update_reason: nil) } | ||
| context "doesn't have an store credit reason" do |
There was a problem hiding this comment.
doesn't have an -> doesn't have a
| end | ||
| end | ||
| end | ||
|
|
||
| context "invalidate event" do | ||
| context "has an update reason" do | ||
| context "has an store credit reason" do |
| let(:event) { build(:store_credit_invalidate_event) } | ||
|
|
||
| it "returns true" do | ||
| expect(subject).to eq true | ||
| end | ||
| end | ||
|
|
||
| context "doesn't have an update reason" do | ||
| let(:event) { build(:store_credit_invalidate_event, update_reason: nil) } | ||
| context "doesn't have an store credit reason" do |
| end | ||
| end | ||
| end | ||
|
|
||
| context "event doesn't require an update reason" do | ||
| context "event doesn't require an store credit reason" do |
|
Hey @jtapia, can you please rebase? I'd like to push merging this with the Core Team this week. Thanks! |
kennyadsl
added
changelog:solidus_backend
|
@kennyadsl done |
We already converted the rest of the factories attributes to dynamic into solidusio#2831 but after merging that PR, we merged solidusio#2798 which was pretty old and was still using the old "way".
Issue
What it does?
Preview