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

Fix Distribution#new,#create,#edit to only show active items #4848

Merged
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
8 changes: 4 additions & 4 deletions app/controllers/distributions_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ def create
elsif request_id
@distribution.initialize_request_items
end
@items = current_organization.items.alphabetized
@items = current_organization.items.active.alphabetized
@partner_list = current_organization.partners.where.not(status: 'deactivated').alphabetized

inventory = View::Inventory.new(@distribution.organization_id)
Expand Down Expand Up @@ -147,7 +147,7 @@ def new
@distribution.line_items.build
@distribution.copy_from_donation(params[:donation_id], params[:storage_location_id])
end
@items = current_organization.items.alphabetized
@items = current_organization.items.active.alphabetized
@partner_list = current_organization.partners.where.not(status: 'deactivated').alphabetized

inventory = View::Inventory.new(current_organization.id)
Expand All @@ -173,7 +173,7 @@ def edit
if ([email protected]? && @distribution.future?) ||
current_user.has_role?(Role::ORG_ADMIN, current_organization)
@distribution.line_items.build if @distribution.line_items.size.zero?
@items = current_organization.items.alphabetized
@items = current_organization.items.active.alphabetized
@partner_list = current_organization.partners.alphabetized
@audit_warning = current_organization.audits
.where(storage_location_id: @distribution.storage_location_id)
Expand Down Expand Up @@ -204,7 +204,7 @@ def update
flash[:error] = insufficient_error_message(result.error.message)
@distribution.line_items.build if @distribution.line_items.size.zero?
@distribution.initialize_request_items
@items = current_organization.items.alphabetized
@items = current_organization.items.active.alphabetized
@storage_locations = current_organization.storage_locations.active_locations.alphabetized
render :edit
end
Expand Down
1 change: 0 additions & 1 deletion app/models/distribution.rb
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,6 @@ class Distribution < ApplicationRecord

enum state: { scheduled: 5, complete: 10 }
enum delivery_method: { pick_up: 0, delivery: 1, shipped: 2 }
scope :active, -> { joins(:line_items).joins(:items).where(items: { active: true }) }
Copy link
Collaborator Author

@coalest coalest Dec 12, 2024

Choose a reason for hiding this comment

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

I can remove this change if preferred as it's unrelated to this PR. But it looks like a piece of dead code—I can't find it being used anywhere.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's a weird scope anyway... it would only exclude distributions where all the items are inactive, which I can't imagine being a common case.

# add item_id scope to allow filtering distributions by item
scope :by_item_id, ->(item_id) { includes(:items).where(items: { id: item_id }) }
# partner scope to allow filtering by partner
Expand Down
39 changes: 39 additions & 0 deletions spec/requests/distributions_requests_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -233,6 +233,20 @@
expect(response).to have_error
end

it "renders #new on failure with only active items in dropdown" do
create(:item, organization: organization, name: 'Active Item')
create(:item, :inactive, organization: organization, name: 'Inactive Item')

post distributions_path(distribution: { comment: nil, partner_id: nil, storage_location_id: nil }, format: :turbo_stream)
expect(response).to have_http_status(400)

page = Nokogiri::HTML(response.body)
selectable_items = page.at_css("select.line_item_name").text.split("\n")

expect(selectable_items).to include("Active Item")
expect(selectable_items).not_to include("Inactive Item")
end

context "Deactivated partners should not be displayed in partner dropdown" do
before do
create(:partner, name: 'Active Partner', organization: organization, status: "approved")
Expand Down Expand Up @@ -275,6 +289,18 @@
expect(page.css('#distribution_storage_location_id option[selected]')).to be_empty
end

it "should only show active items in item dropdown" do
create(:item, :inactive, organization: organization, name: 'Inactive Item')

get new_distribution_path(default_params)

page = Nokogiri::HTML(response.body)
selectable_items = page.at_css("select#barcode_item_barcodeable_id").text.split("\n")

expect(selectable_items).to include("Item 1", "Item 2")
expect(selectable_items).not_to include("Inactive Item")
end

context "with org default but no partner default" do
it "selects org default" do
organization.update!(default_storage_location: storage_location.id)
Expand Down Expand Up @@ -572,6 +598,19 @@
expect(response.body).to include("Active Partner")
end

it "should only show active items in item dropdown" do
create(:item, organization: organization, name: 'Active Item')
create(:item, :inactive, organization: organization, name: 'Inactive Item')

get edit_distribution_path(id: distribution.id)

page = Nokogiri::HTML(response.body)
selectable_items = page.at_css("select#barcode_item_barcodeable_id").text.split("\n")

expect(selectable_items).to include("Active Item")
expect(selectable_items).not_to include("Inactive Item")
end

context 'with units' do
let!(:request) {
create(:request,
Expand Down
Loading