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

#4217 kit factory refactor #4585

Open
wants to merge 23 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
7a351cd
Update bundler version
jimmyli97 Aug 7, 2024
ceec002
REFACTOR clean up unused line item trait introduced in #4163
jimmyli97 Aug 1, 2024
4e1bc8b
REFACTOR replace kit :with_item with KitCreateService, hard code rspec
jimmyli97 Aug 1, 2024
7cabfa6
FIX typo in params in donation and purchase controllers
jimmyli97 Aug 1, 2024
1109170
DOCS add comment clarifying itemizable works with line items
jimmyli97 Jul 26, 2024
b9b8755
REFACTOR rename Item:kits scope to :housing_a_kit, add rspecs
jimmyli97 Aug 1, 2024
8c18e42
REFACTOR DRY up kit base_item, seed_base_items, prevent kit base_item…
jimmyli97 Aug 2, 2024
e1e2c31
Add rspec for item.is_in_kit?
jimmyli97 Aug 2, 2024
7f166cc
Fix lint
jimmyli97 Aug 8, 2024
2c5ed05
rename findorcreatebaseitem to add !
jimmyli97 Aug 9, 2024
141d959
REFACTOR remove unused code childrenservedreportservice
jimmyli97 Aug 15, 2024
080721e
Fix bin/setup so it's working with seed_base_item move
jimmyli97 Aug 15, 2024
e26f441
Merge branch 'main' into 4217-kit-various-refactors
jimmyli97 Aug 15, 2024
a111f74
Revert "REFACTOR clean up unused line item trait introduced in #4163"
jimmyli97 Sep 24, 2024
81c99af
Revert "FIX typo in params in donation and purchase controllers"
jimmyli97 Sep 24, 2024
7b4da7d
Revert "DOCS add comment clarifying itemizable works with line items"
jimmyli97 Sep 24, 2024
04917ba
Revert "REFACTOR rename Item:kits scope to :housing_a_kit, add rspecs"
jimmyli97 Sep 24, 2024
e4de92c
Revert "REFACTOR DRY up kit base_item, seed_base_items, prevent kit b…
jimmyli97 Sep 24, 2024
96b3ceb
Revert "Add rspec for item.is_in_kit?"
jimmyli97 Sep 24, 2024
af1dfd5
Revert "REFACTOR remove unused code childrenservedreportservice"
jimmyli97 Sep 24, 2024
6b50518
Revert "Fix bin/setup so it's working with seed_base_item move"
jimmyli97 Sep 24, 2024
730371a
REFACTOR move kit creation in specs into helper method
jimmyli97 Sep 25, 2024
51f0585
Merge branch 'main' into 4217-kit-various-refactors
jimmyli97 Sep 25, 2024
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 Gemfile.lock
Original file line number Diff line number Diff line change
Expand Up @@ -800,4 +800,4 @@ DEPENDENCIES
webmock (~> 3.23)

BUNDLED WITH
2.5.16
2.5.17
5 changes: 4 additions & 1 deletion app/controllers/admin/base_items_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -38,9 +38,12 @@ def show
@items = @base_item.items
end

# TODO there are no buttons on the view to call this method, consider removing?
Copy link
Collaborator

Choose a reason for hiding this comment

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

We're going to be removing the current approach to base items at some point in the future. I think for now it's safe to just leave this method alone if nothing calls it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added a note for this in PR #4665 a74dcc9

def destroy
@base_item = BaseItem.includes(:items).find(params[:id])
if @base_item.items.any? && @base_item.destroy
if (@base_item.id = KitCreateService.FindOrCreateKitBaseItem!.id)
redirect_to admin_base_items_path, alert: "You cannot delete the Kits base item. This is reserved for all Kits."
elsif @base_item.items.any? && @base_item.destroy
redirect_to admin_base_items_path, notice: "Base Item deleted!"
else
redirect_to admin_base_items_path, alert: "Failed to delete Base Item. Are there still items attached?"
Expand Down
2 changes: 1 addition & 1 deletion app/controllers/donations_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,7 @@ def strip_unnecessary_params

# If line_items have submitted with empty rows, clear those out first.
def compact_line_items
return params unless params[:donation].key?(:line_item_attributes)
return params unless params[:donation].key?(:line_items_attributes)

params[:donation][:line_items_attributes].delete_if { |_row, data| data["quantity"].blank? && data["item_id"].blank? }
params
Expand Down
2 changes: 1 addition & 1 deletion app/controllers/purchases_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ def filter_params

# If line_items have submitted with empty rows, clear those out first.
def compact_line_items
return params unless params[:purchase].key?(:line_item_attributes)
return params unless params[:purchase].key?(:line_items_attributes)

params[:purchase][:line_items_attributes].delete_if { |_row, data| data["quantity"].blank? && data["item_id"].blank? }
params
Expand Down
21 changes: 20 additions & 1 deletion app/models/base_item.rb
Original file line number Diff line number Diff line change
Expand Up @@ -28,5 +28,24 @@ class BaseItem < ApplicationRecord
def to_h
{ partner_key: partner_key, name: name }
end
end

def self.seed_base_items
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 only used in specs and seeds. Not sure this belongs on the main BaseItem class. I'd probably create a module call Seeds or something like that that can be referenced from those two contexts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved into lib/seed_base_items.rb in PR #4665 8fde086

# Initial starting qty for our test organizations
base_items = File.read(Rails.root.join("db", "base_items.json"))
items_by_category = JSON.parse(base_items)

items_by_category.each do |category, entries|
entries.each do |entry|
BaseItem.find_or_create_by!(
name: entry["name"],
category: category,
partner_key: entry["key"],
updated_at: Time.zone.now,
created_at: Time.zone.now
)
end
end
# Create global 'Kit' base item
KitCreateService.FindOrCreateKitBaseItem!
end
end
2 changes: 1 addition & 1 deletion app/models/concerns/itemizable.rb
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
# Creates a veritable powerhouse.
# This module provides Duck Typed behaviors for anything that shuttle Items
# This module provides Duck Typed behaviors for anything that shuttles LINE ITEMS (not items)
# throughout the system. e.g. things that `has_many :line_items` -- this provides
# all the logic about how those kinds of things behave.
module Itemizable
Expand Down
4 changes: 2 additions & 2 deletions app/models/item.rb
Original file line number Diff line number Diff line change
Expand Up @@ -51,8 +51,8 @@ class Item < ApplicationRecord

scope :active, -> { where(active: true) }

# Add spec for these
scope :kits, -> { where.not(kit_id: nil) }
# :housing_a_kit are items which house a kit, NOT items is_in_kit
scope :housing_a_kit, -> { where.not(kit_id: nil) }
scope :loose, -> { where(kit_id: nil) }

scope :visible, -> { where(visible_to_partners: true) }
Expand Down
2 changes: 1 addition & 1 deletion app/services/itemizable_update_service.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ module ItemizableUpdateService
# @param itemizable [Itemizable]
# @param type [Symbol] :increase or :decrease - if the original line items added quantities (purchases or
# donations), use :increase. If the original line_items reduced quantities (distributions) use :decrease.
# @param params [Hash] Parameters passed from the controller. Should include `line_item_attributes`.
# @param params [Hash] Parameters passed from the controller. Should include `line_items_attributes`.
# @param event_class [Class<Event>] the event class to publish the itemizable to.
def self.call(itemizable:, type: :increase, params: {}, event_class: nil)
StorageLocation.transaction do
Expand Down
43 changes: 14 additions & 29 deletions app/services/kit_create_service.rb
Original file line number Diff line number Diff line change
@@ -1,8 +1,18 @@
class KitCreateService
include ServiceObjectErrorsMixin

KIT_BASE_ITEM_ATTRS = {
name: 'Kit',
category: 'kit',
partner_key: 'kit'
}

attr_reader :kit

def self.FindOrCreateKitBaseItem!
Copy link
Collaborator

Choose a reason for hiding this comment

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

Idiomatic Ruby uses snake_case: find_or_create_kit_base_item!.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed this in PR #4665 a74dcc9

BaseItem.find_or_create_by!(KIT_BASE_ITEM_ATTRS)
end

def initialize(organization_id:, kit_params:)
@organization_id = organization_id
@kit_params = kit_params
Expand All @@ -16,16 +26,15 @@ def call
@kit = Kit.new(kit_params_with_organization)
@kit.save!

# Create a BaseItem that houses each
# kit item created.
kit_base_item = fetch_or_create_kit_base_item
# Find or create the BaseItem for all items housing kits
item_housing_a_kit_base_item = KitCreateService.FindOrCreateKitBaseItem!

# Create the Item.
# Create the item
item_creation = ItemCreateService.new(
organization_id: organization.id,
item_params: {
name: kit.name,
partner_key: kit_base_item.partner_key,
partner_key: item_housing_a_kit_base_item.partner_key,
kit_id: kit.id
}
)
Expand All @@ -45,7 +54,6 @@ def call
private

attr_reader :organization_id, :kit_params

def organization
@organization ||= Organization.find_by(id: organization_id)
end
Expand All @@ -56,18 +64,6 @@ def kit_params_with_organization
})
end

def fetch_or_create_kit_base_item
BaseItem.find_or_create_by!({
name: 'Kit',
category: 'kit',
partner_key: 'kit'
})
end

def partner_key_for_kits
'kit'
end

def valid?
if organization.blank?
errors.add(:organization_id, 'does not match any Organization')
Expand All @@ -89,15 +85,4 @@ def kit_validation_errors

@kit_validation_errors = kit.errors
end

def associated_item_params
{
kit: kit.name
}
end

def partner_key_for(name)
"kit_#{name.underscore.gsub(/\s+/, '_')}"
end
end

39 changes: 0 additions & 39 deletions app/services/reports/children_served_report_service.rb
Original file line number Diff line number Diff line change
Expand Up @@ -31,47 +31,8 @@ def average_children_monthly
total_children_served / 12.0
end

def disposable_diapers_from_kits_total
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why were these lines removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

continued conversation in #4665

organization_id = @organization.id
year = @year

sql_query = <<-SQL
SELECT SUM(line_items.quantity * kit_line_items.quantity)
FROM distributions
INNER JOIN line_items ON line_items.itemizable_type = 'Distribution' AND line_items.itemizable_id = distributions.id
INNER JOIN items ON items.id = line_items.item_id
INNER JOIN kits ON kits.id = items.kit_id
INNER JOIN line_items AS kit_line_items ON kits.id = kit_line_items.itemizable_id
INNER JOIN items AS kit_items ON kit_items.id = kit_line_items.item_id
INNER JOIN base_items ON base_items.partner_key = kit_items.partner_key
WHERE distributions.organization_id = ?
AND EXTRACT(year FROM issued_at) = ?
AND LOWER(base_items.category) LIKE '%diaper%'
AND NOT (LOWER(base_items.category) LIKE '%cloth%' OR LOWER(base_items.name) LIKE '%cloth%')
AND NOT (LOWER(base_items.category) LIKE '%adult%')
SQL

sanitized_sql = ActiveRecord::Base.send(:sanitize_sql_array, [sql_query, organization_id, year])

result = ActiveRecord::Base.connection.execute(sanitized_sql)
result.first['sum'].to_i
end

private

def total_disposable_diapers_distributed
loose_disposable_distribution_total + disposable_diapers_from_kits_total
end

def loose_disposable_distribution_total
organization
.distributions
.for_year(year)
.joins(line_items: :item)
.merge(Item.disposable)
.sum("line_items.quantity")
end

def total_children_served_with_loose_disposables
organization
.distributions
Expand Down
19 changes: 2 additions & 17 deletions db/seeds.rb
Original file line number Diff line number Diff line change
Expand Up @@ -19,26 +19,11 @@ def random_record_for_org(org, klass)
# Script-Global Variables
# ----------------------------------------------------------------------------

# Initial starting qty for our test organizations
base_items = File.read(Rails.root.join("db", "base_items.json"))
items_by_category = JSON.parse(base_items)

# ----------------------------------------------------------------------------
# Base Items
# ----------------------------------------------------------------------------

items_by_category.each do |category, entries|
entries.each do |entry|
BaseItem.find_or_create_by!(name: entry["name"], category: category, partner_key: entry["key"])
end
end

# Create global 'Kit' base item
BaseItem.find_or_create_by!(
name: 'Kit',
category: 'kit',
partner_key: 'kit'
)
BaseItem.seed_base_items

# ----------------------------------------------------------------------------
# NDBN Members
Expand Down Expand Up @@ -515,7 +500,7 @@ def seed_quantity(item_name, organization, storage_location, quantity)
AdjustmentCreateService.new(adjustment).call
end

items_by_category.each do |_category, entries|
JSON.parse(File.read(Rails.root.join("db", "base_items.json"))).each do |_category, entries|
entries.each do |entry|
seed_quantity(entry['name'], pdx_org, inv_arbor, entry['qty']['arbor'])
seed_quantity(entry['name'], pdx_org, inv_pdxdb, entry['qty']['pdxdb'])
Expand Down
23 changes: 15 additions & 8 deletions spec/events/inventory_aggregate_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -381,11 +381,14 @@
end

it "should process a kit allocation event" do
kit = FactoryBot.create(:kit, :with_item, organization: organization)
params = FactoryBot.attributes_for(:kit)
params[:line_items_attributes] = [
{item_id: item1.id, quantity: 10},
{item_id: item2.id, quantity: 3}
]

kit = KitCreateService.new(organization_id: organization.id, kit_params: params).call.kit
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a way for us to just update the factory itself to call KitCreateService? E.g. by overriding to_create? https://github.com/thoughtbot/factory_bot/blob/main/GETTING_STARTED.md#custom-methods-to-persist-objects

If not, can we create a reusable method to do this work, allowing you just to pass in the LineItems you want to create, and provide a sane default if we don't care what they are?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I considered overriding to_create but I don't think it'll work because the way it works right now KitCreateService creates its own instance of kit and I don't think it's worth the time to rejig it so that it can accept FactoryBot's instance, since it will be changed in the kit refactoring.

I created a helper method under spec/support/kit_helper.rb in 730371a


kit.line_items = []
kit.line_items << build(:line_item, quantity: 10, item: item1, itemizable: kit)
kit.line_items << build(:line_item, quantity: 3, item: item2, itemizable: kit)
KitAllocateEvent.publish(kit, storage_location1.id, 2)

# 30 - (10*2) = 10, 10 - (3*2) = 4
Expand Down Expand Up @@ -416,7 +419,14 @@
end

it "should process a kit deallocation event" do
kit = FactoryBot.create(:kit, :with_item, organization: organization)
params = FactoryBot.attributes_for(:kit)
params[:line_items_attributes] = [
{item_id: item1.id, quantity: 20},
{item_id: item2.id, quantity: 5}
]

kit = KitCreateService.new(organization_id: organization.id, kit_params: params).call.kit

TestInventory.create_inventory(organization,
{
storage_location1.id => {
Expand All @@ -432,9 +442,6 @@
})
inventory = InventoryAggregate.inventory_for(organization.id) # reload

kit.line_items = []
kit.line_items << build(:line_item, quantity: 20, item: item1, itemizable: kit)
kit.line_items << build(:line_item, quantity: 5, item: item2, itemizable: kit)
KitDeallocateEvent.publish(kit, storage_location1, 2)

# 30 + (20*2) = 70, 10 + (5*2) = 20
Expand Down
14 changes: 7 additions & 7 deletions spec/factories/kits.rb
Original file line number Diff line number Diff line change
Expand Up @@ -13,19 +13,19 @@
#
FactoryBot.define do
factory :kit do
sequence(:name) { |n| "Test Kit #{n}" }
sequence(:name) { |n| "Default Kit Name #{n} - Don't Match" }
organization

after(:build) do |instance, _|
if instance.line_items.blank?
instance.line_items << create(:line_item, item: create(:item, organization: instance.organization), itemizable: instance)
instance.line_items << build(:line_item, item: create(:item, organization: instance.organization), itemizable: nil)
end
end

trait :with_item do
after(:create) do |instance, _|
create(:item, kit: instance, organization: instance.organization)
end
end
# See #3652, changes to this factory are in progress
# For now, to create corresponding item and line item and persist to database call KitCreateService with:
# kit_params = attributes_for(:kit)
# kit_params[:line_items_attributes] = [{ item_id: _id_, quantity: _quantity_, ...}]
# KitCreateService.new(organization_id: organization.id, kit_params: kit_params).call.kit
end
end
5 changes: 0 additions & 5 deletions spec/factories/line_items.rb
Original file line number Diff line number Diff line change
Expand Up @@ -51,10 +51,5 @@
itemizable_type { "Transfer" }
itemizable_id { create(:transfer).id }
end

trait :kit do
itemizable_type { "Kit" }
itemizable_id { create(:kit).id }
end
end
end
2 changes: 1 addition & 1 deletion spec/factories/organizations.rb
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@

trait :with_items do
after(:create) do |instance, evaluator|
seed_base_items if BaseItem.count.zero? # seeds 45 base items if none exist
BaseItem.seed_base_items if BaseItem.count.zero? # seeds 45 base items if none exist
Organization.seed_items(instance) # creates 1 item for each base item
end
end
Expand Down
Loading