Resolves #4739 #4763
Resolves #4739 #4763
Conversation
app/models/product_drive.rb
Outdated
| @@ -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? | |||
| @@ -130,4 +130,34 @@ | |||
| expect(page).to have_content 'Endless drive' | |||
| end | |||
| end | |||
|
|
|||
| context "when deleting a Product drive" do | |||
There was a problem hiding this comment.
Can we move these tests to request tests? There's no reason to load a whole browser to test the HTML output by the controller.
There was a problem hiding this comment.
Got it! Thanks for the heads-up
Hmm. All other things being equal, I would prefer the somewhat safer solution. Do I think it's going to be an issue before such time as we would want to add deactivation? Nope. So I guess I'm ok with going forward with it if @dorner is. Should we add some warning comments? |
|
I made a few changes. I thought about keeping the "ProductDrive#can_delete?" method as it was and doing all the validation in the controller, but that way it would still be possible to delete a product drive with donations from the Rails console. So I took another approach. |
| @@ -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? | |||
There was a problem hiding this comment.
I'm a bit confused here. Can we instead move this to a policy object? Maybe under app/policies? We can pass in the product drive and the user. Then we can reuse this method across both the view and the backend. Something like:
module ProductDrivePolicy
class << self
def can_destroy?(product_drive, user)
user.has_role?(Role::ORG_ADMIN, product_drive.organization) && product_drive.donations.empty?
end
end
endand then you can call ProductDrivePolicy.can_destroy?(product_drive, user) in both places.
I know we don't have any of these policy objects yet, but I think I like this pattern for future similar use cases as well.
| @@ -69,6 +71,13 @@ def self.search_date_range(dates) | |||
| @search_date_range = { start_date: dates[0], end_date: dates[1] } | |||
| end | |||
|
|
|||
| def validate_destroy | |||
There was a problem hiding this comment.
This seems a bit heavy for a validation. I'd rather extract this to a ProductDriveDestroyService which calls the policy. That matches the pattern we have a bit better. It can be pretty lightweight.
Resolves #4739
Description
Type of change
How Has This Been Tested?