From 15acf55e96b0a0d57a6dac740c41d65e650ea9e9 Mon Sep 17 00:00:00 2001 From: Tamara Temple Date: Thu, 24 Dec 2020 10:42:31 -0600 Subject: [PATCH 1/5] [750] Order can be viewed by non-super-admins after submitted This commit fixes a problem where non-super-admins cannot view an order once it's been submitted. - Update the controller to check if the current user is allowed to view the order chosen when they are not allowed to edit the order. - Add a permission check for viewing an order. - Add a show view to the orders views. - Update several partials to only display buttons that can modify the order and not when the user can only view the order. - Add some test cases and fixtures to verify the function of Users::OrderManipulator. --- app/controllers/orders_controller.rb | 2 + .../concerns/users/order_manipulator.rb | 4 ++ app/views/orders/_order_info.html.erb | 2 +- .../orders/_tracking_detail_info.html.erb | 8 ++- app/views/orders/show.html.erb | 9 +++ .../_tracking_detail_table.html.erb | 14 ++-- spec/fixtures/orders.yml | 20 ++++++ spec/models/user_spec.rb | 67 +++++++++++++++++++ 8 files changed, 116 insertions(+), 10 deletions(-) create mode 100644 app/views/orders/show.html.erb diff --git a/app/controllers/orders_controller.rb b/app/controllers/orders_controller.rb index d4513176..d634878d 100644 --- a/app/controllers/orders_controller.rb +++ b/app/controllers/orders_controller.rb @@ -41,6 +41,8 @@ def edit if Rails.root.join("app/views/orders/status/#{@order.status}.html.erb").exist? render "orders/status/#{@order.status}" end + elsif current_user.can_view_order?(@order) + render :show else redirect_to orders_path end diff --git a/app/models/concerns/users/order_manipulator.rb b/app/models/concerns/users/order_manipulator.rb index 81fd07e6..6ae5e56e 100644 --- a/app/models/concerns/users/order_manipulator.rb +++ b/app/models/concerns/users/order_manipulator.rb @@ -18,6 +18,10 @@ def can_sync_order?(order) can_sync_orders? && order.closed? && !order.synced? end + def can_view_order?(order) + super_admin? || member_at?(order.organization) + end + def can_edit_order_at?(organization) super_admin? || member_at?(organization) end diff --git a/app/views/orders/_order_info.html.erb b/app/views/orders/_order_info.html.erb index b31c32b3..d00c45d9 100644 --- a/app/views/orders/_order_info.html.erb +++ b/app/views/orders/_order_info.html.erb @@ -33,6 +33,6 @@
- <%= text_area :order, :notes, class: 'form-control' %> + <%= text_area :order, :notes, class: 'form-control', disabled: !current_user.can_edit_order?(order) %>
<% end %> diff --git a/app/views/orders/_tracking_detail_info.html.erb b/app/views/orders/_tracking_detail_info.html.erb index c8d0c006..a3548733 100644 --- a/app/views/orders/_tracking_detail_info.html.erb +++ b/app/views/orders/_tracking_detail_info.html.erb @@ -28,9 +28,11 @@ - <%= render partial: "tracking_details/tracking_detail_table", locals: { tracking_details: order.tracking_details, hidden: order.tracking_details.empty? } %> + <%= render partial: "tracking_details/tracking_detail_table", locals: { order: order, tracking_details: order.tracking_details, hidden: order.tracking_details.empty? } %> - <%= link_to "Add Tracking", nil, type: "button", class: "btn btn-default", id: "add-tracking-number" %> + <% if current_user.can_edit_order?(order) %> + <%= link_to "Add Tracking", nil, type: "button", class: "btn btn-default", id: "add-tracking-number" %> - <%= link_to "Hand Delivered", order_path(order, order: { tracking_details: { tracking_number: ["N/A"], shipping_carrier: [TrackingDetail.shipping_carriers["Hand"]] } }), method: :put, class: "btn btn-default" %> + <%= link_to "Hand Delivered", order_path(order, order: { tracking_details: { tracking_number: ["N/A"], shipping_carrier: [TrackingDetail.shipping_carriers["Hand"]] } }), method: :put, class: "btn btn-default" %> + <% end %> <% end %> diff --git a/app/views/orders/show.html.erb b/app/views/orders/show.html.erb new file mode 100644 index 00000000..5761b935 --- /dev/null +++ b/app/views/orders/show.html.erb @@ -0,0 +1,9 @@ +<% content_for :title, "Order #{@order.id}" %> + +<% content_for :content do %> +
+

Status: <%= @order.status.titleize %>

+ <%= render partial: "order_header", locals: { order: @order } %> + <%= render partial: "order_table", locals: { order: @order, include_sku: true } %> +
+<% end %> diff --git a/app/views/tracking_details/_tracking_detail_table.html.erb b/app/views/tracking_details/_tracking_detail_table.html.erb index 815c641c..33f69b9b 100644 --- a/app/views/tracking_details/_tracking_detail_table.html.erb +++ b/app/views/tracking_details/_tracking_detail_table.html.erb @@ -20,14 +20,16 @@ <%= shipment.delivery_date %> - <% if shipment.delivery_date.blank? %> - <%= link_to tracking_detail_path(shipment, status: 'delivered'), method: :patch, class: 'btn btn-success btn-xs' do %> - Mark Delivered + <% if current_user.can_edit_order?(order) %> + <% if shipment.delivery_date.blank?%> + <%= link_to tracking_detail_path(shipment, status: 'delivered'), method: :patch, class: 'btn btn-success btn-xs' do %> + Mark Delivered + <% end %> <% end %> - <% end %> - <%= link_to tracking_detail_path(shipment), method: :delete, class: "btn btn-danger btn-xs", data: confirm(title: "Deleting Tracking Number: #{shipment.tracking_number}") do %> - + <%= link_to tracking_detail_path(shipment), method: :delete, class: "btn btn-danger btn-xs", data: confirm(title: "Deleting Tracking Number: #{shipment.tracking_number}") do %> + + <% end %> <% end %> diff --git a/spec/fixtures/orders.yml b/spec/fixtures/orders.yml index e0812ede..fdbb8347 100644 --- a/spec/fixtures/orders.yml +++ b/spec/fixtures/orders.yml @@ -47,3 +47,23 @@ received_order_with_order_details: status: 5 ship_to_name: "Acme Receiver" ship_to_address: "123 Fake St." + +acme_order: + organization: acme + user: acme_root + order_date: <%= Time.zone.now %> + created_at: <%= Time.zone.now %> + updated_at: <%= Time.zone.now %> + status: -1 + ship_to_name: "Unsubmitted Order Receiver" + ship_to_address: "123 Fake St." + +acme_submitted_order: + organization: acme + user: acme_root + order_date: <%= Time.zone.now %> + created_at: <%= Time.zone.now %> + updated_at: <%= Time.zone.now %> + status: 1 + ship_to_name: "Open Order Receiver" + ship_to_address: "123 Fake St." diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index b5d33ebe..c8337794 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -65,4 +65,71 @@ expect(foo_inc_root.member_at?(acme)).to be_falsey end end + + describe "User::OrderManipulator" do + describe "#can_edit_order?" do + context "when order has not been shipped" do + let(:order) { orders(:acme_order) } + it "permits super_admin to edit" do + expect(root.can_edit_order?(order)).to be_truthy + end + it "permits acme_root to edit" do + expect(acme_root.can_edit_order?(order)).to be_truthy + end + it "permits acme_normal to edit" do + expect(acme_normal.can_edit_order?(order)).to be_truthy + end + it "denies non-org users to edit" do + expect(foo_inc_root.can_edit_order?(order)).to be_falsy + end + end + context "when order has been shipped" do + let(:order) { orders(:acme_submitted_order) } + it "permits super_admin to edit" do + expect(root.can_edit_order?(order)).to be_truthy + end + it "denies acme_root to edit" do + expect(acme_root.can_edit_order?(order)).to be_falsy + end + it "denies acme_normal to edit" do + expect(acme_normal.can_edit_order?(order)).to be_falsy + end + it "denies non-org users to edit" do + expect(foo_inc_root.can_edit_order?(order)).to be_falsy + end + end + end + describe "#can_view_order?" do + context "when order has not been shipped" do + let(:order) { orders(:acme_order) } + it "permits super_admin to view" do + expect(root.can_view_order?(order)).to be_truthy + end + it "permits acme_root to view" do + expect(acme_root.can_view_order?(order)).to be_truthy + end + it "permits acme_normal to view" do + expect(acme_normal.can_view_order?(order)).to be_truthy + end + it "denies non-org users to view" do + expect(foo_inc_root.can_view_order?(order)).to be_falsy + end + end + context "when order has been shipped" do + let(:order) { orders(:acme_submitted_order) } + it "permits super_admin to view" do + expect(root.can_view_order?(order)).to be_truthy + end + it "denies acme_root to view" do + expect(acme_root.can_view_order?(order)).to be_truthy + end + it "denies acme_normal to view" do + expect(acme_normal.can_view_order?(order)).to be_truthy + end + it "denies non-org users to view" do + expect(foo_inc_root.can_view_order?(order)).to be_falsy + end + end + end + end end From 348b834294b0202b23b3a991cb6b40f68c7b45e1 Mon Sep 17 00:00:00 2001 From: Tamara Temple Date: Mon, 28 Dec 2020 08:01:08 -0600 Subject: [PATCH 2/5] Disable rubocop branch metric in orders#edit --- app/controllers/orders_controller.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/app/controllers/orders_controller.rb b/app/controllers/orders_controller.rb index d634878d..d38e7735 100644 --- a/app/controllers/orders_controller.rb +++ b/app/controllers/orders_controller.rb @@ -34,6 +34,7 @@ def create redirect_to edit_order_path(order) end + # rubocop:disable Metrics/AbcSize def edit @order = Order.includes(order_details: :item).find(params[:id]) @@ -47,6 +48,7 @@ def edit redirect_to orders_path end end + # rubocop:enable Metrics/AbcSize def update order = current_user.update_order params From efa88d94d9972137734b25daab070e97b8b21c5a Mon Sep 17 00:00:00 2001 From: Tamara Temple Date: Mon, 25 Jan 2021 06:21:44 -0600 Subject: [PATCH 3/5] Add request test for orders_controller.rb Ensure correct templates rendered and redirects based on user. --- app/controllers/orders_controller.rb | 27 ++++--- spec/controllers/orders_controller_spec.rb | 4 - .../orders_controller_request_spec.rb | 76 +++++++++++++++++++ 3 files changed, 91 insertions(+), 16 deletions(-) delete mode 100644 spec/controllers/orders_controller_spec.rb create mode 100644 spec/requests/orders_controller_request_spec.rb diff --git a/app/controllers/orders_controller.rb b/app/controllers/orders_controller.rb index d38e7735..dbdbe5bb 100644 --- a/app/controllers/orders_controller.rb +++ b/app/controllers/orders_controller.rb @@ -34,21 +34,10 @@ def create redirect_to edit_order_path(order) end - # rubocop:disable Metrics/AbcSize def edit @order = Order.includes(order_details: :item).find(params[:id]) - - if current_user.can_edit_order?(@order) - if Rails.root.join("app/views/orders/status/#{@order.status}.html.erb").exist? - render "orders/status/#{@order.status}" - end - elsif current_user.can_view_order?(@order) - render :show - else - redirect_to orders_path - end + determine_edit_view end - # rubocop:enable Metrics/AbcSize def update order = current_user.update_order params @@ -59,4 +48,18 @@ def sync order = current_user.sync_order(params) redirect_to edit_order_path(order) end + + private + + def determine_edit_view + if current_user.can_edit_order?(@order) + if Rails.root.join("app/views/orders/status/#{@order.status}.html.erb").exist? + render "orders/status/#{@order.status}" + end + elsif current_user.can_view_order?(@order) + render :show + else + redirect_to orders_path + end + end end diff --git a/spec/controllers/orders_controller_spec.rb b/spec/controllers/orders_controller_spec.rb deleted file mode 100644 index d2b741dd..00000000 --- a/spec/controllers/orders_controller_spec.rb +++ /dev/null @@ -1,4 +0,0 @@ -require "rails_helper" - -describe OrdersController, type: :controller do -end diff --git a/spec/requests/orders_controller_request_spec.rb b/spec/requests/orders_controller_request_spec.rb new file mode 100644 index 00000000..de56851b --- /dev/null +++ b/spec/requests/orders_controller_request_spec.rb @@ -0,0 +1,76 @@ +require "rails_helper" + +describe OrdersController, type: :request do + let(:root) { users(:root) } + let(:org_admin) { users(:view_check_root) } + let(:org_user) { users(:view_check_normal) } + let(:non_org_user) { users(:foo_inc_root) } + + describe "#edit" do + context "before order has been submitted" do + let(:order) { orders(:view_check_unsubmitted_order) } + subject { get edit_order_path(order) } + + context "when logged in as super_admin" do + before { sign_in root } + it "confirm order view is shown" do + expect(subject).to render_template("orders/status/confirm_order") + end + end + + context "when logged in as order's organization admin user" do + before { sign_in org_admin } + it "confirm order view is shown" do + expect(subject).to render_template("orders/status/confirm_order") + end + end + + context "when logged in as order's organization normal user" do + before { sign_in org_user } + it "confirm order view is shown" do + expect(subject).to render_template("orders/status/confirm_order") + end + end + + context "when logged in as another organization user" do + before { sign_in non_org_user } + it "redirects to orders index" do + expect(subject).to redirect_to(orders_path) + end + end + end + + context "after order has been submitted" do + let(:order) { orders(:view_check_submitted_order) } + subject { get edit_order_path(order) } + + context "when logged in as super_admin" do + before { sign_in root } + it "edit view is shown" do + expect(subject).to render_template("edit") + end + end + + context "when logged in as order's organization admin user" do + before { sign_in org_admin } + it "show view is shown" do + expect(subject).to render_template("show") + end + end + + context "when logged in as order's organization normal user" do + before { sign_in org_user } + it "show view is shown" do + expect(subject).to render_template("show") + end + end + + context "when logged in as another organization user" do + before { sign_in non_org_user } + it "redirects to orders index" do + expect(subject).to redirect_to(orders_path) + end + end + end + end +end From 5485a6bce876f7eee2e6f10e8e780b1a3fe87b47 Mon Sep 17 00:00:00 2001 From: Tamara Temple Date: Mon, 25 Jan 2021 07:31:27 -0600 Subject: [PATCH 4/5] [750] Refactor C: Add show action If user calls edit action but is not allows to edit, redirects to show action (order_path(@order)) Show action shows if user is allowed, otherwise redirects to index action (orders_path) routes for orders resource now excludes only :destroy action --- app/controllers/orders_controller.rb | 14 ++++-- config/routes.rb | 2 +- .../orders_controller_request_spec.rb | 49 ++++++++++++++++--- 3 files changed, 53 insertions(+), 12 deletions(-) diff --git a/app/controllers/orders_controller.rb b/app/controllers/orders_controller.rb index dbdbe5bb..a5491917 100644 --- a/app/controllers/orders_controller.rb +++ b/app/controllers/orders_controller.rb @@ -56,10 +56,18 @@ def determine_edit_view if Rails.root.join("app/views/orders/status/#{@order.status}.html.erb").exist? render "orders/status/#{@order.status}" end - elsif current_user.can_view_order?(@order) - render :show else - redirect_to orders_path + redirect_to order_path(@order) end end + + def show + @order = Order.includes(order_details: :item).find(params[:id]) + redirect_to orders_path unless current_user.can_view_order?(@order) + end + + def update + order = current_user.update_order params + redirect_to edit_order_path(order) + end end diff --git a/config/routes.rb b/config/routes.rb index 96d19040..834d1641 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -61,7 +61,7 @@ end end - resources :orders, only: %i[index new create edit update] do + resources :orders, except: %i[destroy] do collection do get :rejected, :closed, :canceled end diff --git a/spec/requests/orders_controller_request_spec.rb b/spec/requests/orders_controller_request_spec.rb index de56851b..1da30b94 100644 --- a/spec/requests/orders_controller_request_spec.rb +++ b/spec/requests/orders_controller_request_spec.rb @@ -34,8 +34,8 @@ context "when logged in as another organization user" do before { sign_in non_org_user } - it "redirects to orders index" do - expect(subject).to redirect_to(orders_path) + it "redirects to order show" do + expect(subject).to redirect_to(order_path(order)) end end end @@ -53,24 +53,57 @@ context "when logged in as order's organization admin user" do before { sign_in org_admin } - it "show view is shown" do - expect(subject).to render_template("show") + it "redirects to order show" do + expect(subject).to redirect_to(order_path(order)) end end context "when logged in as order's organization normal user" do before { sign_in org_user } - it "show view is shown" do - expect(subject).to render_template("show") + it "redirects to order show" do + expect(subject).to redirect_to(order_path(order)) end end context "when logged in as another organization user" do before { sign_in non_org_user } - it "redirects to orders index" do - expect(subject).to redirect_to(orders_path) + it "redirects to order show" do + expect(subject).to redirect_to(order_path(order)) end end end end + + describe "#show" do + let(:order) { orders(:view_check_submitted_order) } + subject { get order_path(order) } + + context "when logged in as super_admin" do + before { sign_in root } + it "show view is shown" do + expect(subject).to render_template("show") + end + end + + context "when logged in as order's organization admin user" do + before { sign_in org_admin } + it "show view is shown" do + expect(subject).to render_template("show") + end + end + + context "when logged in as order's organization normal user" do + before { sign_in org_user } + it "redirects to order show" do + expect(subject).to render_template("show") + end + end + + context "when logged in as another organization user" do + before { sign_in non_org_user } + it "redirects to order show" do + expect(subject).to redirect_to(orders_path) + end + end + end end From 0f548b6763a0126118f65739e9500b7a97ea3273 Mon Sep 17 00:00:00 2001 From: Tamara Temple Date: Mon, 25 Jan 2021 06:50:24 -0600 Subject: [PATCH 5/5] [750] Un-refactor edit action a bit Shows logic inherent in permissions, moves logic based on status to private method. This passes rubocop's length rules while keeping the necessary logic in front --- app/controllers/orders_controller.rb | 33 ++++++++++++++-------------- 1 file changed, 16 insertions(+), 17 deletions(-) diff --git a/app/controllers/orders_controller.rb b/app/controllers/orders_controller.rb index a5491917..3b6605d3 100644 --- a/app/controllers/orders_controller.rb +++ b/app/controllers/orders_controller.rb @@ -36,7 +36,18 @@ def create def edit @order = Order.includes(order_details: :item).find(params[:id]) - determine_edit_view + if current_user.can_edit_order?(@order) + render_status_or_edit + elsif current_user.can_view_order?(@order) + render :show + else + redirect_to orders_path + end + end + + def show + @order = Order.includes(order_details: :item).find(params[:id]) + redirect_to orders_path unless current_user.can_view_order?(@order) end def update @@ -51,23 +62,11 @@ def sync private - def determine_edit_view - if current_user.can_edit_order?(@order) - if Rails.root.join("app/views/orders/status/#{@order.status}.html.erb").exist? - render "orders/status/#{@order.status}" - end + def render_status_or_edit + if Rails.root.join("app/views/orders/status/#{@order.status}.html.erb").exist? + render "orders/status/#{@order.status}" else - redirect_to order_path(@order) + render :edit end end - - def show - @order = Order.includes(order_details: :item).find(params[:id]) - redirect_to orders_path unless current_user.can_view_order?(@order) - end - - def update - order = current_user.update_order params - redirect_to edit_order_path(order) - end end