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

#4739: Fix product drive delete (updated) #4918

Open
wants to merge 19 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
39c7550
add new logic for product_drive deletion
jorgecoutinhobr Nov 1, 2024
3aec94e
add new tests for Product Drive system spec
jorgecoutinhobr Nov 1, 2024
cb17ab1
Implement new role verification and validation for product drive dele…
jorgecoutinhobr Nov 4, 2024
f7b416a
Remove product drive deletion tests from system spec
jorgecoutinhobr Nov 4, 2024
65d9b7e
Merge branch 'main' of https://github.com/MichaScant/human-essentials…
MichaScant Dec 18, 2024
83976b7
fixed proposed changes
MichaScant Jan 3, 2025
ea47123
Merge branch 'main' of https://github.com/MichaScant/human-essentials…
MichaScant Jan 6, 2025
3204abe
proposed changes
MichaScant Jan 10, 2025
677de12
fixed ruby lint errors
MichaScant Jan 10, 2025
b0ac10f
fixed error in rspec-system(6,3)
MichaScant Jan 10, 2025
045d8be
replaced ProductDrivePolicy with the can destroy in product destroy s…
MichaScant Jan 10, 2025
018c9f4
changed can_destroy to class method so it is able to be called
MichaScant Jan 10, 2025
1b8c05f
fixed rubocop errors and arguments in show.html.erb
MichaScant Jan 13, 2025
4d822f3
attempted to fixes, made can_destroy class method ot make it a class …
MichaScant Jan 13, 2025
542a1e7
removed redundant operation as product_drive is already accessible
MichaScant Jan 13, 2025
cbb18ec
no longer destroying in controller
MichaScant Jan 13, 2025
4bb1407
Merge branch 'main' of https://github.com/MichaScant/human-essentials…
MichaScant Jan 13, 2025
e8edee3
updated code based on requested changes
MichaScant Jan 16, 2025
21cf464
throwing errors in destroy service at times now as test expects it
MichaScant Jan 16, 2025
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
7 changes: 3 additions & 4 deletions app/controllers/product_drives_controller.rb
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -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
Expand Down
2 changes: 0 additions & 2 deletions app/models/product_drive.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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?

Expand Down
79 changes: 22 additions & 57 deletions app/services/product_drive_destroy_service.rb
Original file line number Diff line number Diff line change
@@ -1,64 +1,29 @@
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)

unless can_destroy?(product_drive, user)
product_drive.errors.add(:base, "Cannot delete product drive with donations.")
raise ActiveRecord::RecordInvalid.new(product_drive)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are we sometimes raising an error and sometimes setting the error to the Result object?

end

if product_drive.destroy
Result.new(value: "Product drive was successfully destroyed.")
else
raise ActiveRecord::RecordNotDestroyed.new("Failed to destroy product drive", product_drive)
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
Loading