From 39c755089f291416471923725ca29abd2a0ee870 Mon Sep 17 00:00:00 2001 From: Jorge Coutinho Date: Fri, 1 Nov 2024 13:56:03 -0300 Subject: [PATCH 01/16] 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 02/16] 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 03/16] 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 04/16] 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 From 83976b79885ac56390a70a0eec6eeb446af73543 Mon Sep 17 00:00:00 2001 From: michascant <89426143+MichaScant@users.noreply.github.com> Date: Fri, 3 Jan 2025 14:29:59 -0500 Subject: [PATCH 05/16] fixed proposed changes --- app/helpers/product_drive_helper.rb | 4 ---- app/models/product_drive.rb | 7 ++---- app/services/product_drive_destroy_service.rb | 23 +++++++++++++++++++ app/views/product_drives/show.html.erb | 6 ++--- 4 files changed, 28 insertions(+), 12 deletions(-) create mode 100644 app/services/product_drive_destroy_service.rb diff --git a/app/helpers/product_drive_helper.rb b/app/helpers/product_drive_helper.rb index f5b857ab81..f6dd0c3784 100644 --- a/app/helpers/product_drive_helper.rb +++ b/app/helpers/product_drive_helper.rb @@ -4,8 +4,4 @@ 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 77353e8e27..058d35914b 100644 --- a/app/models/product_drive.rb +++ b/app/models/product_drive.rb @@ -71,11 +71,8 @@ def self.search_date_range(dates) @search_date_range = { start_date: dates[0], end_date: dates[1] } end - def validate_destroy - return if donations.empty? - - errors.add(:base, "Cannot delete product drive with donations.") - throw(:abort) + def destroy_product_drive + ProductDriveDestroyService.new(self).call end # quantities are FILTERED by date then SORTED by name diff --git a/app/services/product_drive_destroy_service.rb b/app/services/product_drive_destroy_service.rb new file mode 100644 index 0000000000..7fbf829444 --- /dev/null +++ b/app/services/product_drive_destroy_service.rb @@ -0,0 +1,23 @@ +class ProductDriveDestroyService + def initialize(product_drive) + @product_drive = product_drive + end + + def call + return false unless can_destroy? + + # Perform destruction logic + @product_drive.destroy + end + + private + + def can_destroy? + if @product_drive.donations.empty? + true + else + @product_drive.errors.add(:base, "Cannot delete product drive with donations.") + false + end + end +end diff --git a/app/views/product_drives/show.html.erb b/app/views/product_drives/show.html.erb index f663d4cbeb..db8414138d 100644 --- a/app/views/product_drives/show.html.erb +++ b/app/views/product_drives/show.html.erb @@ -90,10 +90,10 @@

<%= 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", - enabled: can_delete_product_drive?(current_user, @product_drive) %> + size: "md", <%= delete_button_to product_drive_path(@product_drive), + + enabled: ProductDrivePolicy.can_destroy?(@product_drive, current_user) %>
From 3204abe21d11c93e6f7f512ea267fae3a8c7b543 Mon Sep 17 00:00:00 2001 From: michascant <89426143+MichaScant@users.noreply.github.com> Date: Fri, 10 Jan 2025 16:50:00 -0500 Subject: [PATCH 06/16] proposed changes --- app/controllers/product_drives_controller.rb | 9 +--- app/models/product_drive.rb | 4 -- app/services/product_drive_destroy_service.rb | 46 +++++++++++++++++-- app/views/product_drives/show.html.erb | 5 +- spec/requests/product_drives_requests_spec.rb | 34 ++------------ 5 files changed, 47 insertions(+), 51 deletions(-) diff --git a/app/controllers/product_drives_controller.rb b/app/controllers/product_drives_controller.rb index 67c24a268d..6abe8f3064 100644 --- a/app/controllers/product_drives_controller.rb +++ b/app/controllers/product_drives_controller.rb @@ -1,7 +1,7 @@ class ProductDrivesController < ApplicationController include Importable before_action :set_product_drive, only: [:show, :edit, :update, :destroy] - before_action :verify_role, only: :destroy + before_action only: :destroy def index setup_date_range_picker @@ -94,13 +94,6 @@ 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/models/product_drive.rb b/app/models/product_drive.rb index 058d35914b..dd4376efbf 100644 --- a/app/models/product_drive.rb +++ b/app/models/product_drive.rb @@ -71,10 +71,6 @@ def self.search_date_range(dates) @search_date_range = { start_date: dates[0], end_date: dates[1] } end - def destroy_product_drive - ProductDriveDestroyService.new(self).call - end - # quantities are FILTERED by date then SORTED by name # # @param date_range [Range] diff --git a/app/services/product_drive_destroy_service.rb b/app/services/product_drive_destroy_service.rb index 7fbf829444..0ecf32491d 100644 --- a/app/services/product_drive_destroy_service.rb +++ b/app/services/product_drive_destroy_service.rb @@ -1,23 +1,59 @@ class ProductDriveDestroyService - def initialize(product_drive) + attr_reader :product_drive, :user, :organization + + def initialize(product_drive, user, organization) @product_drive = product_drive + @user = user + @organization = organization + end + + def self.call(product_drive, user, organization) + new(product_drive, user, organization).call end def call - return false unless can_destroy? + return unauthorized_error unless verify_role + return donation_error unless can_destroy? - # Perform destruction logic - @product_drive.destroy + product_drive = organization.product_drives.find(product_drive.id) + product_drive.destroy + + if product_drive.errors.any? + { + success: false, + message: product_drive.errors.full_messages.join("\n") + } + else + { + success: true, + message: 'Product drive was successfully destroyed.' + } + end end private + def verify_role + return true if user.has_role?(Role::ORG_ADMIN, @organization) + flash[:error] = 'You are not allowed to perform this action.' + redirect_to product_drives_url + end + def can_destroy? - if @product_drive.donations.empty? + if product_drive.donations.empty? true else @product_drive.errors.add(:base, "Cannot delete product drive with donations.") false end end + + def unauthorized_error + { success: false, message: "You are not allowed to perform this action." } + end + + def donation_error + product_drive.errors.add(:base, "Cannot delete product drive with donations.") + { success: false, message: "Cannot delete product drive with donations." } + end end diff --git a/app/views/product_drives/show.html.erb b/app/views/product_drives/show.html.erb index db8414138d..29b4f59c01 100644 --- a/app/views/product_drives/show.html.erb +++ b/app/views/product_drives/show.html.erb @@ -90,10 +90,7 @@

<%= edit_button_to edit_product_drive_path(@product_drive), { text: "Make a correction", size: "md" } %> - confirm: "Are you sure you want to permanently remove this product drive?", - size: "md", <%= delete_button_to product_drive_path(@product_drive), - - enabled: ProductDrivePolicy.can_destroy?(@product_drive, current_user) %> + <%= delete_button_to product_drive_path(@product_drive), { confirm: "Are you sure you want to permanently remove this product drive?", size: "md" }, enabled: ProductDrivePolicy.can_destroy?(@product_drive, current_user) %>
diff --git a/spec/requests/product_drives_requests_spec.rb b/spec/requests/product_drives_requests_spec.rb index 9e5223413c..fe6f4476ea 100644 --- a/spec/requests/product_drives_requests_spec.rb +++ b/spec/requests/product_drives_requests_spec.rb @@ -209,37 +209,11 @@ end describe "DELETE #destroy" do - 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! + it "redirects to the index" do + product_drive = create(:product_drive, organization: organization) - expect(response.body).to include('Cannot delete product drive with donations.') - end + delete product_drive_path(id: product_drive.id) + expect(response).to redirect_to(product_drives_path) end end end From 677de1252fe2720d518ee1b0ed7192f195100f38 Mon Sep 17 00:00:00 2001 From: michascant <89426143+MichaScant@users.noreply.github.com> Date: Fri, 10 Jan 2025 16:58:20 -0500 Subject: [PATCH 07/16] fixed ruby lint errors --- app/services/product_drive_destroy_service.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/app/services/product_drive_destroy_service.rb b/app/services/product_drive_destroy_service.rb index 0ecf32491d..39a5c74d54 100644 --- a/app/services/product_drive_destroy_service.rb +++ b/app/services/product_drive_destroy_service.rb @@ -26,7 +26,7 @@ def call else { success: true, - message: 'Product drive was successfully destroyed.' + message: "Product drive was successfully destroyed." } end end @@ -35,7 +35,7 @@ def call def verify_role return true if user.has_role?(Role::ORG_ADMIN, @organization) - flash[:error] = 'You are not allowed to perform this action.' + flash[:error] = "You are not allowed to perform this action." redirect_to product_drives_url end @@ -49,11 +49,11 @@ def can_destroy? end def unauthorized_error - { success: false, message: "You are not allowed to perform this action." } + {success: false, message: "You are not allowed to perform this action."} end def donation_error product_drive.errors.add(:base, "Cannot delete product drive with donations.") - { success: false, message: "Cannot delete product drive with donations." } + {success: false, message: "Cannot delete product drive with donations."} end end From b0ac10f9593dcd6ff28f7de33c86da36e4d32cf3 Mon Sep 17 00:00:00 2001 From: michascant <89426143+MichaScant@users.noreply.github.com> Date: Fri, 10 Jan 2025 17:03:46 -0500 Subject: [PATCH 08/16] fixed error in rspec-system(6,3) --- app/services/product_drive_destroy_service.rb | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/app/services/product_drive_destroy_service.rb b/app/services/product_drive_destroy_service.rb index 39a5c74d54..f43fd0b732 100644 --- a/app/services/product_drive_destroy_service.rb +++ b/app/services/product_drive_destroy_service.rb @@ -35,8 +35,7 @@ def call def verify_role return true if user.has_role?(Role::ORG_ADMIN, @organization) - flash[:error] = "You are not allowed to perform this action." - redirect_to product_drives_url + false end def can_destroy? From 045d8be942b17f198e78a62771a9e4659b610ecb Mon Sep 17 00:00:00 2001 From: michascant <89426143+MichaScant@users.noreply.github.com> Date: Fri, 10 Jan 2025 17:32:35 -0500 Subject: [PATCH 09/16] replaced ProductDrivePolicy with the can destroy in product destroy service --- app/views/product_drives/show.html.erb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/product_drives/show.html.erb b/app/views/product_drives/show.html.erb index 29b4f59c01..d5260db14c 100644 --- a/app/views/product_drives/show.html.erb +++ b/app/views/product_drives/show.html.erb @@ -90,7 +90,7 @@

<%= 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" }, enabled: ProductDrivePolicy.can_destroy?(@product_drive, current_user) %> + <%= delete_button_to product_drive_path(@product_drive), { confirm: "Are you sure you want to permanently remove this product drive?", size: "md" }, enabled: ProductDriveDestroyService.can_destroy?(@product_drive, current_user) %>
From 018c9f42d1b5e470e11c3b931df3594e6adf1c1b Mon Sep 17 00:00:00 2001 From: michascant <89426143+MichaScant@users.noreply.github.com> Date: Fri, 10 Jan 2025 18:06:50 -0500 Subject: [PATCH 10/16] changed can_destroy to class method so it is able to be called --- app/services/product_drive_destroy_service.rb | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/app/services/product_drive_destroy_service.rb b/app/services/product_drive_destroy_service.rb index f43fd0b732..aa17bfabb9 100644 --- a/app/services/product_drive_destroy_service.rb +++ b/app/services/product_drive_destroy_service.rb @@ -13,7 +13,7 @@ def self.call(product_drive, user, organization) def call return unauthorized_error unless verify_role - return donation_error unless can_destroy? + return donation_error unless self.class.can_destroy?(product_drive, user) product_drive = organization.product_drives.find(product_drive.id) product_drive.destroy @@ -38,11 +38,13 @@ def verify_role false end - def can_destroy? + def self.can_destroy?(product_drive, user) + return false unless user.has_role?(Role::ORG_ADMIN, product_drive.organization) + if product_drive.donations.empty? true else - @product_drive.errors.add(:base, "Cannot delete product drive with donations.") + product_drive.errors.add(:base, "Cannot delete product drive with donations.") false end end From 1b8c05fa24e60a06ceb3bf07b180b9e8190a9f16 Mon Sep 17 00:00:00 2001 From: michascant <89426143+MichaScant@users.noreply.github.com> Date: Mon, 13 Jan 2025 15:16:13 -0500 Subject: [PATCH 11/16] fixed rubocop errors and arguments in show.html.erb --- app/services/product_drive_destroy_service.rb | 4 ++-- app/views/product_drives/show.html.erb | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/app/services/product_drive_destroy_service.rb b/app/services/product_drive_destroy_service.rb index aa17bfabb9..7b7bce706d 100644 --- a/app/services/product_drive_destroy_service.rb +++ b/app/services/product_drive_destroy_service.rb @@ -13,7 +13,7 @@ def self.call(product_drive, user, organization) def call return unauthorized_error unless verify_role - return donation_error unless self.class.can_destroy?(product_drive, user) + return donation_error unless self.class.can_destroy?(product_drive) product_drive = organization.product_drives.find(product_drive.id) product_drive.destroy @@ -38,7 +38,7 @@ def verify_role false end - def self.can_destroy?(product_drive, user) + def can_destroy?(product_drive, user) return false unless user.has_role?(Role::ORG_ADMIN, product_drive.organization) if product_drive.donations.empty? diff --git a/app/views/product_drives/show.html.erb b/app/views/product_drives/show.html.erb index d5260db14c..906c925cf4 100644 --- a/app/views/product_drives/show.html.erb +++ b/app/views/product_drives/show.html.erb @@ -90,7 +90,7 @@

<%= 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" }, enabled: ProductDriveDestroyService.can_destroy?(@product_drive, current_user) %> + <%= delete_button_to product_drive_path(@product_drive), { confirm: "Are you sure you want to permanently remove this product drive?", size: "md", enabled: ProductDriveDestroyService.can_destroy?(@product_drive, current_user)} %>
From 4d822f3b75c12f94cdddcbaf443389a549a8ffac Mon Sep 17 00:00:00 2001 From: michascant <89426143+MichaScant@users.noreply.github.com> Date: Mon, 13 Jan 2025 15:34:06 -0500 Subject: [PATCH 12/16] attempted to fixes, made can_destroy class method ot make it a class method --- app/services/product_drive_destroy_service.rb | 25 +++++++++++-------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/app/services/product_drive_destroy_service.rb b/app/services/product_drive_destroy_service.rb index 7b7bce706d..7dfc12486e 100644 --- a/app/services/product_drive_destroy_service.rb +++ b/app/services/product_drive_destroy_service.rb @@ -11,9 +11,14 @@ def self.call(product_drive, user, organization) new(product_drive, user, organization).call end + def self.can_destroy?(product_drive, user) + return false unless user.has_role?(Role::ORG_ADMIN, product_drive.organization) + product_drive.donations.empty? + end + def call return unauthorized_error unless verify_role - return donation_error unless self.class.can_destroy?(product_drive) + return donation_error unless self.class.can_destroy?(product_drive, user) product_drive = organization.product_drives.find(product_drive.id) product_drive.destroy @@ -38,16 +43,16 @@ def verify_role false end - def can_destroy?(product_drive, user) - return false unless user.has_role?(Role::ORG_ADMIN, product_drive.organization) + # def can_destroy?(product_drive, user) + # return false unless user.has_role?(Role::ORG_ADMIN, product_drive.organization) - if product_drive.donations.empty? - true - else - product_drive.errors.add(:base, "Cannot delete product drive with donations.") - false - end - end + # if product_drive.donations.empty? + # true + # else + # product_drive.errors.add(:base, "Cannot delete product drive with donations.") + # false + # end + # end def unauthorized_error {success: false, message: "You are not allowed to perform this action."} From 542a1e75317eca658ec74d9475b3b6b3ba124597 Mon Sep 17 00:00:00 2001 From: michascant <89426143+MichaScant@users.noreply.github.com> Date: Mon, 13 Jan 2025 15:58:49 -0500 Subject: [PATCH 13/16] removed redundant operation as product_drive is already accessible --- app/services/product_drive_destroy_service.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/app/services/product_drive_destroy_service.rb b/app/services/product_drive_destroy_service.rb index 7dfc12486e..f3b9ebf679 100644 --- a/app/services/product_drive_destroy_service.rb +++ b/app/services/product_drive_destroy_service.rb @@ -20,7 +20,6 @@ def call return unauthorized_error unless verify_role return donation_error unless self.class.can_destroy?(product_drive, user) - product_drive = organization.product_drives.find(product_drive.id) product_drive.destroy if product_drive.errors.any? From cbb18ec05bc721be5cdc90068ec1d75e92511bed Mon Sep 17 00:00:00 2001 From: michascant <89426143+MichaScant@users.noreply.github.com> Date: Mon, 13 Jan 2025 16:06:08 -0500 Subject: [PATCH 14/16] no longer destroying in controller --- app/controllers/product_drives_controller.rb | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/app/controllers/product_drives_controller.rb b/app/controllers/product_drives_controller.rb index 6abe8f3064..7260bdd299 100644 --- a/app/controllers/product_drives_controller.rb +++ b/app/controllers/product_drives_controller.rb @@ -77,18 +77,13 @@ def update end def destroy - product_drive = current_organization.product_drives.find(params[:id]) - product_drive.destroy + result = ProductDriveDestroyService.call(@product_drive, current_user, current_organization) - if product_drive.errors.any? - flash[:error] = product_drive.errors.full_messages.join("\n") + if result[:success] + redirect_to product_drives_url, notice: result[:message] + else + flash[:error] = result[:message] 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 } end end From e8edee3e82ec768a0c7d5fcdced2a2b795c0aa52 Mon Sep 17 00:00:00 2001 From: michascant <89426143+MichaScant@users.noreply.github.com> Date: Thu, 16 Jan 2025 14:57:02 -0500 Subject: [PATCH 15/16] updated code based on requested changes --- app/controllers/product_drives_controller.rb | 7 +- app/models/product_drive.rb | 2 - app/services/product_drive_destroy_service.rb | 75 +++++-------------- 3 files changed, 21 insertions(+), 63 deletions(-) diff --git a/app/controllers/product_drives_controller.rb b/app/controllers/product_drives_controller.rb index 7260bdd299..c7be796378 100644 --- a/app/controllers/product_drives_controller.rb +++ b/app/controllers/product_drives_controller.rb @@ -1,7 +1,6 @@ class ProductDrivesController < ApplicationController include Importable before_action :set_product_drive, only: [:show, :edit, :update, :destroy] - before_action only: :destroy def index setup_date_range_picker @@ -79,10 +78,10 @@ def update def destroy result = ProductDriveDestroyService.call(@product_drive, current_user, current_organization) - if result[:success] - redirect_to product_drives_url, notice: result[:message] + if result.success? + redirect_to product_drives_url, notice: result.value else - flash[:error] = result[:message] + flash[:error] = result.error redirect_back(fallback_location: product_drives_url) end end diff --git a/app/models/product_drive.rb b/app/models/product_drive.rb index dd4376efbf..f9a36a4b87 100644 --- a/app/models/product_drive.rb +++ b/app/models/product_drive.rb @@ -40,8 +40,6 @@ 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? diff --git a/app/services/product_drive_destroy_service.rb b/app/services/product_drive_destroy_service.rb index f3b9ebf679..978679d00e 100644 --- a/app/services/product_drive_destroy_service.rb +++ b/app/services/product_drive_destroy_service.rb @@ -1,64 +1,25 @@ class ProductDriveDestroyService - attr_reader :product_drive, :user, :organization - - def initialize(product_drive, user, organization) - @product_drive = product_drive - @user = user - @organization = organization - end - - def self.call(product_drive, user, organization) - new(product_drive, user, organization).call - end - - def self.can_destroy?(product_drive, user) - return false unless user.has_role?(Role::ORG_ADMIN, product_drive.organization) - product_drive.donations.empty? - end - - def call - return unauthorized_error unless verify_role - return donation_error unless self.class.can_destroy?(product_drive, user) - - product_drive.destroy - - if product_drive.errors.any? - { - success: false, - message: product_drive.errors.full_messages.join("\n") - } - else - { - success: true, - message: "Product drive was successfully destroyed." - } + class << self + def call(product_drive, user, organization) + return Result.new(error: "You are not allowed to perform this action.") unless verify_role(user, organization) + return Result.new(error: "Cannot delete product drive with donations.") unless can_destroy?(product_drive, user) + + if product_drive.destroy + Result.new(value: "Product drive was successfully destroyed.") + else + Result.new(error: product_drive.errors.full_messages.join("\n")) + end end - end - - private - - def verify_role - return true if user.has_role?(Role::ORG_ADMIN, @organization) - false - end - # def can_destroy?(product_drive, user) - # return false unless user.has_role?(Role::ORG_ADMIN, product_drive.organization) - - # if product_drive.donations.empty? - # true - # else - # product_drive.errors.add(:base, "Cannot delete product drive with donations.") - # false - # end - # end + def can_destroy?(product_drive, user) + return false unless user.has_role?(Role::ORG_ADMIN, product_drive.organization) + product_drive.donations.empty? + end - def unauthorized_error - {success: false, message: "You are not allowed to perform this action."} - end + private - def donation_error - product_drive.errors.add(:base, "Cannot delete product drive with donations.") - {success: false, message: "Cannot delete product drive with donations."} + def verify_role(user, organization) + user.has_role?(Role::ORG_ADMIN, organization) + end end end From 21cf464a628b9fbcc102d263546e56f427dda6ad Mon Sep 17 00:00:00 2001 From: michascant <89426143+MichaScant@users.noreply.github.com> Date: Thu, 16 Jan 2025 15:21:42 -0500 Subject: [PATCH 16/16] throwing errors in destroy service at times now as test expects it --- app/services/product_drive_destroy_service.rb | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/app/services/product_drive_destroy_service.rb b/app/services/product_drive_destroy_service.rb index 978679d00e..ea6533bc90 100644 --- a/app/services/product_drive_destroy_service.rb +++ b/app/services/product_drive_destroy_service.rb @@ -2,12 +2,16 @@ class ProductDriveDestroyService class << self def call(product_drive, user, organization) return Result.new(error: "You are not allowed to perform this action.") unless verify_role(user, organization) - return Result.new(error: "Cannot delete product drive with donations.") unless can_destroy?(product_drive, user) + + unless can_destroy?(product_drive, user) + product_drive.errors.add(:base, "Cannot delete product drive with donations.") + raise ActiveRecord::RecordInvalid.new(product_drive) + end if product_drive.destroy Result.new(value: "Product drive was successfully destroyed.") else - Result.new(error: product_drive.errors.full_messages.join("\n")) + raise ActiveRecord::RecordNotDestroyed.new("Failed to destroy product drive", product_drive) end end