From 39c755089f291416471923725ca29abd2a0ee870 Mon Sep 17 00:00:00 2001 From: Jorge Coutinho Date: Fri, 1 Nov 2024 13:56:03 -0300 Subject: [PATCH 1/4] add new logic for product_drive deletion --- app/models/product_drive.rb | 4 ++++ app/views/product_drives/show.html.erb | 6 +++++- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/app/models/product_drive.rb b/app/models/product_drive.rb index f9a36a4b87..999694d068 100644 --- a/app/models/product_drive.rb +++ b/app/models/product_drive.rb @@ -69,6 +69,10 @@ def self.search_date_range(dates) @search_date_range = { start_date: dates[0], end_date: dates[1] } end + def can_delete?(user) + user.has_role?(Role::ORG_ADMIN, organization) && donations.empty? + end + # quantities are FILTERED by date then SORTED by name # # @param date_range [Range] diff --git a/app/views/product_drives/show.html.erb b/app/views/product_drives/show.html.erb index 4153457f04..da244bceb4 100644 --- a/app/views/product_drives/show.html.erb +++ b/app/views/product_drives/show.html.erb @@ -90,7 +90,11 @@

<%= edit_button_to edit_product_drive_path(@product_drive), { text: "Make a correction", size: "md" } %> - <%= delete_button_to product_drive_path(@product_drive), { confirm: "Are you sure you want to permanently remove this product drive?", size: "md" } if current_user.has_role?(Role::ORG_ADMIN, current_organization) %> + <% if @product_drive.can_delete?(current_user) %> + <%= delete_button_to product_drive_path(@product_drive), + confirm: "Are you sure you want to permanently remove this product drive?", + size: "md" %> + <% end %>
From 3aec94e073f2892444abc98e09e9a1e2c4678332 Mon Sep 17 00:00:00 2001 From: Jorge Coutinho Date: Fri, 1 Nov 2024 13:56:35 -0300 Subject: [PATCH 2/4] add new tests for Product Drive system spec --- spec/system/product_drive_system_spec.rb | 30 ++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/spec/system/product_drive_system_spec.rb b/spec/system/product_drive_system_spec.rb index c60bc0122a..67a534e11f 100644 --- a/spec/system/product_drive_system_spec.rb +++ b/spec/system/product_drive_system_spec.rb @@ -130,4 +130,34 @@ expect(page).to have_content 'Endless drive' end end + + context "when deleting a Product drive" do + let(:new_product_drive) { create(:product_drive, name: 'New Test', start_date: 3.weeks.ago, end_date: 3.weeks.from_now, organization: organization) } + let(:subject) { product_drive_path(new_product_drive.id) } + + context "when user is a org_admin" do + before do + user.add_role(:org_admin, organization) + end + + it "must delete" do + visit subject + click_on 'Delete' + expect(page).to have_content 'Product drive was successfully destroyed.' + end + + it "must not delete if there are donations" do + create(:donation, product_drive: new_product_drive) + visit subject + expect(page).not_to have_button 'Delete' + end + end + + context "when user is not a org_admin" do + it "must not delete" do + visit subject + expect(page).not_to have_button 'Delete' + end + end + end end From cb17ab10a371fa78fae8ded3c56f3acec5113636 Mon Sep 17 00:00:00 2001 From: Jorge Coutinho Date: Mon, 4 Nov 2024 20:41:22 -0300 Subject: [PATCH 3/4] Implement new role verification and validation for product drive deletion --- app/controllers/product_drives_controller.rb | 18 +++++++++- app/helpers/product_drive_helper.rb | 4 +++ app/models/product_drive.rb | 9 +++-- app/views/product_drives/show.html.erb | 9 +++-- spec/requests/product_drives_requests_spec.rb | 34 ++++++++++++++++--- 5 files changed, 62 insertions(+), 12 deletions(-) diff --git a/app/controllers/product_drives_controller.rb b/app/controllers/product_drives_controller.rb index 05e55e5dca..67c24a268d 100644 --- a/app/controllers/product_drives_controller.rb +++ b/app/controllers/product_drives_controller.rb @@ -1,6 +1,7 @@ class ProductDrivesController < ApplicationController include Importable before_action :set_product_drive, only: [:show, :edit, :update, :destroy] + before_action :verify_role, only: :destroy def index setup_date_range_picker @@ -76,7 +77,15 @@ def update end def destroy - current_organization.product_drives.find(params[:id]).destroy + product_drive = current_organization.product_drives.find(params[:id]) + product_drive.destroy + + if product_drive.errors.any? + flash[:error] = product_drive.errors.full_messages.join("\n") + redirect_back(fallback_location: product_drives_url) + return + end + respond_to do |format| format.html { redirect_to product_drives_url, notice: 'Product drive was successfully destroyed.' } format.json { head :no_content } @@ -85,6 +94,13 @@ def destroy private + def verify_role + return if current_user.has_role?(Role::ORG_ADMIN, current_organization) + + flash[:error] = 'You are not allowed to perform this action.' + redirect_to product_drives_url + end + # Use callbacks to share common setup or constraints between actions. def set_product_drive @product_drive_info = ProductDrive.find(params[:id]) diff --git a/app/helpers/product_drive_helper.rb b/app/helpers/product_drive_helper.rb index f6dd0c3784..f5b857ab81 100644 --- a/app/helpers/product_drive_helper.rb +++ b/app/helpers/product_drive_helper.rb @@ -4,4 +4,8 @@ def is_virtual(product_drive:) product_drive.virtual? ? 'Yes' : 'No' end + + def can_delete_product_drive?(user, product_drive) + user.has_role?(Role::ORG_ADMIN, product_drive.organization) && product_drive.donations.empty? + end end diff --git a/app/models/product_drive.rb b/app/models/product_drive.rb index 999694d068..77353e8e27 100644 --- a/app/models/product_drive.rb +++ b/app/models/product_drive.rb @@ -40,6 +40,8 @@ class ProductDrive < ApplicationRecord validate :end_date_is_bigger_of_end_date + before_destroy :validate_destroy, prepend: true + def end_date_is_bigger_of_end_date return if start_date.nil? || end_date.nil? @@ -69,8 +71,11 @@ def self.search_date_range(dates) @search_date_range = { start_date: dates[0], end_date: dates[1] } end - def can_delete?(user) - user.has_role?(Role::ORG_ADMIN, organization) && donations.empty? + def validate_destroy + return if donations.empty? + + errors.add(:base, "Cannot delete product drive with donations.") + throw(:abort) end # quantities are FILTERED by date then SORTED by name diff --git a/app/views/product_drives/show.html.erb b/app/views/product_drives/show.html.erb index da244bceb4..f663d4cbeb 100644 --- a/app/views/product_drives/show.html.erb +++ b/app/views/product_drives/show.html.erb @@ -90,11 +90,10 @@

<%= edit_button_to edit_product_drive_path(@product_drive), { text: "Make a correction", size: "md" } %> - <% if @product_drive.can_delete?(current_user) %> - <%= delete_button_to product_drive_path(@product_drive), - confirm: "Are you sure you want to permanently remove this product drive?", - size: "md" %> - <% end %> + <%= delete_button_to product_drive_path(@product_drive), + confirm: "Are you sure you want to permanently remove this product drive?", + size: "md", + enabled: can_delete_product_drive?(current_user, @product_drive) %>
diff --git a/spec/requests/product_drives_requests_spec.rb b/spec/requests/product_drives_requests_spec.rb index fe6f4476ea..9e5223413c 100644 --- a/spec/requests/product_drives_requests_spec.rb +++ b/spec/requests/product_drives_requests_spec.rb @@ -209,11 +209,37 @@ end describe "DELETE #destroy" do - it "redirects to the index" do - product_drive = create(:product_drive, organization: organization) + let(:product_drive) { create(:product_drive, organization: organization) } + + context 'when user is not a org_admin' do + it "does not delete the product drive" do + delete product_drive_path(id: product_drive.id) + follow_redirect! + + expect(response.body).to include('You are not allowed to perform this action.') + end + end + + context 'when user is a org_admin' do + before do + user.add_role(Role::ORG_ADMIN, organization) + end + + it 'deletes the product drive' do + delete product_drive_path(id: product_drive.id) + follow_redirect! + + expect(response.body).to include('Product drive was successfully destroyed.') + end + + it "does not delete the product drive if it has donations" do + user.add_role(Role::ORG_ADMIN, organization) + create(:donation, product_drive: product_drive) + delete product_drive_path(id: product_drive.id) + follow_redirect! - delete product_drive_path(id: product_drive.id) - expect(response).to redirect_to(product_drives_path) + expect(response.body).to include('Cannot delete product drive with donations.') + end end end end From f7b416af05d2cd1f2500cb358c2b50bcd309420d Mon Sep 17 00:00:00 2001 From: Jorge Coutinho Date: Mon, 4 Nov 2024 20:41:39 -0300 Subject: [PATCH 4/4] Remove product drive deletion tests from system spec --- spec/system/product_drive_system_spec.rb | 30 ------------------------ 1 file changed, 30 deletions(-) diff --git a/spec/system/product_drive_system_spec.rb b/spec/system/product_drive_system_spec.rb index 67a534e11f..c60bc0122a 100644 --- a/spec/system/product_drive_system_spec.rb +++ b/spec/system/product_drive_system_spec.rb @@ -130,34 +130,4 @@ expect(page).to have_content 'Endless drive' end end - - context "when deleting a Product drive" do - let(:new_product_drive) { create(:product_drive, name: 'New Test', start_date: 3.weeks.ago, end_date: 3.weeks.from_now, organization: organization) } - let(:subject) { product_drive_path(new_product_drive.id) } - - context "when user is a org_admin" do - before do - user.add_role(:org_admin, organization) - end - - it "must delete" do - visit subject - click_on 'Delete' - expect(page).to have_content 'Product drive was successfully destroyed.' - end - - it "must not delete if there are donations" do - create(:donation, product_drive: new_product_drive) - visit subject - expect(page).not_to have_button 'Delete' - end - end - - context "when user is not a org_admin" do - it "must not delete" do - visit subject - expect(page).not_to have_button 'Delete' - end - end - end end