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 part 2 #11003

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
d9153f2
Introduce "zero priced order" concept with no required payment
Matt-Yorkley Jun 1, 2023
24b2ff3
Simplify voucher controller
Matt-Yorkley Jun 1, 2023
da11241
Move form definition down into each checkout step
Matt-Yorkley Jun 1, 2023
3501101
Move voucher section out of main checkout form
Matt-Yorkley Jun 1, 2023
7978aa8
Add separate voucher form
Matt-Yorkley Jun 1, 2023
0e93946
Move voucher processing out of checkout controller
Matt-Yorkley Jun 1, 2023
2b98d2e
Move voucher adjustment calculations out of checkout controller
Matt-Yorkley Jun 2, 2023
930d4b1
Don't apply tax calculations if there's no tax
Matt-Yorkley Jun 2, 2023
28a8aa7
Drop superfluous method
Matt-Yorkley Jun 2, 2023
2e5ae8a
Extract voucher tests to separate controller spec
Matt-Yorkley Jun 2, 2023
f23cda5
Introduce "zero priced orders" to checkout UI and order state flow
Matt-Yorkley Jun 2, 2023
0c3877e
Introduce "zero priced orders" in admin order payments UI and helper
Matt-Yorkley Jun 2, 2023
53e5ee5
Move loading of saved cards out of checkout concern
Matt-Yorkley Jun 2, 2023
f92ceb6
Remove @voucher_adjustment instance variable
Matt-Yorkley Jun 2, 2023
49499a9
Show/hide payment methods if voucher changes order total to zero
Matt-Yorkley Jun 2, 2023
264abcc
Clarify named vouchers in UI
Matt-Yorkley Jun 2, 2023
0bf0f38
Re-enable voucher test
Matt-Yorkley Jun 5, 2023
657a9a3
Improve feature toggling
Matt-Yorkley Jun 8, 2023
472cf6f
Fix rubocop complaint
Matt-Yorkley Jun 8, 2023
3d3370f
Update use of params
Matt-Yorkley Jun 8, 2023
821b579
Fix CSS/layout issues
Matt-Yorkley Jun 8, 2023
eced839
Fix flaky test
Matt-Yorkley Jun 8, 2023
1085557
Add nil safety in reports for zero priced orders with no payment method
Matt-Yorkley Jun 8, 2023
a211605
Use pre_discount_total when comparing to voucher amount
Matt-Yorkley Jun 8, 2023
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
9 changes: 3 additions & 6 deletions app/controllers/admin/vouchers_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,11 @@ def new
end

def create
voucher_params = permitted_resource_params.merge(enterprise: @enterprise)
@voucher = Voucher.create(voucher_params)
@voucher = Voucher.create(permitted_resource_params.merge(enterprise: @enterprise))

if @voucher.save
redirect_to(
"#{edit_admin_enterprise_path(@enterprise)}#vouchers_panel",
flash: { success: flash_message_for(@voucher, :successfully_created) }
)
flash[:success] = flash_message_for(@voucher, :successfully_created)
redirect_to edit_admin_enterprise_path(@enterprise, anchor: :vouchers_panel)
else
flash[:error] = @voucher.errors.full_messages.to_sentence
render :new
Expand Down
9 changes: 1 addition & 8 deletions app/controllers/concerns/checkout_callbacks.rb
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ module CheckoutCallbacks
prepend_before_action :require_order_cycle
prepend_before_action :require_distributor_chosen

before_action :load_order, :associate_user, :load_saved_addresses, :load_saved_credit_cards
before_action :load_order, :associate_user, :load_saved_addresses
before_action :load_shipping_methods, if: -> { params[:step] == "details" }

before_action :ensure_order_not_completed
Expand All @@ -30,8 +30,6 @@ def load_order
@order.manual_shipping_selection = true
@order.checkout_processing = true

@voucher_adjustment = @order.voucher_adjustments.first

redirect_to(main_app.shop_path) && return if redirect_to_shop?
redirect_to_cart_path && return unless valid_order_line_items?
end
Expand All @@ -43,11 +41,6 @@ def load_saved_addresses
@order.ship_address ||= finder.ship_address
end

def load_saved_credit_cards
@saved_credit_cards = spree_current_user&.credit_cards&.with_payment_profile.to_a
@selected_card = nil
end

def load_shipping_methods
@shipping_methods = available_shipping_methods.sort { |a, b| a.name.casecmp(b.name) }
end
Expand Down
66 changes: 4 additions & 62 deletions app/controllers/split_checkout_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -25,12 +25,12 @@ def edit
redirect_to_step_based_on_order unless params[:step]
check_step if params[:step]

flash_error_when_no_shipping_method_available if available_shipping_methods.none?
return if available_shipping_methods.any?

flash[:error] = I18n.t('split_checkout.errors.no_shipping_methods_available')
end

def update
return process_voucher if params[:apply_voucher].present?

if confirm_order || update_order
return if performed?

Expand Down Expand Up @@ -60,27 +60,6 @@ def render_error
replace("#flashes", partial("shared/flashes", locals: { flashes: flash }))
end

def render_voucher_section_or_redirect
respond_to do |format|
format.cable_ready { render_voucher_section }
format.html { redirect_to checkout_step_path(:payment) }
end
end

# Using the power of cable_car we replace only the #voucher_section instead of reloading the page
def render_voucher_section
render(
status: :ok,
cable_ready: cable_car.replace(
"#voucher-section",
partial(
"split_checkout/voucher_section",
locals: { order: @order, voucher_adjustment: @order.voucher_adjustments.first }
)
)
)
end

def order_error_messages
# Remove ship_address.* errors if no shipping method is not selected
remove_ship_address_errors if no_ship_address_needed?
Expand Down Expand Up @@ -125,10 +104,6 @@ def bill_address_error_order(error)
end
end

def flash_error_when_no_shipping_method_available
flash[:error] = I18n.t('split_checkout.errors.no_shipping_methods_available')
end

def check_payments_adjustments
@order.payments.each(&:ensure_correct_adjustment)
end
Expand Down Expand Up @@ -201,40 +176,6 @@ def shipping_method_ship_address_not_required?
selected_shipping_method.first.require_ship_address == false
end

def process_voucher
if add_voucher
VoucherAdjustmentsService.calculate(@order)
render_voucher_section_or_redirect
elsif @order.errors.present?
render_error
end
end

def add_voucher
if params.dig(:order, :voucher_code).blank?
@order.errors.add(:voucher, I18n.t('split_checkout.errors.voucher_not_found'))
return false
end

# Fetch Voucher
voucher = Voucher.find_by(code: params[:order][:voucher_code], enterprise: @order.distributor)

if voucher.nil?
@order.errors.add(:voucher, I18n.t('split_checkout.errors.voucher_not_found'))
return false
end

adjustment = voucher.create_adjustment(voucher.code, @order)

unless adjustment.valid?
@order.errors.add(:voucher, I18n.t('split_checkout.errors.add_voucher_error'))
adjustment.errors.each { |error| @order.errors.import(error) }
return false
end

true
end

def summary_step?
params[:step] == "summary"
end
Expand Down Expand Up @@ -262,6 +203,7 @@ def validate_details!

def validate_payment!
return true if params.dig(:order, :payments_attributes, 0, :payment_method_id).present?
return true if @order.zero_priced_order?

@order.errors.add :payment_method, I18n.t('split_checkout.errors.select_a_payment_method')
end
Expand Down
69 changes: 57 additions & 12 deletions app/controllers/voucher_adjustments_controller.rb
Original file line number Diff line number Diff line change
@@ -1,32 +1,77 @@
# frozen_string_literal: true

class VoucherAdjustmentsController < BaseController
include CablecarResponses
before_action :set_order

def create
if add_voucher
VoucherAdjustmentsService.calculate(@order)
@order.update_totals_and_states

update_payment_section
elsif @order.errors.present?
render_error
end
end

def destroy
@order.voucher_adjustments.find_by(id: params[:id])&.destroy

update_payment_section
end

private

def set_order
@order = current_order
end

@order.voucher_adjustments.find_by(id: params[:id])&.destroy
def add_voucher
if voucher_params[:voucher_code].blank?
@order.errors.add(:voucher_code, I18n.t('split_checkout.errors.voucher_not_found'))
return false
end

respond_to do |format|
format.cable_ready { render_voucher_section }
format.html { redirect_to checkout_step_path(:payment) }
voucher = Voucher.find_by(code: voucher_params[:voucher_code], enterprise: @order.distributor)

if voucher.nil?
@order.errors.add(:voucher_code, I18n.t('split_checkout.errors.voucher_not_found'))
return false
end

adjustment = voucher.create_adjustment(voucher.code, @order)

unless adjustment.valid?
@order.errors.add(:voucher_code, I18n.t('split_checkout.errors.add_voucher_error'))
adjustment.errors.each { |error| @order.errors.import(error) }
return false
end

true
end

private
def update_payment_section
render cable_ready: cable_car.replace(
selector: "#checkout-payment-methods",
html: render_to_string(partial: "split_checkout/payment", locals: { step: "payment" })
)
end

def render_error
flash.now[:error] = @order.errors.full_messages.to_sentence

# Using the power of cable_car we replace only the #voucher_section instead of reloading the page
def render_voucher_section
render(
status: :ok,
cable_ready: cable_car.replace(
render status: :unprocessable_entity, cable_ready: cable_car.
replace("#flashes", partial("shared/flashes", locals: { flashes: flash })).
replace(
"#voucher-section",
partial(
"split_checkout/voucher_section",
locals: { order: @order, voucher_adjustment: @order.voucher_adjustments.first }
)
)
)
end

def voucher_params
params.require(:order).permit(:voucher_code)
end
end
2 changes: 2 additions & 0 deletions app/helpers/checkout_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,8 @@ def payment_method_price(method, order)
end

def payment_or_shipping_price(method, order)
return unless method

price = method.compute_amount(order)
if price.zero?
t('checkout_method_free')
Expand Down
5 changes: 3 additions & 2 deletions app/helpers/spree/payment_methods_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,13 @@ module Spree
module PaymentMethodsHelper
def payment_method(payment)
# hack to allow us to retrieve the name of a "deleted" payment method
id = payment.payment_method_id
return unless (id = payment.payment_method_id)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I find this hard to read and it's easy to miss you are not doing a comparison here (my rails console also gives me a nice warning : warning: found '= literal' in conditional, should be ==) .
I'd prefer something like this for clarity:

Suggested change
return unless (id = payment.payment_method_id)
id = payment.payment_method_id
return if id.nil?


Spree::PaymentMethod.find_with_destroyed(id)
end

def payment_method_name(payment)
payment_method(payment).name
payment_method(payment)&.name
end
end
end
12 changes: 11 additions & 1 deletion app/models/spree/order.rb
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ class Order < ApplicationRecord
go_to_state :delivery
go_to_state :payment, if: ->(order) {
order.update_totals
order.payment_required?
order.payment_required? || order.zero_priced_order?
}
go_to_state :confirmation
go_to_state :complete
Expand Down Expand Up @@ -220,6 +220,12 @@ def payment_required?
total.to_f > 0.0 && !skip_payment_for_subscription?
end

# There are items present in the order, but either the items have zero price,
# or the order's total has been modified (maybe discounted) to zero.
def zero_priced_order?
valid? && line_items.count.positive? && total.zero?
end

# Returns the relevant zone (if any) to be used for taxation purposes.
# Uses default tax zone unless there is a specific match
def tax_zone
Expand Down Expand Up @@ -616,6 +622,10 @@ def process_each_payment
raise Core::GatewayError, Spree.t(:no_pending_payments) if pending_payments.empty?

pending_payments.each do |payment|
if payment.amount.zero? && zero_priced_order?
payment.update_columns(state: "completed", captured_at: Time.zone.now)
end

break if payment_total >= total

yield payment
Expand Down
4 changes: 3 additions & 1 deletion app/services/voucher_adjustments_service.rb
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,10 @@ def self.calculate(order)
# For now we just assume it is either all tax included in price or all tax excluded from price.
if order.additional_tax_total.positive?
handle_tax_excluded_from_price(order, amount)
else
elsif order.included_tax_total.positive?
handle_tax_included_in_price(order, amount)
else
adjustment.amount = amount
end

# Move to closed state
Expand Down
Loading