#4739: Fix product drive delete (updated) #4918
Conversation
…into 4739-fix-product-drive-delete
…into 4739-fix-product-drive-delete
app/models/product_drive.rb
Outdated
| @@ -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 | |||
There was a problem hiding this comment.
Not sure this method is really worth defining when it's a one-liner.
| @@ -0,0 +1,23 @@ | |||
| class ProductDriveDestroyService | |||
| def initialize(product_drive) | |||
There was a problem hiding this comment.
Can we change call to a class method? It's easier to work with in terms of stubbing tests.
| @@ -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?", | |||
There was a problem hiding this comment.
I think this was a mistake?
| expect(response.body).to include('Product drive was successfully destroyed.') | ||
| end | ||
|
|
||
| it "does not delete the product drive if it has donations" do |
There was a problem hiding this comment.
This test probably should be moved to the destroy service.
There was a problem hiding this comment.
ok, should I replace it with the code that was previously in (describe "DELETE #destroy") or would you rather I delete it?
There was a problem hiding this comment.
You can leave the code that was there before.
| @@ -85,6 +94,13 @@ def destroy | |||
|
|
|||
| private | |||
|
|
|||
| def verify_role | |||
There was a problem hiding this comment.
Can we move this verification to the destroy service? You can pass the user into it.
…into fix-product-drive-delete
| @@ -1,6 +1,7 @@ | |||
| class ProductDrivesController < ApplicationController | |||
| include Importable | |||
| before_action :set_product_drive, only: [:show, :edit, :update, :destroy] | |||
| before_action only: :destroy | |||
There was a problem hiding this comment.
Is this missing something, or am I reading it wrong?
There was a problem hiding this comment.
I don't believe so, although it can probably be removed seeing as the above line already is doing it along with other before_actions
| format.json { head :no_content } | ||
| result = ProductDriveDestroyService.call(@product_drive, current_user, current_organization) | ||
|
|
||
| if result[:success] |
There was a problem hiding this comment.
We have a Result class that can be used for this now.
There was a problem hiding this comment.
ok will modify such that it uses the result class, and will change the destroy service to return result as well
app/models/product_drive.rb
Outdated
| @@ -40,6 +40,8 @@ class ProductDrive < ApplicationRecord | |||
|
|
|||
| validate :end_date_is_bigger_of_end_date | |||
|
|
|||
| before_destroy :validate_destroy, prepend: true | |||
There was a problem hiding this comment.
Is this method defined somewhere?
| end | ||
|
|
||
| def self.call(product_drive, user, organization) | ||
| new(product_drive, user, organization).call |
There was a problem hiding this comment.
um... my point was that we do not need to call new at all. All methods can be class-level methods (you can wrap the whole shebang in a class << self block to make it simpler).
| # product_drive.errors.add(:base, "Cannot delete product drive with donations.") | ||
| # false | ||
| # end | ||
| # end |
There was a problem hiding this comment.
Do we need this commented-out code?
Resolves #4739
Builds of the suggested changes from PR: #4763