4949-Delete vendors if no purchases (#5051)

* adds delete button and route

* delete vendors and specs

* apply pr suggestions

* fix spec

* changes expectation to be more meaningful

Author: jorgedjr21

app/controllers/vendors_controller.rb

@@ -88,6 +88,18 @@ def reactivate
     redirect_to vendors_path, notice: "#{vendor.business_name} has been reactivated."
   end
 
+  def destroy
+    vendor = current_organization.vendors.find(params[:id])
+    vendor.destroy
+    if vendor.errors.any?
+      errors = vendor.errors.full_messages.join("\n")
+      redirect_to vendors_path, error: "#{vendor.business_name} could not be removed. \n#{errors}"
+      return
+    end
+
+    redirect_to vendors_path, notice: "#{vendor.business_name} has been removed."
+  end
+
   private
 
   def vendor_params

app/models/vendor.rb

@@ -35,6 +35,8 @@ class Vendor < ApplicationRecord
       .group(:id)
   }
 
+  before_destroy :check_for_purchases, prepend: true
+
   def volume
     LineItem.where(
       itemizable_type: "Purchase",
@@ -49,4 +51,13 @@ def deactivate!
   def reactivate!
     update!(active: true)
   end
+
+  private
+
+  def check_for_purchases
+    return if purchases.empty?
+
+    errors.add(:base, "Vendor has purchase items")
+    throw(:abort)
+  end
 end

app/views/vendors/_vendor_row.html.erb

@@ -8,9 +8,15 @@
     <%= view_button_to vendor_row %>
     <%= edit_button_to edit_vendor_path(vendor_row) %>
     <% if vendor_row.active? %>
-      <%= deactivate_button_to deactivate_vendor_path(vendor_row),
-                             text: 'Deactivate',
-                             confirm: confirm_deactivate_msg(vendor_row.business_name) %>
+      <% if vendor_row&.purchases&.none? %>
+        <%= delete_button_to vendor_path(vendor_row),
+                          text: 'Delete',
+                          confirm: confirm_delete_msg(vendor_row.business_name) %>
+      <% else %>
+        <%= deactivate_button_to deactivate_vendor_path(vendor_row),
+                              text: 'Deactivate',
+                              confirm: confirm_deactivate_msg(vendor_row.business_name) %>
+      <% end %>
     <% else %>
       <%= reactivate_button_to reactivate_vendor_path(vendor_row),
                              text: 'Reactivate',

config/routes.rb

@@ -172,7 +172,7 @@ def set_up_flipper
     end
   end
 
-  resources :vendors, except: [:destroy] do
+  resources :vendors do
     collection do
       post :import_csv
     end

spec/requests/vendors_requests_spec.rb

@@ -16,21 +16,32 @@
       before { create(:vendor) }
 
       context "html" do
-        let!(:first_vendor) { create(:vendor, business_name: "Abc", organization: organization) }
+        let!(:no_purchases_vendor) { create(:vendor, business_name: "Abc", organization: organization) }
+        let(:purchase_vendor) { create(:vendor, business_name: "Xyz", organization: organization) }
         let!(:deactivated_vendor) { create(:vendor, business_name: "Deactivated", organization: organization, active: false) }
 
         let(:response_format) { 'html' }
 
         it { is_expected.to be_successful }
 
+        before do
+          create(:purchase, :with_items, vendor: purchase_vendor)
+        end
+
         it "should have only activated vendor names" do
           subject
-          expect(response.body).to include(first_vendor.business_name)
+          expect(response.body).to include(no_purchases_vendor.business_name)
           expect(response.body).not_to include(deactivated_vendor.business_name)
         end
 
-        it "should have a deactivate button for each active vendor" do
-          expect(subject.body.scan("Deactivate").count).to eq(2)
+        it "should have a delete button for no_purchases_vendor and a deactivate button for purchase_vendor" do
+          subject
+          parsed_body = Nokogiri::HTML(response.body)
+          no_purchases_vendor_row = parsed_body.css("tr").find { |row| row.text.include?(no_purchases_vendor.business_name) }
+          purchase_vendor_row = parsed_body.css("tr").find { |row| row.text.include?(purchase_vendor.business_name) }
+
+          expect(no_purchases_vendor_row.at_css("a", text: "Delete")).to be_present
+          expect(purchase_vendor_row.at_css("a", text: "Deactivate")).to be_present
         end
       end
 
@@ -115,10 +126,28 @@
     end
 
     describe "DELETE #destroy" do
-      subject { delete vendor_path(id: create(:vendor)) }
-      it "does not have a route for this" do
-        subject
-        expect(response.code).to eq('404')
+      let!(:vendor) { create(:vendor, organization: organization) }
+
+      subject { delete vendor_path(id: vendor.id) }
+
+      context 'when vendor does not have purchase items' do
+        it 'shoud delete the vendor' do
+          expect { subject }.to change(Vendor, :count)
+          expect(response).to redirect_to(vendors_path)
+          follow_redirect!
+          expect(response.body).to include("#{vendor.business_name} has been removed.")
+        end
+      end
+
+      context 'when vendor has purchase items' do
+        before do
+          create(:purchase, :with_items, vendor: vendor)
+        end
+
+        it 'shoud not delete the vendor' do
+          expect { subject }.not_to change(Vendor, :count)
+          expect(response).to have_error(/ could not be removed/)
+        end
       end
     end
 

spec/system/vendor_system_spec.rb

@@ -9,13 +9,20 @@
   context "When a user views the index page" do
     before(:each) do
       @second = create(:vendor, business_name: "Bcd")
+      create(:purchase, :with_items, vendor: @second)
       @first = create(:vendor, business_name: "Abc")
+      create(:purchase, :with_items, vendor: @first)
+
       @third = create(:vendor, business_name: "Cde")
+      create(:purchase, :with_items, vendor: @third)
+
+      @last = create(:vendor, business_name: "Zzz")
+
       visit vendors_path
     end
 
     it "should have the vendor names in alphabetical order" do
-      expect(page).to have_xpath("//table//tr", count: 4)
+      expect(page).to have_xpath("//table//tr", count: 5)
       expect(page.find(:xpath, "//table/tbody/tr[1]/td[1]")).to have_content(@first.business_name)
       expect(page.find(:xpath, "//table/tbody/tr[3]/td[1]")).to have_content(@third.business_name)
     end
@@ -33,6 +40,11 @@
       expect { click_link "Reactivate", match: :first }.to change { @first.reload.active }.to(true)
     end
 
+    it "should delete when vendor does not have purchases items" do
+      expect { click_link "Delete" }.to change(Vendor, :count).by(-1)
+      expect(page).to have_content("#{@last.business_name} has been removed.")
+    end
+
     context "When using the include_inactive_vendors filter" do
       before(:each) do
         @active_vendor = create(:vendor, business_name: "Active Vendor", active: true)