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

Inactivating vendors #4959

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion app/controllers/purchases_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ def destroy
def load_form_collections
@storage_locations = current_organization.storage_locations.active.alphabetized
@items = current_organization.items.active.alphabetized
@vendors = current_organization.vendors.alphabetized
@vendors = current_organization.vendors.active.alphabetized
end

def purchase_params
Expand Down
45 changes: 42 additions & 3 deletions app/controllers/vendors_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,14 @@ class VendorsController < ApplicationController
include Importable

def index
@vendors = current_organization.vendors.with_volumes.alphabetized
@vendors = current_organization
.vendors
.with_volumes
.alphabetized
.class_filter(filter_params)

@vendors = @vendors.active unless params[:include_inactive_vendors]
@include_inactive_vendors = params[:include_inactive_vendors]

respond_to do |format|
format.html
Expand Down Expand Up @@ -53,6 +60,36 @@ def update
end
end

def deactivate
vendor = current_organization.vendors.find(params[:id])

begin
vendor.deactivate!
rescue => e
flash[:error] = e.message
redirect_back(fallback_location: vendors_path)
return
end

flash[:notice] = "#{vendor.business_name} has been deactivated."
Comment on lines +69 to +74
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 combine redirect and error/notice:

redirect_back(fallback_location: vendors_path, error: e.message)
redirect_to vendors_path, notice: "#{vendor.business_name} has been deactivated."

redirect_to vendors_path
end

def reactivate
vendor = current_organization.vendors.find(params[:id])

begin
vendor.reactivate!
rescue => e
flash[:error] = e.message
redirect_back(fallback_location: vendors_path)
return
end

flash[:notice] = "#{vendor.business_name} has been reactivated."
redirect_to vendors_path
end

private

def vendor_params
Expand All @@ -61,7 +98,9 @@ def vendor_params
end

helper_method \
def filter_params
{}
def filter_params(_parameters = nil)
return {} unless params.key?(:filters)

params.require(:filters).permit(:include_inactive_vendors)
end
end
16 changes: 15 additions & 1 deletion app/models/vendor.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
# Table name: vendors
#
# id :bigint not null, primary key
# active :boolean default(TRUE)
# address :string
# business_name :string
# comment :string
Expand All @@ -20,16 +21,29 @@ class Vendor < ApplicationRecord
has_paper_trail
include Provideable
include Geocodable
include Filterable

has_many :purchases, inverse_of: :vendor, dependent: :destroy

validates :business_name, presence: true

scope :alphabetized, -> { order(:business_name) }

scope :active, -> { where(active: true) }
scope :with_volumes, -> {
left_joins(purchases: :line_items)
.select("vendors.*, SUM(COALESCE(line_items.quantity, 0)) AS volume")
.group(:id)
}

def volume
purchases.map { |d| d.line_items.total }.reduce(:+)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is going to be slow - better to do it in the database. Something like

LineItem.joins(:itemizable).
  where(itemizable_type: 'Purchase', itemizable_id: self.purchase_ids).
  sum(:quantity)

end

def deactivate!
update!(active: false)
end

def reactivate!
update!(active: true)
end
end
9 changes: 9 additions & 0 deletions app/views/vendors/_vendor_row.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -7,5 +7,14 @@
<td class="text-center">
<%= 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) %>
<% else %>
<%= reactivate_button_to reactivate_vendor_path(vendor_row),
text: 'Reactivate',
confirm: confirm_reactivate_msg(vendor_row.business_name) %>
<% end %>
</td>
</tr>
32 changes: 25 additions & 7 deletions app/views/vendors/index.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -29,20 +29,38 @@
<div class="col-md-12">
<!-- jquery validation -->
<div class="card card-primary">
<div class="card-footer">
<div class="pull-right">
<%= modal_button_to("#csvImportModal", { icon: "upload", text: "Import Vendors", size: "md"}) if @vendors.empty? %>
<%= download_button_to(vendors_path(format: :csv, filters: filter_params.merge(date_range: date_range_params)), {text: "Export Vendors", size: "md"}) if @vendors.any? %>
<%= new_button_to new_vendor_path, text: "New Vendor" %> </div>
<div class="card-header">
<h3 class="card-title">Vendor Filter</h3>
</div>
</div>
<!-- /.card-header -->
<!-- form start -->
<div class="card-body">
<%= form_tag(vendors_path, method: :get) do |f| %>
<div class="row">
<div class="form-group col-lg-3 col-md-4 col-sm-6 col-xs-12">
<%= filter_checkbox(label: "Also include inactive vendors", scope: "include_inactive_vendors", selected: @include_inactive_vendors) %>
</div>
</div>
</div>
<div class="card-footer">
<%= filter_button %>
<%= clear_filter_button %>
<span class="float-right">
<%= download_button_to(vendors_path(format: :csv, filters: filter_params.merge(date_range: date_range_params, include_inactive_vendors: @include_inactive_vendors)), {text: "Export Vendors", size: "md"}) if @vendors.any? %>
<%= modal_button_to("#csvImportModal", { icon: "upload", text: "Import Vendors", size: "md"}) if @vendors.empty? %>
<%= new_button_to new_vendor_path, text: "New Vendor" %>
</span>
</div>
<% end # form %>
</div>
<!-- /.card -->
</div>
<!--/.col (left) -->
</div>
<!-- /.row -->
</div><!-- /.container-fluid -->
</section>

<section class="content">
<div class="container-fluid">
<div class="row">
<div class="col-12">
Expand Down
4 changes: 4 additions & 0 deletions config/routes.rb
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,10 @@ def set_up_flipper
collection do
post :import_csv
end
member do
put :deactivate
put :reactivate
end
end

resources :kits do
Expand Down
11 changes: 11 additions & 0 deletions db/migrate/20250129154820_add_active_to_vendors.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
# Add active column to vendors to make it possible to delete vendors without actually deleting them
class AddActiveToVendors < ActiveRecord::Migration[7.2]
def up
add_column :vendors, :active, :boolean
change_column_default :vendors, :active, true
end

def down
remove_column :vendors, :active
end
end
6 changes: 6 additions & 0 deletions db/migrate/20250201151720_set_existing_vendors_active.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
# Add active column to vendors to make it possible to delete vendors without actually deleting them
class SetExistingVendorsActive < ActiveRecord::Migration[7.2]
def change
Vendor.update_all(active: true)
end
end
57 changes: 41 additions & 16 deletions db/schema.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
#
# It's strongly recommended that you check this file into your version control system.

ActiveRecord::Schema[7.2].define(version: 2025_01_29_015253) do
ActiveRecord::Schema[7.2].define(version: 2025_02_01_151720) do
# These are extensions that must be enabled in order to support this database
enable_extension "plpgsql"

Expand Down Expand Up @@ -230,6 +230,31 @@
t.index ["user_id"], name: "index_deprecated_feedback_messages_on_user_id"
end

create_table "diaper_drive_participants", id: :serial, force: :cascade do |t|
Copy link
Collaborator

Choose a reason for hiding this comment

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

These all seem unrelated to the PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, seems very strange these tables here, I think I will remove this changes and keep only the ones related with the migration I created

t.string "contact_name"
t.string "email"
t.string "phone"
t.string "comment"
t.integer "organization_id"
t.datetime "created_at", precision: nil, null: false
t.datetime "updated_at", precision: nil, null: false
t.string "address"
t.string "business_name"
t.float "latitude"
t.float "longitude"
t.index ["latitude", "longitude"], name: "index_diaper_drive_participants_on_latitude_and_longitude"
end

create_table "diaper_drives", force: :cascade do |t|
t.string "name"
t.date "start_date"
t.date "end_date"
t.datetime "created_at", precision: nil, null: false
t.datetime "updated_at", precision: nil, null: false
t.bigint "organization_id"
t.index ["organization_id"], name: "index_diaper_drives_on_organization_id"
end

create_table "distributions", id: :serial, force: :cascade do |t|
t.text "comment"
t.datetime "created_at", precision: nil, null: false
Expand Down Expand Up @@ -325,6 +350,16 @@
t.index ["partner_id"], name: "index_families_on_partner_id"
end

create_table "feedback_messages", force: :cascade do |t|
t.bigint "user_id"
t.text "message"
t.string "path"
t.datetime "created_at", precision: nil, null: false
t.datetime "updated_at", precision: nil, null: false
t.boolean "resolved"
t.index ["user_id"], name: "index_feedback_messages_on_user_id"
end

create_table "flipper_features", force: :cascade do |t|
t.string "key", null: false
t.datetime "created_at", precision: nil, null: false
Expand Down Expand Up @@ -513,8 +548,8 @@
t.integer "deadline_day"
t.index ["name", "organization_id"], name: "index_partner_groups_on_name_and_organization_id", unique: true
t.index ["organization_id"], name: "index_partner_groups_on_organization_id"
t.check_constraint "deadline_day <= 28", name: "deadline_day_of_month_check", validate: false
t.check_constraint "reminder_day <= 28", name: "reminder_day_of_month_check", validate: false
t.check_constraint "deadline_day <= 28", name: "deadline_day_of_month_check"
t.check_constraint "reminder_day <= 28", name: "reminder_day_of_month_check"
end

create_table "partner_profiles", force: :cascade do |t|
Expand Down Expand Up @@ -728,17 +763,6 @@
t.index ["resource_type", "resource_id"], name: "index_roles_on_resource"
end

create_table "solid_cache_entries", force: :cascade do |t|
t.binary "key", null: false
t.binary "value", null: false
t.datetime "created_at", null: false
t.bigint "key_hash", null: false
t.integer "byte_size", null: false
t.index ["byte_size"], name: "index_solid_cache_entries_on_byte_size"
t.index ["key_hash", "byte_size"], name: "index_solid_cache_entries_on_key_hash_and_byte_size"
t.index ["key_hash"], name: "index_solid_cache_entries_on_key_hash", unique: true
end

create_table "storage_locations", id: :serial, force: :cascade do |t|
t.string "name"
t.string "address"
Expand Down Expand Up @@ -855,6 +879,7 @@
t.float "longitude"
t.datetime "created_at", precision: nil, null: false
t.datetime "updated_at", precision: nil, null: false
t.boolean "active", default: true
t.index ["latitude", "longitude"], name: "index_vendors_on_latitude_and_longitude"
end

Expand All @@ -869,7 +894,7 @@
t.index ["item_type", "item_id"], name: "index_versions_on_item_type_and_item_id"
end

add_foreign_key "account_requests", "ndbn_members", primary_key: "ndbn_member_id", validate: false
add_foreign_key "account_requests", "ndbn_members", primary_key: "ndbn_member_id"
add_foreign_key "active_storage_variant_records", "active_storage_blobs", column: "blob_id"
add_foreign_key "adjustments", "organizations"
add_foreign_key "adjustments", "storage_locations"
Expand Down Expand Up @@ -903,7 +928,7 @@
add_foreign_key "partner_requests", "users", column: "partner_user_id"
add_foreign_key "partner_served_areas", "counties"
add_foreign_key "partner_served_areas", "partner_profiles"
add_foreign_key "partners", "storage_locations", column: "default_storage_location_id", validate: false
add_foreign_key "partners", "storage_locations", column: "default_storage_location_id"
add_foreign_key "product_drives", "organizations"
add_foreign_key "requests", "distributions"
add_foreign_key "requests", "organizations"
Expand Down
17 changes: 17 additions & 0 deletions spec/models/vendor_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
# Table name: vendors
#
# id :bigint not null, primary key
# active :boolean default(TRUE)
# address :string
# business_name :string
# comment :string
Expand Down Expand Up @@ -37,6 +38,22 @@
expect(subject.first.volume).to eq(10)
end
end

describe "deactivate!" do
it "deactivates the vendor" do
vendor = create(:vendor)
vendor.deactivate!
expect(vendor.active).to be(false)
end
end

describe "reactivate!" do
it "reactivates the vendor" do
vendor = create(:vendor, active: false)
vendor.reactivate!
expect(vendor.active).to be(true)
end
end
end

describe "versioning" do
Expand Down
6 changes: 6 additions & 0 deletions spec/system/purchase_system_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,12 @@
expect(Purchase.last.line_items.first.quantity).to eq(16)
end

it 'does not show inactive vendors in the vendor dropdown' do
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can this be a request spec? System specs are a lot heavier and flakier.

deactivated_vendor = create(:vendor, business_name: 'Deactivated Vendor', organization: organization, active: false)
visit new_purchase_path
expect(page).to have_no_select('purchase_vendor_id', with_options: [deactivated_vendor.business_name])
end

context 'when creating a purchase incorrectly' do
# Bug fix -- Issue #71
# When a user creates a purchase without it passing validation, the items
Expand Down
Loading