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 7 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
18 changes: 17 additions & 1 deletion app/controllers/product_drives_controller.rb
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -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 }
Expand All @@ -85,6 +94,13 @@ def destroy

private

def verify_role
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we move this verification to the destroy service? You can pass the user into it.

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])
Expand Down
6 changes: 6 additions & 0 deletions app/models/product_drive.rb
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,8 @@ class ProductDrive < ApplicationRecord

validate :end_date_is_bigger_of_end_date

before_destroy :validate_destroy, prepend: true
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this method defined somewhere?


def end_date_is_bigger_of_end_date
return if start_date.nil? || end_date.nil?

Expand Down Expand Up @@ -69,6 +71,10 @@ def self.search_date_range(dates)
@search_date_range = { start_date: dates[0], end_date: dates[1] }
end

def destroy_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.

Not sure this method is really worth defining when it's a one-liner.

ProductDriveDestroyService.new(self).call
end

# quantities are FILTERED by date then SORTED by name
#
# @param date_range [Range]
Expand Down
23 changes: 23 additions & 0 deletions app/services/product_drive_destroy_service.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
class ProductDriveDestroyService
def initialize(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.

Can we change call to a class method? It's easier to work with in terms of stubbing tests.

@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
5 changes: 4 additions & 1 deletion app/views/product_drives/show.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,10 @@
</p>
<div class="card-footer clearfix">
<%= 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) %>
confirm: "Are you sure you want to permanently remove this 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.

I think this was a mistake?

size: "md", <%= delete_button_to product_drive_path(@product_drive),

enabled: ProductDrivePolicy.can_destroy?(@product_drive, current_user) %>
</div>
<!-- /.card-footer-->
</div>
Expand Down
34 changes: 30 additions & 4 deletions spec/requests/product_drives_requests_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

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

This test probably should be moved to the destroy service.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, should I replace it with the code that was previously in (describe "DELETE #destroy") or would you rather I delete it?

Copy link
Collaborator

Choose a reason for hiding this comment

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

You can leave the code that was there before.

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
Expand Down
Loading