Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Vouchers] Flat rate of any amount #10761

Merged
merged 7 commits into from
May 19, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions app/controllers/admin/vouchers_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -26,11 +26,14 @@ def create
private

def load_enterprise
@enterprise = Enterprise.find_by permalink: params[:enterprise_id]
@enterprise = OpenFoodNetwork::Permissions
.new(spree_current_user)
.editable_enterprises
.find_by(permalink: params[:enterprise_id])
rioug marked this conversation as resolved.
Show resolved Hide resolved
end

def permitted_resource_params
params.require(:voucher).permit(:code)
params.require(:voucher).permit(:code, :amount)
end
end
end
11 changes: 4 additions & 7 deletions app/models/voucher.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,21 +3,18 @@
class Voucher < ApplicationRecord
acts_as_paranoid

belongs_to :enterprise
belongs_to :enterprise, optional: false

has_many :adjustments,
as: :originator,
class_name: 'Spree::Adjustment',
dependent: :nullify

validates :code, presence: true, uniqueness: { scope: :enterprise_id }

def value
10
end
validates :amount, presence: true, numericality: { greater_than: 0 }

def display_value
Spree::Money.new(value)
Spree::Money.new(amount)
end

# Ideally we would use `include CalculatedAdjustments` to be consistent with other adjustments,
Expand All @@ -44,6 +41,6 @@ def create_adjustment(label, order)
# We limit adjustment to the maximum amount needed to cover the order, ie if the voucher
# covers more than the order.total we only need to create an adjustment covering the order.total
def compute_amount(order)
-value.clamp(0, order.total)
-amount.clamp(0, order.total)
end
end
2 changes: 1 addition & 1 deletion app/views/admin/vouchers/new.html.haml
Original file line number Diff line number Diff line change
Expand Up @@ -20,4 +20,4 @@
= f.label :amount, t('.voucher_amount')
.omega.eight.columns
= Spree::Money.currency_symbol
= f.text_field :amount, value: @voucher.value, disabled: true
= f.text_field :amount, value: @voucher.amount
rioug marked this conversation as resolved.
Show resolved Hide resolved
2 changes: 1 addition & 1 deletion app/views/split_checkout/_voucher_section.cable_ready.haml
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
= t("split_checkout.step2.voucher.voucher", voucher_amount: voucher_adjustment.originator.display_value)
= link_to t("split_checkout.step2.voucher.remove_code"), voucher_adjustment_path(id: voucher_adjustment.id), method: "delete", data: { confirm: t("split_checkout.step2.voucher.confirm_delete") }
- # This might not be true, ie payment method including a fee which wouldn't be covered by voucher or tax implication raising total to be bigger than the voucher amount ?
- if voucher_adjustment.originator.value > order.total
- if voucher_adjustment.originator.amount > order.total
.checkout-input
%span.formError.standalone
= t("split_checkout.step2.voucher.warning_forfeit_remaining_amount")
Expand Down
5 changes: 5 additions & 0 deletions db/migrate/20230418030646_add_amount_to_vouchers.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
class AddAmountToVouchers < ActiveRecord::Migration[7.0]
def change
add_column :vouchers, :amount, :decimal, precision: 10, scale: 2, null: false, default: 0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, the default is 0 but the Rails validation requires it to be greater than that. It's contradictory but I don't have any better ideas either. 🤷

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is a default necessary? If an amount isn't provided, then we want an error to be thrown don't we?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you are right, my thinking was a voucher with an amount 0 is probably not a big problem, the worse than can happen is you apply a voucher that won't change the order amount..

end
end
1 change: 1 addition & 0 deletions db/schema.rb
Original file line number Diff line number Diff line change
Expand Up @@ -1197,6 +1197,7 @@
t.datetime "updated_at", null: false
t.bigint "enterprise_id"
t.datetime "deleted_at", precision: nil
t.decimal "amount", precision: 10, scale: 2, default: "0.0", null: false
t.index ["code", "enterprise_id"], name: "index_vouchers_on_code_and_enterprise_id", unique: true
t.index ["deleted_at"], name: "index_vouchers_on_deleted_at"
t.index ["enterprise_id"], name: "index_vouchers_on_enterprise_id"
Expand Down
2 changes: 1 addition & 1 deletion spec/controllers/split_checkout_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -237,7 +237,7 @@
end

describe "Vouchers" do
let(:voucher) { Voucher.create(code: 'some_code', enterprise: distributor) }
let(:voucher) { create(:voucher, code: 'some_code', enterprise: distributor) }

describe "adding a voucher" do
let(:checkout_params) do
Expand Down
8 changes: 8 additions & 0 deletions spec/factories/voucher_factory.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
# frozen_string_literal: true

FactoryBot.define do
factory :voucher do
enterprise { build(:distributor_enterprise) }
amount { 15 }
end
end
5 changes: 3 additions & 2 deletions spec/models/spree/order_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -1432,7 +1432,9 @@ def advance_to_delivery_state(order)
end

describe "#voucher_adjustments" do
let(:voucher) { Voucher.create(code: 'new_code', enterprise: order.distributor) }
let(:distributor) { create(:distributor_enterprise) }
let(:order) { create(:order, user: user, distributor: distributor) }
let(:voucher) { create(:voucher, code: 'new_code', enterprise: order.distributor) }

context "when no voucher adjustment" do
it 'returns an empty array' do
Expand All @@ -1441,7 +1443,6 @@ def advance_to_delivery_state(order)
end

it "returns an array of voucher adjusment" do
order.save!
expected_adjustments = Array.new(2) { voucher.create_adjustment(voucher.code, order) }

expect(order.voucher_adjustments).to eq(expected_adjustments)
Expand Down
14 changes: 8 additions & 6 deletions spec/models/voucher_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,19 +6,21 @@
let(:enterprise) { build(:enterprise) }

describe 'associations' do
it { is_expected.to belong_to(:enterprise) }
it { is_expected.to belong_to(:enterprise).required }
it { is_expected.to have_many(:adjustments) }
end

describe 'validations' do
subject { Voucher.new(code: 'new_code', enterprise: enterprise) }
subject { build(:voucher, code: 'new_code', enterprise: enterprise) }

it { is_expected.to validate_presence_of(:code) }
it { is_expected.to validate_uniqueness_of(:code).scoped_to(:enterprise_id) }
it { is_expected.to validate_presence_of(:amount) }
it { is_expected.to validate_numericality_of(:amount).is_greater_than(0) }
end

describe '#compute_amount' do
subject { Voucher.create(code: 'new_code', enterprise: enterprise) }
subject { create(:voucher, code: 'new_code', enterprise: enterprise, amount: 10) }

let(:order) { create(:order_with_totals) }

Expand All @@ -39,11 +41,11 @@
describe '#create_adjustment' do
subject(:adjustment) { voucher.create_adjustment(voucher.code, order) }

let(:voucher) { Voucher.create(code: 'new_code', enterprise: enterprise) }
let(:order) { create(:order_with_line_items, line_items_count: 1, distributor: enterprise) }
let(:voucher) { create(:voucher, code: 'new_code', enterprise: enterprise, amount: 25) }
let(:order) { create(:order_with_line_items, line_items_count: 3, distributor: enterprise) }

it 'includes the full voucher amount' do
expect(adjustment.amount.to_f).to eq(-10.0)
expect(adjustment.amount.to_f).to eq(-25.0)
end

it 'has no included_tax' do
Expand Down
52 changes: 52 additions & 0 deletions spec/requests/admin/vouchers_controller_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
# frozen_string_literal: true

require "spec_helper"

describe Admin::VouchersController, type: :request do
let(:enterprise) { create(:supplier_enterprise, name: "Feedme") }
let(:enterprise_user) { create(:user, enterprise_limit: 1) }

before do
Flipper.enable(:vouchers)

enterprise_user.enterprise_roles.build(enterprise: enterprise).save
sign_in enterprise_user
end

describe "GET /admin/enterprises/:enterprise_id/vouchers/new" do
it "loads the new voucher page" do
get new_admin_enterprise_voucher_path(enterprise)

expect(response).to render_template("admin/vouchers/new")
end
end

describe "POST /admin/enterprises/:enterprise_id/vouchers" do
subject(:create_voucher) { post admin_enterprise_vouchers_path(enterprise), params: params }

let(:params) do
{
voucher: {
code: code,
amount: amount
}
}
end
let(:code) { "new_code" }
let(:amount) { 15 }

it "creates a new voucher" do
expect { create_voucher }.to change(Voucher, :count).by(1)

voucher = Voucher.last
expect(voucher.code).to eq(code)
expect(voucher.amount).to eq(amount)
end

it "redirects to admin enterprise setting page, voucher panel" do
create_voucher

expect(response).to redirect_to("#{edit_admin_enterprise_path(enterprise)}#vouchers_panel")
end
end
end
2 changes: 1 addition & 1 deletion spec/requests/voucher_adjustments_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
let(:user) { order.user }
let(:distributor) { create(:distributor_enterprise, with_payment_and_shipping: true) }
let(:order) { create( :order_with_line_items, line_items_count: 1, distributor: distributor) }
let(:voucher) { Voucher.create(code: 'some_code', enterprise: distributor) }
let(:voucher) { create(:voucher, code: 'some_code', enterprise: distributor) }
let!(:adjustment) { voucher.create_adjustment(voucher.code, order) }

before do
Expand Down
2 changes: 1 addition & 1 deletion spec/services/voucher_adjustments_service_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
describe VoucherAdjustmentsService do
describe '.calculate' do
let(:enterprise) { build(:enterprise) }
let(:voucher) { Voucher.create(code: 'new_code', enterprise: enterprise) }
let(:voucher) { create(:voucher, code: 'new_code', enterprise: enterprise, amount: 10) }

context 'when voucher covers the order total' do
subject { order.voucher_adjustments.first }
Expand Down
8 changes: 5 additions & 3 deletions spec/system/admin/vouchers_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@

let(:enterprise) { create(:supplier_enterprise, name: 'Feedme') }
let(:voucher_code) { 'awesomevoucher' }
let(:amount) { 25 }
dacook marked this conversation as resolved.
Show resolved Hide resolved
let(:enterprise_user) { create(:user, enterprise_limit: 1) }

before do
Expand All @@ -22,7 +23,7 @@

it 'lists enterprise vouchers' do
# Given an enterprise with vouchers
Voucher.create!(enterprise: enterprise, code: voucher_code)
create(:voucher, enterprise: enterprise, code: voucher_code, amount: amount)

# When I go to the enterprise voucher tab
visit edit_admin_enterprise_path(enterprise)
Expand All @@ -31,7 +32,7 @@

# Then I see a list of vouchers
expect(page).to have_content voucher_code
expect(page).to have_content "10"
expect(page).to have_content amount
end

it 'creates a voucher' do
Expand All @@ -46,12 +47,13 @@

# And I fill in the fields for a new voucher click save
fill_in 'voucher_code', with: voucher_code
fill_in 'voucher_amount', with: amount
click_button 'Save'

# Then I should get redirect to the entreprise voucher tab and see the created voucher
expect(page).to have_selector '.success', text: 'Voucher has been successfully created!'
expect(page).to have_content voucher_code
expect(page).to have_content "10"
expect(page).to have_content amount

voucher = Voucher.where(enterprise: enterprise, code: voucher_code).first

Expand Down
11 changes: 8 additions & 3 deletions spec/system/consumer/split_checkout_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -720,7 +720,9 @@
end

context "with voucher available" do
let!(:voucher) { Voucher.create(code: 'some_code', enterprise: distributor) }
let!(:voucher) do
create(:voucher, code: 'some_code', enterprise: distributor, amount: 15)
end

before do
visit checkout_step_path(:payment)
Expand All @@ -738,7 +740,7 @@
end

it "adds a voucher to the order" do
expect(page).to have_content("$10.00 Voucher")
expect(page).to have_content("$15.00 Voucher")
expect(order.reload.voucher_adjustments.length).to eq(1)
end
end
Expand Down Expand Up @@ -1111,20 +1113,23 @@
end

describe "vouchers" do
let(:voucher) { Voucher.create(code: 'some_code', enterprise: distributor) }
let(:voucher) { create(:voucher, code: 'some_code', enterprise: distributor, amount: 6) }

before do
# Add voucher to the order
voucher.create_adjustment(voucher.code, order)

# Update order so voucher adjustment is properly taken into account
order.update_order!
VoucherAdjustmentsService.calculate(order)

visit checkout_step_path(:summary)
end

it "shows the applied voucher" do
within ".summary-right" do
expect(page).to have_content "some_code"
expect(page).to have_content "-6"
end
end
end
Expand Down
6 changes: 4 additions & 2 deletions spec/system/consumer/split_checkout_tax_incl_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,9 @@
end

context "when using a voucher" do
let!(:voucher) { Voucher.create(code: 'some_code', enterprise: distributor) }
let!(:voucher) do
create(:voucher, code: 'some_code', enterprise: distributor, amount: 10)
end

it "will include a tax included amount on the voucher adjustment" do
visit checkout_step_path(:details)
Expand All @@ -149,7 +151,7 @@
fill_in "Enter voucher code", with: voucher.code
click_button("Apply")

# Choose payment ??
# Choose payment
click_on "Next - Order summary"
click_on "Complete order"

Expand Down
5 changes: 3 additions & 2 deletions spec/system/consumer/split_checkout_tax_not_incl_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,9 @@
end

context "when using a voucher" do
let!(:voucher) { Voucher.create(code: 'some_code', enterprise: distributor) }
let!(:voucher) do
create(:voucher, code: 'some_code', enterprise: distributor, amount: 10)
end

it "will include a tax included amount on the voucher adjustment" do
visit checkout_step_path(:details)
Expand All @@ -156,7 +158,6 @@
fill_in "Enter voucher code", with: voucher.code
click_button("Apply")

# Choose payment ??
click_on "Next - Order summary"
click_on "Complete order"

Expand Down