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/services/item_update_service.rb b/app/services/item_update_service.rb new file mode 100644 index 0000000000..a99047073d --- /dev/null +++ b/app/services/item_update_service.rb @@ -0,0 +1,34 @@ +# 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 + + kit_value_in_cents = item.kit.items.reduce(0) do |sum, i| + sum + i.value_in_cents.to_i * item.kit.line_items.find_by(item_id: i.id).quantity.to_i + end + item.kit.update!(value_in_cents: kit_value_in_cents) + end +end diff --git a/app/services/kit_create_service.rb b/app/services/kit_create_service.rb index 7884d5d61c..397b529714 100644 --- a/app/services/kit_create_service.rb +++ b/app/services/kit_create_service.rb @@ -43,6 +43,11 @@ def call unless item_creation_result.success? raise item_creation_result.error end + kit.items.update_all(visible_to_partners: kit.visible_to_partners) + kit_value_in_cents = kit.items.reduce(0) do |sum, i| + sum + i.value_in_cents.to_i * kit.line_items.find_by(item_id: i.id).quantity.to_i + end + kit.update!(value_in_cents: kit_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..c003a864f0 --- /dev/null +++ b/spec/services/item_update_service_spec.rb @@ -0,0 +1,79 @@ +# 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(:item2) { create(:item, kit: kit) } + let(:params) do + { + name: "Updated Item Name", + reporting_category: "pads", + value_in_cents: 2000 + } + end + let(:request_unit_ids) { [] } + let(:kit_value_in_cents) do + kit.line_items.reduce(0) do |sum, li| + item = Item.find(li.item_id) + sum + item.value_in_cents.to_i * li.quantity.to_i + end + 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(kit_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..6fba9cb393 100644 --- a/spec/services/kit_create_service_spec.rb +++ b/spec/services/kit_create_service_spec.rb @@ -24,6 +24,12 @@ } end end + let(:kit_value_in_cents) do + line_items_attr.sum do |li| + item = Item.find(li[:item_id]) + item.value_in_cents.to_i * li[:quantity].to_i + end + end it 'should return an the instance' do expect(subject).to be_a_kind_of(described_class) @@ -31,15 +37,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 +95,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..f5717cb8f6 100644 --- a/spec/system/item_system_spec.rb +++ b/spec/system/item_system_spec.rb @@ -1,6 +1,6 @@ RSpec.describe "Item management", type: :system do let(:organization) { create(:organization) } - let(:user) { create(:user, organization: organization) } + let(:user) { create(:organization_admin, organization: organization) } before do sign_in(user) @@ -39,12 +39,86 @@ expect(Item.last.value_in_cents).to eq(123_456) end - it "can update an existing item as a user" do - item = create(:item) - visit edit_item_path(item.id) - click_button "Save" + context "update item" do + let!(:item) { create(:item, organization: organization, name: "Old Name") } + let(:params) do + { + name: "New Name", + item_category_id: nil, + reporting_category: "Pads", + partner_key: "other", + value_in_cents: 1234, + package_size: 20, + on_hand_minimum_quantity: 5, + on_hand_recommended_quantity: 10, + distribution_quantity: 2, + visible_to_partners: true, + active: true, + additional_info: "Some additional info" + } + end + + before do + visit edit_item_path(item.id) + fill_in "Name", with: params[:name] + select params[:reporting_category], from: "Reporting Category" + fill_in "Value per item", with: params[:value_in_cents] / 100.00 + fill_in "Package size", with: params[:package_size] + fill_in "On hand minimum quantity", with: params[:on_hand_minimum_quantity] + fill_in "On hand recommended quantity", with: params[:on_hand_recommended_quantity] + fill_in "Quantity Per Individual", with: params[:distribution_quantity] + fill_in "Additional Info", with: params[:additional_info] + end + + subject { click_button "Save" } + + it "updates the item with valid inputs" do + subject + item.reload + expect(page.find(".alert")).to have_content "#{item.name} updated!" + expect(item.name).to eq(params[:name]) + expect(item.item_category_id).to eq(params[:item_category_id]) + expect(item.reporting_category).to eq(params[:reporting_category].underscore) + expect(item.value_in_cents).to eq(params[:value_in_cents]) + expect(item.package_size).to eq(params[:package_size]) + expect(item.on_hand_minimum_quantity).to eq(params[:on_hand_minimum_quantity]) + expect(item.on_hand_recommended_quantity).to eq(params[:on_hand_recommended_quantity]) + expect(item.distribution_quantity).to eq(params[:distribution_quantity]) + expect(item.visible_to_partners).to eq(params[:visible_to_partners]) + expect(item.active).to eq(params[:active]) + expect(item.additional_info).to eq(params[:additional_info]) + end - expect(page.find(".alert")).to have_content "updated" + context "item belongs to a kit" do + let!(:kit) { create(:kit, organization: organization) } + let!(:item2) { create(:item, organization: organization) } + let(:kit_value_in_cents) { item.value_in_cents.to_i + item2.value_in_cents.to_i } + + before do + item.update!(kit: kit) + item2.update!(kit: kit) + visit edit_item_path(item.id) + fill_in "Name", with: params[:name] + end + + it "does not allow changing reporting category" do + expect(page).to have_field("Reporting Category", disabled: true) + expect(page).to have_content("Kits are reported based on their contents.") + subject + expect(kit.value_in_cents).to eq(kit_value_in_cents) + end + end + + context "with invalid inputs" do + let(:params) do + super().merge(name: "", reporting_category: "") + end + + it "shows the error messages" do + subject + expect(page.find(".alert")).to have_content "Name can't be blank and Reporting category can't be blank" + end + end end it "can update an existing item with empty attributes as a user" do @@ -53,7 +127,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 @@ -111,7 +185,7 @@ end # Consolidated these into one to reduce the setup/teardown - it "should display items in separate tabs", js: true do + it "should display items in separate tabs", js: true, driver: :selenium_chrome do tab_items_only_text = page.find("#items-table", visible: true).text expect(tab_items_only_text).to have_content item_pullups.name expect(tab_items_only_text).to have_content item_tampons.name diff --git a/spec/system/kit_system_spec.rb b/spec/system/kit_system_spec.rb index c79a6d5a38..c13e987811 100644 --- a/spec/system/kit_system_spec.rb +++ b/spec/system/kit_system_spec.rb @@ -33,23 +33,58 @@ }) 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(items[0].reload.visible_to_partners).to eq(false) + expect(items[1].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 +247,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"