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

Move return authorizations from spree_backend to OFN #4450

Merged
merged 2 commits into from
Nov 22, 2019
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
24 changes: 24 additions & 0 deletions app/controllers/spree/admin/return_authorizations_controller.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
module Spree
module Admin
class ReturnAuthorizationsController < ResourceController
belongs_to 'spree/order', find_by: :number

update.after :associate_inventory_units
create.after :associate_inventory_units

def fire
@return_authorization.public_send("#{params[:e]}!")
Copy link
Member

Choose a reason for hiding this comment

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

This is a terrible pattern for security. I can't even see any authorisation here. The client can call any method ending in ! on this object. Good to have that visible in our code now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Um... I'm not sure we should merge this as is?

Copy link
Contributor

Choose a reason for hiding this comment

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

Or at least make an issue to change this line, and put it at the top of Tech Debt.

Copy link
Contributor Author

@luisramos0 luisramos0 Nov 22, 2019

Choose a reason for hiding this comment

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

this is live code .
this is not only spree 2 code but also spree master code 🙈 🤕
https://github.com/spree/spree/blob/98fa319554e9bad01fee9a9f56500eea99847d29/backend/app/controllers/spree/admin/return_authorizations_controller.rb#L11

I think the security risk is really just adding return authorizations.

I'll create a tech debt issue.
https://github.com/openfoodfoundation/ofn-security/issues/21

flash[:success] = Spree.t(:return_authorization_updated)
redirect_to :back
end

protected

def associate_inventory_units
(params[:return_quantity] || []).each do |variant_id, qty|
@return_authorization.add_variant(variant_id.to_i, qty.to_i)
end
end
end
end
end
48 changes: 48 additions & 0 deletions app/views/spree/admin/return_authorizations/_form.html.haml
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
%div
%table.index
%thead
%tr
%th= t('.product')
%th= t('.quantity_shipped')
%th= t('.quantity_returned')
%th= t('.return_quantity')
%tbody
- @return_authorization.order.shipments.shipped.collect{|s| s.inventory_units.all}.flatten.group_by(&:variant).each do | variant, units|
- tr_class = cycle('odd', 'even')
- tr_id = dom_id(variant)
%tr{class: tr_class, id: tr_id}
%td
.variant-name= variant.name
.variant-options= variant.options_text
%td.align-center= units.select(&:shipped?).size
%td.align-center= units.select(&:returned?).size
%td.return_quantity.align-center
- if @return_authorization.received?
= @return_authorization.inventory_units.group_by(&:variant)[variant].try(:size) || 0
- elsif units.select(&:shipped?).empty?
0
- else
= number_field_tag "return_quantity[#{variant.id}]", @return_authorization.inventory_units.group_by(&:variant)[variant].try(:size) || 0, { style: 'width:100px;', min: 0 }

= f.field_container :amount do
= f.label :amount, t('.amount')
%span.required *
%br/
- if @return_authorization.received?
= @return_authorization.display_amount
- else
= f.text_field :amount, { style: 'width:80px;' }
= t('.rma_value')
\:
%span#rma_value 0.00
= f.error_message_on :amount

= f.field_container :reason do
= f.label :reason, t('.reason')
= f.text_area :reason, { style: 'height:100px;', class: 'fullwidth' }
= f.error_message_on :reason

= f.field_container :stock_location do
= f.label :stock_location, t('.stock_location')
= f.select :stock_location_id, Spree::StockLocation.active.all.collect{ |l| [l.name, l.id] }, { style: 'height:100px;', class: 'fullwidth' }
= f.error_message_on :reason
25 changes: 25 additions & 0 deletions app/views/spree/admin/return_authorizations/edit.html.haml
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
- content_for :page_actions do
%li
- if @return_authorization.can_receive?
= button_link_to t('.receive'), fire_admin_order_return_authorization_url(@order, @return_authorization, e: 'receive'), method: :put, data: { confirm: t('.are_you_sure') }, icon: 'icon-download-alt'
%li
- if @return_authorization.can_cancel?
= button_link_to t('actions.cancel'), fire_admin_order_return_authorization_url(@order, @return_authorization, e: 'cancel'), method: :put, data: { confirm: t('.are_you_sure') }, icon: 'icon-remove'

= render partial: 'spree/admin/shared/order_tabs', locals: { current: 'Return Authorizations' }

- content_for :page_title do
%i.icon-arrow-right
= t('.return_authorization')
= @return_authorization.number
(#{t("spree.admin.return_authorizations.states." + @return_authorization.state.downcase)})

= render partial: 'spree/shared/error_messages', locals: { target: @return_authorization }

= form_for [:admin, @order, @return_authorization] do |f|
%fieldset.no-border-top
= render partial: 'form', locals: { f: f }
.form-buttons.filter-actions.actions
= button t('actions.update'), 'icon-repeat'
%span.or= t(:or)
= button_link_to t('actions.cancel'), admin_order_return_authorizations_url(@order), icon: 'icon-remove'
39 changes: 39 additions & 0 deletions app/views/spree/admin/return_authorizations/index.html.haml
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
= render partial: 'spree/admin/shared/order_tabs', locals: { current: 'Return Authorizations' }

- content_for :page_actions do
- if @order.shipments.any? &:shipped?
%li
= button_link_to t('.new_return_authorization'), new_admin_order_return_authorization_url(@order), icon: 'icon-plus'
%li= button_link_to t('.back_to_orders_list'), spree.admin_orders_path, icon: 'icon-arrow-left'

- content_for :page_title do
%i.icon-arrow-right
= t('.return_authorizations')

- if @order.shipments.any?(&:shipped?) || @order.return_authorizations.any?
%table.index
%thead
%tr
%th= t('.rma_number')
%th= t('.status')
%th= t('.amount')
%th= "#{t('spree.date')}/#{t('spree.time')}"
%th.actions
%tbody
- @return_authorizations.each do |return_authorization|
- tr_class = cycle('odd', 'even')
- tr_id = spree_dom_id(return_authorization)
%tr{class: tr_class, id: tr_id}
%td= return_authorization.number
%td= Spree.t('spree.admin.return_authorizations.states.' + return_authorization.state.downcase)
%td= return_authorization.display_amount.to_html
%td= pretty_time(return_authorization.created_at)
%td.actions
= link_to_edit return_authorization, no_text: true, class: 'edit'
- unless return_authorization.received?
&nbsp;
= link_to_delete return_authorization, no_text: true
- else
.no-objects-found
= t('.cannot_create_returns')
= button_link_to t('.continue'), admin_orders_url, icon: 'icon-arrow-right'
18 changes: 18 additions & 0 deletions app/views/spree/admin/return_authorizations/new.html.haml
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
= render partial: 'spree/admin/shared/order_tabs', locals: { current: 'Return Authorizations' }

- content_for :page_title do
%i.icon-arrow-right
= t('.new_return_authorization')

- content_for :page_actions do
%li= button_link_to t('.back_to_return_authorizations_list'), spree.admin_order_return_authorizations_url, icon: 'icon-arrow-left'

= render partial: 'spree/shared/error_messages', locals: { target: @return_authorization }

= form_for [:admin, @order, @return_authorization] do |f|
%fieldset.no-border-top
= render partial: 'form', locals: { f: f }
.form-buttons.filter-actions.actions
= button t('.continue'), 'icon-arrow-right'
%span.or= t(:or)
= button_link_to t('actions.cancel'), admin_order_return_authorizations_url(@order), icon: 'icon-remove'
32 changes: 32 additions & 0 deletions config/locales/en.yml
Original file line number Diff line number Diff line change
Expand Up @@ -295,6 +295,7 @@ en:
cancel: "Cancel"
save: "Save"
edit: "Edit"
update: "Update"
delete: "Delete"

admin:
Expand Down Expand Up @@ -3033,6 +3034,37 @@ See the %{link} to find out more about %{sitename}'s features and to start using
form:
name: "Name"
presentation: "Presentation"
return_authorizations:
index:
new_return_authorization: "New Return Authorization"
return_authorizations: "Return Authorizations"
back_to_orders_list: "Back To Orders List"
rma_number: "RMA Number"
status: "Status"
amount: "Amount"
cannot_create_returns: "Cannot create returns as this order has no shipped units."
continue: "Continue"
new:
new_return_authorization: "New Return Authorization"
back_to_return_authorizations_list: "Back To Return Authorization List"
continue: "Continue"
edit:
receive: "receive"
are_you_sure: "Are you sure?"
return_authorization: "Return Authorization"
form:
product: "Product"
quantity_shipped: "Quantity Shipped"
quantity_returned: "Quantity Returned"
return_quantity: "Return Quantity"
amount: "Amount"
rma_value: "RMA Value"
reason: "Reason"
stock_location: "Stock Location"
states:
authorized: "Authorized"
received: "Received"
canceled: "Canceled"
orders:
index:
listing_orders: "Listing Orders"
Expand Down