fix(kit_creation): Add ItemUpdateService

Author: Olexii Kasianenko

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)

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 StandardError => 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

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 %>
               </div>
 
-              <%= f.input :name, label: "Value per item", wrapper: :input_group do %>
+              <%= f.input :value_in_dollars, label: "Value per item", wrapper: :input_group do %>
                 <span class="input-group-text"><i class="fa fa-money"></i></span>
                 <%= 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 %>
 

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

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