diff --git a/app/controllers/items_controller.rb b/app/controllers/items_controller.rb index c87e608e92..2dec08ba32 100644 --- a/app/controllers/items_controller.rb +++ b/app/controllers/items_controller.rb @@ -73,18 +73,18 @@ def show def update @item = current_organization.items.find(params[:id]) - @item.attributes = item_params deactivated = @item.active_changed? && !@item.active if deactivated && !@item.can_deactivate? flash.now[:error] = "Can't deactivate this item - it is currently assigned to either an active kit or a storage location!" render action: :edit return end + result = ItemUpdateService.new(item: @item, params: item_params, request_unit_ids: request_unit_ids).call - if update_item + if result.success? redirect_to items_path, notice: "#{@item.name} updated!" else - flash.now[:error] = "Something didn't work quite right -- try again? #{@item.errors.map { |error| "#{error.attribute}: #{error.message}" }}" + flash.now[:error] = result.error.record.errors.full_messages.to_sentence render action: :edit end end @@ -185,27 +185,6 @@ def request_unit_ids params.require(:item).permit(request_unit_ids: []).fetch(:request_unit_ids, []) end - # We need to update both the item and the request_units together and fail together - def update_item - if Flipper.enabled?(:enable_packs) - update_item_and_request_units - else - @item.save - end - end - - def update_item_and_request_units - begin - Item.transaction do - @item.save! - @item.sync_request_units!(request_unit_ids) - end - rescue - return false - end - true - end - helper_method \ def filter_params(_parameters = nil) return {} unless params.key?(:filters) diff --git a/app/models/kit.rb b/app/models/kit.rb index 363407266f..2d4f7245fd 100644 --- a/app/models/kit.rb +++ b/app/models/kit.rb @@ -55,6 +55,13 @@ def reactivate item.update!(active: true) end + def update_value_in_cents + kit_value_in_cents = line_items.includes(:item).reduce(0) do |sum, line_item| + sum + line_item.item.value_in_cents.to_i * line_item.quantity.to_i + end + update!(value_in_cents: kit_value_in_cents) + end + private def at_least_one_item diff --git a/app/services/item_update_service.rb b/app/services/item_update_service.rb new file mode 100644 index 0000000000..ed368782c3 --- /dev/null +++ b/app/services/item_update_service.rb @@ -0,0 +1,31 @@ +# frozen_string_literal: true + +class ItemUpdateService + attr_reader :item, :params, :request_unit_ids + def initialize(item:, params:, request_unit_ids: []) + @item = item + @request_unit_ids = request_unit_ids + @params = params + end + + def call + ActiveRecord::Base.transaction do + item.update!(params) + update_kit_value + if Flipper.enabled?(:enable_packs) + item.sync_request_units!(request_unit_ids) + end + end + Result.new(value: item) + rescue => e + Result.new(error: e) + end + + private + + def update_kit_value + return unless item.kit + + item.kit.update_value_in_cents + end +end diff --git a/app/services/kit_create_service.rb b/app/services/kit_create_service.rb index 7884d5d61c..0ca2894f85 100644 --- a/app/services/kit_create_service.rb +++ b/app/services/kit_create_service.rb @@ -43,6 +43,8 @@ def call unless item_creation_result.success? raise item_creation_result.error end + kit.item.update(visible_to_partners: kit.visible_to_partners) + kit.update_value_in_cents rescue StandardError => e errors.add(:base, e.message) raise ActiveRecord::Rollback diff --git a/app/views/items/_form.html.erb b/app/views/items/_form.html.erb index 874d9c0933..8b44b3e71d 100644 --- a/app/views/items/_form.html.erb +++ b/app/views/items/_form.html.erb @@ -22,25 +22,25 @@ <%= f.input :reporting_category, label: 'NDBN Reporting Category', collection: Item.reporting_categories_for_select, disabled: !!@item.kit_id, required: !@item.kit_id, hint: @reporting_category_hint %> - <%= f.input :name, label: "Value per item", wrapper: :input_group do %> + <%= f.input :value_in_dollars, label: "Value per item", wrapper: :input_group do %> <%= f.input_field :value_in_dollars, class: "form-control" %> <% end %> - <%= f.input :name, label: "Quantity Per Individual", wrapper: :input_group do %> + <%= f.input :distribution_quantity, label: "Quantity Per Individual", wrapper: :input_group do %> <%= f.input_field :distribution_quantity, class: "form-control" %> <% end %> <% if current_user.has_cached_role?(Role::ORG_ADMIN, current_organization) %> - <%= f.input :name, label: "On hand minimum quantity", wrapper: :input_group do %> + <%= f.input :on_hand_minimum_quantity, label: "On hand minimum quantity", wrapper: :input_group do %> <%= f.input_field :on_hand_minimum_quantity, input_html: {value: 0}, class: "form-control" %> <% end %> - <%= f.input :name, label: "On hand recommended quantity", wrapper: :input_group do %> + <%= f.input :on_hand_recommended_quantity, label: "On hand recommended quantity", wrapper: :input_group do %> <%= f.input_field :on_hand_recommended_quantity, class: "form-control" %> <% end %> <% end %> - <%= f.input :name, label: "Package size", wrapper: :input_group do %> + <%= f.input :package_size, label: "Package size", wrapper: :input_group do %> <%= f.input_field :package_size, class: "form-control", min: 0 %> <% end %> @@ -50,7 +50,7 @@ <% end %> <% end %> - <%= f.input :visible, label: "Item is Visible to Partners?", wrapper: :input_group do %> + <%= f.input :visible_to_partners, label: "Item is Visible to Partners?", wrapper: :input_group do %> <%= f.check_box :visible_to_partners, {class: "input-group-text", id: "visible_to_partners"}, "true", "false" %> <% end %> diff --git a/app/views/kits/_form.html.erb b/app/views/kits/_form.html.erb index 3c1acb8320..abe2761c79 100644 --- a/app/views/kits/_form.html.erb +++ b/app/views/kits/_form.html.erb @@ -14,11 +14,6 @@ <%= f.check_box :visible_to_partners, {class: "input-group-text", id: "visible_to_partners"}, "true", "false" %> <% end %> - <%= f.input :value_in_cents, label: "Value for kit", wrapper: :input_group do %> - - <%= f.input_field :value_in_dollars, class: "form-control", min: 0 %> - <% end %> -
Items in this Kit
@@ -38,5 +33,6 @@
+ <% end %> diff --git a/app/views/kits/_table.html.erb b/app/views/kits/_table.html.erb index 5f633ea8f1..167c469892 100644 --- a/app/views/kits/_table.html.erb +++ b/app/views/kits/_table.html.erb @@ -3,6 +3,7 @@ Name Items + Value for kit Allocations Actions @@ -18,6 +19,12 @@ <% end %> + + + <%= number_to_currency(kit.value_in_dollars) %> + + + diff --git a/spec/services/item_update_service_spec.rb b/spec/services/item_update_service_spec.rb new file mode 100644 index 0000000000..184d1a87db --- /dev/null +++ b/spec/services/item_update_service_spec.rb @@ -0,0 +1,77 @@ +# frozen_string_literal: true + +require "rspec" + +RSpec.describe ItemUpdateService, type: :service do + describe ".call" do + subject { described_class.new(item: item, params: params, request_unit_ids: request_unit_ids).call } + + let(:kit) { create(:kit) } + let(:item) { create(:item, kit: kit) } + let(:line_item) { create(:line_item, item: item, quantity: 1) } + let(:params) do + { + name: "Updated Item Name", + reporting_category: "pads", + value_in_cents: 2000 + } + end + let(:request_unit_ids) { [] } + + before do + kit.line_items = [line_item] + end + + context "params are ok" do + it "returns a Result with success? true and the item" do + result = subject + expect(result).to be_a_kind_of(Result) + expect(result.success?).to eq(true) + expect(result.value).to eq(item) + end + + it "updates the item attributes" do + subject + item.reload + expect(item.name).to eq("Updated Item Name") + expect(item.value_in_cents).to eq(2000) + end + + it "updates the kit value_in_cents" do + subject + kit.reload + expect(kit.value_in_cents).to eq(params[:value_in_cents]) + end + end + + context "params are invalid" do + let(:params) do + { + name: "" # Invalid as name can't be blank + } + end + + it "returns a Result with success? false and an error" do + result = subject + expect(result).to be_a_kind_of(Result) + expect(result.success?).to eq(false) + expect(result.error).to be_a(ActiveRecord::RecordInvalid) + expect(result.error.message).to include("Validation failed: Name can't be blank") + end + + it "does not update the item attributes" do + original_name = item.name + subject + item.reload + expect(item.name).to eq(original_name) + end + + it "does not update the kit value_in_cents" do + original_kit_value = kit.value_in_cents + subject + kit.reload + expect(kit.value_in_cents).to eq(original_kit_value) + end + end + end +end diff --git a/spec/services/kit_create_service_spec.rb b/spec/services/kit_create_service_spec.rb index 7a81c93683..d48f02a6b0 100644 --- a/spec/services/kit_create_service_spec.rb +++ b/spec/services/kit_create_service_spec.rb @@ -16,14 +16,17 @@ end let!(:line_items_attr) do - items = create_list(:item, 3, organization: organization) + items = create_list(:item, 3, value_in_cents: 2000, organization: organization) items.map do |item| { item_id: item.id, - quantity: Faker::Number.number(digits: 2) + quantity: 1 } end end + let(:kit_value_in_cents) do + 6000 + end it 'should return an the instance' do expect(subject).to be_a_kind_of(described_class) @@ -31,15 +34,12 @@ context 'when the parameters are valid' do it 'should create a new Kit' do - expect { subject }.to change { Kit.all.count }.by(1) + expect { subject }.to change { Kit.count }.by(1) + expect(Kit.last.value_in_cents).to eq(kit_value_in_cents) end it 'should create a new Item' do - expect { subject }.to change { Item.all.count }.by(1) - end - - it 'should create the new Item associated with the Kit' do - expect { subject }.to change { Kit.all.count }.by(1) + expect { subject }.to change { Item.count }.by(1) end context 'but an unexpected error gets raised' do @@ -92,6 +92,14 @@ end end + context 'line_items_attributes is empty' do + let(:line_items_attr) { [] } + + it 'should have an error At least one item is required' do + expect(subject.errors.full_messages).to include("At least one item is required") + end + end + context 'because the kit_params is invalid for kit creation' do let(:kit_params) { { organization_id: organization_id } } let(:kit_validation_errors) do diff --git a/spec/system/item_system_spec.rb b/spec/system/item_system_spec.rb index 55f94cec2c..cde74b43d3 100644 --- a/spec/system/item_system_spec.rb +++ b/spec/system/item_system_spec.rb @@ -53,7 +53,7 @@ fill_in "Name", with: "" click_button "Save" - expect(page.find(".alert")).to have_content "didn't work" + expect(page.find(".alert")).to have_content "Name can't be blank" end it "can make the item invisible to partners" do diff --git a/spec/system/kit_system_spec.rb b/spec/system/kit_system_spec.rb index c79a6d5a38..606123f967 100644 --- a/spec/system/kit_system_spec.rb +++ b/spec/system/kit_system_spec.rb @@ -33,23 +33,57 @@ }) end - it "can create a new kit as a user with the proper quantity" do - visit new_kit_path - kit_traits = attributes_for(:kit) - - fill_in "Name", with: kit_traits[:name] - find(:css, '#kit_value_in_dollars').set('10.10') + context "kit creation" do + let(:kit_traits) { attributes_for(:kit) } + let(:value_in_dollars) { 10.10 } + let(:value_in_cents) { value_in_dollars * 100 } + let!(:items) { create_list(:item, 2, value_in_cents: value_in_cents, organization: organization) } + let(:quantity_per_kit) { 5 } + let(:kit_value_in_dollars) { (items.count * quantity_per_kit * value_in_dollars).round(2) } - item = Item.last - quantity_per_kit = 5 - select item.name, from: "kit_line_items_attributes_0_item_id" - find(:css, '#kit_line_items_attributes_0_quantity').set(quantity_per_kit) + before do + visit new_kit_path + fill_in "Name", with: kit_traits[:name] + find(:css, '#visible_to_partners').set(false) + all(:css, '.line_item_name').first.select(items[0].name) + all(:css, '.quantity').first.set(quantity_per_kit) + click_link "Add Another Item" + all(:css, '.line_item_name').last.select(items[1].name) + all(:css, '.quantity').last.set(quantity_per_kit) + end - click_button "Save" + subject { click_button "Save" } + + it "can create a new kit as a user with the proper quantity" do + expect { + subject + expect(page.find(".alert")).to have_content "Kit created successfully" + expect(page).to have_content(kit_traits[:name]) + expect(page).to have_content("#{quantity_per_kit} #{items[0].name}") + expect(page).to have_content("#{quantity_per_kit} #{items[1].name}") + expect(Kit.last.name).to eq(kit_traits[:name]) + expect(Kit.last.visible_to_partners).to eq(false) + expect(Kit.last.value_in_dollars).to eq(kit_value_in_dollars) + expect(Kit.last.item.reload.visible_to_partners).to eq(false) + expect(items[0].reload.value_in_dollars).to eq(value_in_dollars) + expect(items[1].reload.value_in_dollars).to eq(value_in_dollars) + }.to change(Kit, :count).by(1) + end - expect(page.find(".alert")).to have_content "Kit created successfully" - expect(page).to have_content(kit_traits[:name]) - expect(page).to have_content("#{quantity_per_kit} #{item.name}") + context "items not selected" do + before do + visit new_kit_path + fill_in "Name", with: kit_traits[:name] + find(:css, '#visible_to_partners').set(false) + end + + it "displays error indicating at least one item is required" do + expect { + subject + expect(page.find(".alert")).to have_content "At least one item is required" + }.not_to change(Kit, :count) + end + end end it "can add items correctly" do @@ -212,8 +246,6 @@ visit new_kit_path kit_traits = attributes_for(:kit) - find(:css, '#kit_value_in_dollars').set('10.10') - item = Item.last quantity_per_kit = 5 select item.name, from: "kit_line_items_attributes_0_item_id"