From f8b85d3c27edaaa01ec5842105e7fe0576a4438c Mon Sep 17 00:00:00 2001 From: Jimmy Li Date: Thu, 1 Aug 2024 13:25:02 +0000 Subject: [PATCH 01/17] REFACTOR clean up unused line item trait introduced in #4163 --- spec/factories/line_items.rb | 5 ----- 1 file changed, 5 deletions(-) diff --git a/spec/factories/line_items.rb b/spec/factories/line_items.rb index 913be2b479..d6bdddc5ad 100644 --- a/spec/factories/line_items.rb +++ b/spec/factories/line_items.rb @@ -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 From a4d1164ad804c7e9576a39c45492aca7e0784c6b Mon Sep 17 00:00:00 2001 From: Jimmy Li Date: Thu, 1 Aug 2024 17:44:39 +0000 Subject: [PATCH 02/17] FIX typo in params in donation and purchase controllers * Another typo in docs --- app/controllers/donations_controller.rb | 2 +- app/controllers/purchases_controller.rb | 2 +- app/services/itemizable_update_service.rb | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/app/controllers/donations_controller.rb b/app/controllers/donations_controller.rb index a925ab6c41..1c44db3299 100644 --- a/app/controllers/donations_controller.rb +++ b/app/controllers/donations_controller.rb @@ -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 diff --git a/app/controllers/purchases_controller.rb b/app/controllers/purchases_controller.rb index 32e288c6b6..6053712c7d 100644 --- a/app/controllers/purchases_controller.rb +++ b/app/controllers/purchases_controller.rb @@ -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 diff --git a/app/services/itemizable_update_service.rb b/app/services/itemizable_update_service.rb index 45dc7133da..972b432efe 100644 --- a/app/services/itemizable_update_service.rb +++ b/app/services/itemizable_update_service.rb @@ -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] the event class to publish the itemizable to. def self.call(itemizable:, type: :increase, params: {}, event_class: nil) StorageLocation.transaction do From 8ee83ee7db7a33fdc104c0526b6f179aba7044af Mon Sep 17 00:00:00 2001 From: Jimmy Li Date: Fri, 26 Jul 2024 03:40:09 +0000 Subject: [PATCH 03/17] DOCS add comment clarifying itemizable works with line items --- app/models/concerns/itemizable.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/concerns/itemizable.rb b/app/models/concerns/itemizable.rb index 9248238f8b..c77f6d2a21 100644 --- a/app/models/concerns/itemizable.rb +++ b/app/models/concerns/itemizable.rb @@ -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 From 7f776ab542caf1544bb754d82bf5f94e339d6d95 Mon Sep 17 00:00:00 2001 From: Jimmy Li Date: Thu, 1 Aug 2024 21:11:52 +0000 Subject: [PATCH 04/17] REFACTOR rename Item:kits scope to :housing_a_kit, add rspecs * Clearer name * Add rspecs to test :housing_a_kit and :loose scopes --- app/models/item.rb | 4 ++-- spec/models/item_spec.rb | 23 +++++++++++++++++++++++ 2 files changed, 25 insertions(+), 2 deletions(-) diff --git a/app/models/item.rb b/app/models/item.rb index 66ca08f5c7..8ecf42f1b8 100644 --- a/app/models/item.rb +++ b/app/models/item.rb @@ -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) } diff --git a/spec/models/item_spec.rb b/spec/models/item_spec.rb index c2ea38cf07..3cdfbbe753 100644 --- a/spec/models/item_spec.rb +++ b/spec/models/item_spec.rb @@ -57,6 +57,29 @@ expect(Item.by_size("4").length).to eq(2) end + it "->housing_a_kit returns all items which belongs_to (house) a kit" do + name = "test kit" + kit_params = attributes_for(:kit, name: name) + kit_params[:line_items_attributes] = [{item_id: create(:item).id, quantity: 1}] # shouldn't be counted + KitCreateService.new(organization_id: organization.id, kit_params: kit_params).call + + create(:item) # shouldn't be counted + expect(Item.housing_a_kit.count).to eq(1) + expect(Item.housing_a_kit.first.name = name) + end + + it "->loose returns all items which do not belongs_to a kit" do + name = "A" + item = create(:item, name: name, organization: organization) + + kit_params = attributes_for(:kit) + kit_params[:line_items_attributes] = [{item_id: item.id, quantity: 1}] + KitCreateService.new(organization_id: organization.id, kit_params: kit_params).call # shouldn't be counted + + expect(Item.loose.count).to eq(1) + expect(Item.loose.first.name = name) + end + it "->alphabetized retrieves items in alphabetical order" do item_c = create(:item, name: "C") item_b = create(:item, name: "B") From 48c18357392df437b16a0c28a2cb823e69b3174b Mon Sep 17 00:00:00 2001 From: Jimmy Li Date: Fri, 2 Aug 2024 00:43:51 +0000 Subject: [PATCH 05/17] REFACTOR DRY up kit base_item, seed_base_items, prevent kit base_item deletion * Move seed_base_items code into one static function * Move kit base item creation code into one static function * Add code to prevent calling destroy on kit base item and corresponding rspec * Added comments - not sure about whether other base item request specs are useful or what the purpose of destroy is in the controller if it can't be called --- .../admin/base_items_controller.rb | 5 ++- app/models/base_item.rb | 21 +++++++++- app/services/kit_create_service.rb | 41 ++++++------------- db/seeds.rb | 17 +------- spec/factories/organizations.rb | 2 +- spec/rails_helper.rb | 18 -------- .../admin/base_items_requests_spec.rb | 14 ++++++- 7 files changed, 51 insertions(+), 67 deletions(-) diff --git a/app/controllers/admin/base_items_controller.rb b/app/controllers/admin/base_items_controller.rb index 90d37f0a0e..c62e9ce6cf 100644 --- a/app/controllers/admin/base_items_controller.rb +++ b/app/controllers/admin/base_items_controller.rb @@ -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? 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?" diff --git a/app/models/base_item.rb b/app/models/base_item.rb index d910a46f25..c3effddb30 100644 --- a/app/models/base_item.rb +++ b/app/models/base_item.rb @@ -28,5 +28,24 @@ class BaseItem < ApplicationRecord def to_h { partner_key: partner_key, name: name } end -end + def self.seed_base_items + # 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 diff --git a/app/services/kit_create_service.rb b/app/services/kit_create_service.rb index 80e976d2ee..42853e38be 100644 --- a/app/services/kit_create_service.rb +++ b/app/services/kit_create_service.rb @@ -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 + BaseItem.find_or_create_by!(KIT_BASE_ITEM_ATTRS) + end + def initialize(organization_id:, kit_params:) @organization_id = organization_id @kit_params = kit_params @@ -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 = BaseItem.find_or_create_by!(KIT_BASE_ITEM_ATTRS) # 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 } ) @@ -45,7 +54,6 @@ def call private attr_reader :organization_id, :kit_params - def organization @organization ||= Organization.find_by(id: organization_id) end @@ -55,19 +63,6 @@ def kit_params_with_organization organization_id: organization.id }) 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') @@ -88,16 +83,6 @@ def kit_validation_errors kit.valid? @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 - diff --git a/db/seeds.rb b/db/seeds.rb index ccedde9403..a141bf35ca 100644 --- a/db/seeds.rb +++ b/db/seeds.rb @@ -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 diff --git a/spec/factories/organizations.rb b/spec/factories/organizations.rb index 30ef97daa7..945075bb5d 100644 --- a/spec/factories/organizations.rb +++ b/spec/factories/organizations.rb @@ -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 diff --git a/spec/rails_helper.rb b/spec/rails_helper.rb index a8130707cd..8244ccbefc 100644 --- a/spec/rails_helper.rb +++ b/spec/rails_helper.rb @@ -242,21 +242,3 @@ def await_select2(select2, container = nil, &block) find("#{container} select option[data-select2-id=\"#{current_id.to_i + 1}\"]", wait: 10) end - -def seed_base_items - base_items = File.read(Rails.root.join("db", "base_items.json")) - items_by_category = JSON.parse(base_items) - base_items_data = items_by_category.map do |category, entries| - entries.map do |entry| - { - name: entry["name"], - category: category, - partner_key: entry["key"], - updated_at: Time.zone.now, - created_at: Time.zone.now - } - end - end.flatten - - BaseItem.create!(base_items_data) -end diff --git a/spec/requests/admin/base_items_requests_spec.rb b/spec/requests/admin/base_items_requests_spec.rb index ee04d33c1e..a539f52216 100644 --- a/spec/requests/admin/base_items_requests_spec.rb +++ b/spec/requests/admin/base_items_requests_spec.rb @@ -1,15 +1,25 @@ RSpec.describe "Admin::BaseItems", type: :request do let(:organization) { create(:organization) } let(:user) { create(:user, organization: organization) } + let(:super_admin) { create(:super_admin, organization: organization) } let(:organization_admin) { create(:organization_admin, organization: organization) } - # TODO: should this be testing something? context "while signed in as a super admin" do before do - sign_in(@super_admin) + sign_in(super_admin) + end + + it "doesn't let you delete the Kit base item" do + kit_base_item = KitCreateService.FindOrCreateKitBaseItem + delete admin_base_item_path(id: kit_base_item.id) + expect(flash[:alert]).to include("You cannot delete the Kits base item") + expect(response).to be_redirect + expect(BaseItem.exists?(kit_base_item.id)).to be true end end + # TODO aren't organization_admins not allowed to view base items? + # also, some of these tests are sending organization.id instead of BaseItem.id as args context "When logged in as an organization admin" do before do sign_in(organization_admin) From dd4ce7bd1b7492f6b640cd6f063574cc5d6ed4c8 Mon Sep 17 00:00:00 2001 From: Jimmy Li Date: Fri, 2 Aug 2024 01:34:52 +0000 Subject: [PATCH 06/17] Add rspec for item.is_in_kit? --- spec/models/item_spec.rb | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/spec/models/item_spec.rb b/spec/models/item_spec.rb index 3cdfbbe753..78e8b1378e 100644 --- a/spec/models/item_spec.rb +++ b/spec/models/item_spec.rb @@ -385,6 +385,19 @@ end end + describe '#is_in_kit?' do + it "is true for items that are in a kit and false otherwise" do + item_not_in_kit = create(:item, organization: organization) + item_in_kit = create(:item, organization: organization) + + kit_params = attributes_for(:kit) + kit_params[:line_items_attributes] = [{item_id: item_in_kit.id, quantity: 1}] + KitCreateService.new(organization_id: organization.id, kit_params: kit_params).call + expect(item_in_kit.is_in_kit?).to be true + expect(item_not_in_kit.is_in_kit?).to be false + end + end + describe "other?" do it "is true for items that are partner_key 'other'" do item = create(:item, base_item: create(:base_item, name: "Base")) From dd2b5560b06d720834b73d9acc9a230b68fd2ed6 Mon Sep 17 00:00:00 2001 From: Jimmy Li Date: Thu, 8 Aug 2024 22:38:55 +0000 Subject: [PATCH 07/17] Fix lint --- app/services/kit_create_service.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/services/kit_create_service.rb b/app/services/kit_create_service.rb index 42853e38be..f042af0b5d 100644 --- a/app/services/kit_create_service.rb +++ b/app/services/kit_create_service.rb @@ -63,6 +63,7 @@ def kit_params_with_organization organization_id: organization.id }) end + def valid? if organization.blank? errors.add(:organization_id, 'does not match any Organization') @@ -83,6 +84,5 @@ def kit_validation_errors kit.valid? @kit_validation_errors = kit.errors - end end From b59975ae43849fb1f0a137621f72b8a7d77dc5a9 Mon Sep 17 00:00:00 2001 From: Jimmy Li Date: Fri, 9 Aug 2024 01:33:16 +0000 Subject: [PATCH 08/17] rename findorcreatebaseitem to add ! --- app/controllers/admin/base_items_controller.rb | 2 +- app/models/base_item.rb | 2 +- app/services/kit_create_service.rb | 6 +++--- spec/requests/admin/base_items_requests_spec.rb | 2 +- 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/app/controllers/admin/base_items_controller.rb b/app/controllers/admin/base_items_controller.rb index c62e9ce6cf..2bd658613e 100644 --- a/app/controllers/admin/base_items_controller.rb +++ b/app/controllers/admin/base_items_controller.rb @@ -41,7 +41,7 @@ def show # TODO there are no buttons on the view to call this method, consider removing? def destroy @base_item = BaseItem.includes(:items).find(params[:id]) - if (@base_item.id = KitCreateService.FindOrCreateKitBaseItem.id) + 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!" diff --git a/app/models/base_item.rb b/app/models/base_item.rb index c3effddb30..eba5785e35 100644 --- a/app/models/base_item.rb +++ b/app/models/base_item.rb @@ -46,6 +46,6 @@ def self.seed_base_items end end # Create global 'Kit' base item - KitCreateService.FindOrCreateKitBaseItem + KitCreateService.FindOrCreateKitBaseItem! end end diff --git a/app/services/kit_create_service.rb b/app/services/kit_create_service.rb index f042af0b5d..3bf13e9f80 100644 --- a/app/services/kit_create_service.rb +++ b/app/services/kit_create_service.rb @@ -9,7 +9,7 @@ class KitCreateService attr_reader :kit - def self.FindOrCreateKitBaseItem + def self.FindOrCreateKitBaseItem! BaseItem.find_or_create_by!(KIT_BASE_ITEM_ATTRS) end @@ -27,9 +27,9 @@ def call @kit.save! # Find or create the BaseItem for all items housing kits - item_housing_a_kit_base_item = BaseItem.find_or_create_by!(KIT_BASE_ITEM_ATTRS) + item_housing_a_kit_base_item = KitCreateService.FindOrCreateKitBaseItem! - # Create the Item. + # Create the item item_creation = ItemCreateService.new( organization_id: organization.id, item_params: { diff --git a/spec/requests/admin/base_items_requests_spec.rb b/spec/requests/admin/base_items_requests_spec.rb index a539f52216..aee105de93 100644 --- a/spec/requests/admin/base_items_requests_spec.rb +++ b/spec/requests/admin/base_items_requests_spec.rb @@ -10,7 +10,7 @@ end it "doesn't let you delete the Kit base item" do - kit_base_item = KitCreateService.FindOrCreateKitBaseItem + kit_base_item = KitCreateService.FindOrCreateKitBaseItem! delete admin_base_item_path(id: kit_base_item.id) expect(flash[:alert]).to include("You cannot delete the Kits base item") expect(response).to be_redirect From 64e5a5253213934e3dcd6b8dd067242eb04ce3f4 Mon Sep 17 00:00:00 2001 From: Jimmy Li Date: Thu, 15 Aug 2024 00:18:11 +0000 Subject: [PATCH 09/17] REFACTOR remove unused code childrenservedreportservice * All SQL code is duplicated in AcquisitionReportService * RSpec is close enough to what is in AcquisitionReportService Spec that it can be removed without merging in (only difference is Diapers - Adult Briefs category is not created, but that shouldn't matter because the SQL looks for %diaper% so this category isn't testing anything different) --- .../reports/children_served_report_service.rb | 39 ------------ .../children_served_report_service_spec.rb | 61 +------------------ 2 files changed, 1 insertion(+), 99 deletions(-) diff --git a/app/services/reports/children_served_report_service.rb b/app/services/reports/children_served_report_service.rb index 1242ba29a6..70886d5e00 100644 --- a/app/services/reports/children_served_report_service.rb +++ b/app/services/reports/children_served_report_service.rb @@ -31,47 +31,8 @@ def average_children_monthly total_children_served / 12.0 end - def disposable_diapers_from_kits_total - 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 diff --git a/spec/services/reports/children_served_report_service_spec.rb b/spec/services/reports/children_served_report_service_spec.rb index e660760693..14e6446bd1 100644 --- a/spec/services/reports/children_served_report_service_spec.rb +++ b/spec/services/reports/children_served_report_service_spec.rb @@ -42,6 +42,7 @@ create_list(:line_item, 5, :distribution, quantity: 200, item: disposable_item, itemizable: dist) create_list(:line_item, 5, :distribution, quantity: 300, item: non_disposable_item, itemizable: dist) end + # within_time total distributed i infant_distribution = create(:distribution, organization: organization, issued_at: within_time) toddler_distribution = create(:distribution, organization: organization, issued_at: within_time) @@ -105,64 +106,4 @@ }) end end - describe "#disposable_diapers_from_kits_total" do - it "calculates the number of disposable diapers that have been distributed within kits this year" do - organization = create(:organization) - - # create disposable/ nondisposable base items - create(:base_item, name: "Toddler Disposable Diaper", partner_key: "toddler diapers", category: "disposable diaper") - create(:base_item, name: "Infant Disposable Diaper", partner_key: "infant diapers", category: "infant disposable diaper") - create(:base_item, name: "Infant Cloth Diaper", partner_key: "infant cloth diapers", category: "cloth diaper") - create(:base_item, name: "Adult Brief LXL Test", partner_key: "adult lxl test", category: "Diapers - Adult Briefs") - - # create disposable/ nondisposable items - toddler_disposable_kit_item = create(:item, name: "Toddler Disposable Diaper", partner_key: "toddler diapers", organization: organization) - infant_disposable_kit_item = create(:item, name: "Infant Disposable Diapers", partner_key: "infant diapers", organization: organization) - infant_cloth_kit_item = create(:item, name: "Infant Cloth Diapers", partner_key: "infant cloth diapers", organization: organization) - adult_brief_kit_item = create(:item, name: "Adult Brief L/XL", partner_key: "adult lxl test", organization: organization) - - # create line items that contain the d/nd items - toddler_disposable_line_item = create(:line_item, item: toddler_disposable_kit_item, quantity: 5) - infant_disposable_line_item = create(:line_item, item: infant_disposable_kit_item, quantity: 5) - infant_cloth_line_item = create(:line_item, item: infant_cloth_kit_item, quantity: 5) - adult_brief_line_item = create(:line_item, item: adult_brief_kit_item, quantity: 5) - - # create kits that contain the d/nd line items - toddler_disposable_kit = create(:kit, organization: organization, line_items: [toddler_disposable_line_item]) - infant_disposable_kit = create(:kit, organization: organization, line_items: [infant_disposable_line_item]) - infant_cloth_kit = create(:kit, organization: organization, line_items: [infant_cloth_line_item]) - adult_brief_kit = create(:kit, organization: organization, line_items: [adult_brief_line_item]) - - # create items which have the kits - create(:base_item, name: "Unrelated Base", partner_key: "unrelated base", category: "unrelated base") - infant_disposable_dist_item = create(:item, name: "Dist Item 1", organization: organization, partner_key: "unrelated base", kit: toddler_disposable_kit) - toddler_disposable_dist_item = create(:item, name: "Dist Item 2", organization: organization, partner_key: "unrelated base", kit: infant_disposable_kit) - infant_cloth_dist_item = create(:item, name: "Dist Item 3", organization: organization, partner_key: "unrelated base", kit: infant_cloth_kit) - adult_brief_dist_item = create(:item, name: "Dist Item 4", organization: organization, partner_key: "unrelated base", kit: adult_brief_kit) - - within_time = Time.zone.parse("2020-05-31 14:00:00") - - # create empty distributions - infant_distribution = create(:distribution, organization: organization, issued_at: within_time) - toddler_distribution = create(:distribution, organization: organization, issued_at: within_time) - adult_distribution = create(:distribution, organization: organization, issued_at: within_time) - - # add line items to distributions which contain the d/nd kits - create(:line_item, quantity: 10, item: toddler_disposable_dist_item, itemizable: toddler_distribution) - create(:line_item, quantity: 10, item: infant_disposable_dist_item, itemizable: infant_distribution) - create(:line_item, quantity: 10, item: infant_cloth_dist_item, itemizable: infant_distribution) - create(:line_item, quantity: 10, item: adult_brief_dist_item, itemizable: adult_distribution) - - service = described_class.new(organization: organization, year: within_time.year) - - # Find distributions, that has a - # Line item, that has an - # Item, which has a - # Kit, which has a - # Line item, which has an - # Item, which is a disposable diaper. - # And then add all those quantities up - expect(service.disposable_diapers_from_kits_total).to eq(100) - end - end end From 240a32e026303e4e7a6acc207d224dd8836cecf4 Mon Sep 17 00:00:00 2001 From: Jimmy Li Date: Thu, 15 Aug 2024 03:45:30 +0000 Subject: [PATCH 10/17] Fix bin/setup so it's working with seed_base_item move --- db/seeds.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/db/seeds.rb b/db/seeds.rb index a141bf35ca..5884e563b2 100644 --- a/db/seeds.rb +++ b/db/seeds.rb @@ -506,7 +506,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']) From a74dcc9b2d89e90b410404aef99ee07c6af694e4 Mon Sep 17 00:00:00 2001 From: Jimmy Li Date: Tue, 24 Sep 2024 02:11:00 +0000 Subject: [PATCH 11/17] Base Item linting * Rename to snake case * Add docs noting base items will be changed --- app/controllers/admin/base_items_controller.rb | 5 +++-- app/models/base_item.rb | 2 +- app/services/kit_create_service.rb | 4 ++-- spec/requests/admin/base_items_requests_spec.rb | 2 +- 4 files changed, 7 insertions(+), 6 deletions(-) diff --git a/app/controllers/admin/base_items_controller.rb b/app/controllers/admin/base_items_controller.rb index 2bd658613e..b7489d706a 100644 --- a/app/controllers/admin/base_items_controller.rb +++ b/app/controllers/admin/base_items_controller.rb @@ -1,5 +1,7 @@ # [Super Admin] Manage the BaseItems -- this is the only place in the app where Base Items can be # added / modified. Base Items are both the template and common thread for regular Items +# +# See #4656, BaseItems are pending significant changes/possible deletion class Admin::BaseItemsController < AdminController def edit @base_item = BaseItem.find(params[:id]) @@ -38,10 +40,9 @@ def show @items = @base_item.items end - # TODO there are no buttons on the view to call this method, consider removing? def destroy @base_item = BaseItem.includes(:items).find(params[:id]) - if (@base_item.id = KitCreateService.FindOrCreateKitBaseItem!.id) + if (@base_item.id = KitCreateService.find_or_create_kit_base_item!.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!" diff --git a/app/models/base_item.rb b/app/models/base_item.rb index eba5785e35..992cd0bf79 100644 --- a/app/models/base_item.rb +++ b/app/models/base_item.rb @@ -46,6 +46,6 @@ def self.seed_base_items end end # Create global 'Kit' base item - KitCreateService.FindOrCreateKitBaseItem! + KitCreateService.find_or_create_kit_base_item! end end diff --git a/app/services/kit_create_service.rb b/app/services/kit_create_service.rb index 3bf13e9f80..7884d5d61c 100644 --- a/app/services/kit_create_service.rb +++ b/app/services/kit_create_service.rb @@ -9,7 +9,7 @@ class KitCreateService attr_reader :kit - def self.FindOrCreateKitBaseItem! + def self.find_or_create_kit_base_item! BaseItem.find_or_create_by!(KIT_BASE_ITEM_ATTRS) end @@ -27,7 +27,7 @@ def call @kit.save! # Find or create the BaseItem for all items housing kits - item_housing_a_kit_base_item = KitCreateService.FindOrCreateKitBaseItem! + item_housing_a_kit_base_item = KitCreateService.find_or_create_kit_base_item! # Create the item item_creation = ItemCreateService.new( diff --git a/spec/requests/admin/base_items_requests_spec.rb b/spec/requests/admin/base_items_requests_spec.rb index aee105de93..1208415fdb 100644 --- a/spec/requests/admin/base_items_requests_spec.rb +++ b/spec/requests/admin/base_items_requests_spec.rb @@ -10,7 +10,7 @@ end it "doesn't let you delete the Kit base item" do - kit_base_item = KitCreateService.FindOrCreateKitBaseItem! + kit_base_item = KitCreateService.find_or_create_kit_base_item! delete admin_base_item_path(id: kit_base_item.id) expect(flash[:alert]).to include("You cannot delete the Kits base item") expect(response).to be_redirect From 8fde086ec1cdf4e3a346ec452e43406113265e02 Mon Sep 17 00:00:00 2001 From: Jimmy Li Date: Tue, 24 Sep 2024 02:29:44 +0000 Subject: [PATCH 12/17] REFACTOR move seed_base_items into lib util file --- app/models/base_item.rb | 20 -------------------- db/seeds.rb | 3 ++- lib/seed_base_items.rb | 19 +++++++++++++++++++ spec/factories/organizations.rb | 3 ++- 4 files changed, 23 insertions(+), 22 deletions(-) create mode 100644 lib/seed_base_items.rb diff --git a/app/models/base_item.rb b/app/models/base_item.rb index 992cd0bf79..2623528887 100644 --- a/app/models/base_item.rb +++ b/app/models/base_item.rb @@ -28,24 +28,4 @@ class BaseItem < ApplicationRecord def to_h { partner_key: partner_key, name: name } end - - def self.seed_base_items - # 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.find_or_create_kit_base_item! - end end diff --git a/db/seeds.rb b/db/seeds.rb index 5884e563b2..f0cea14d87 100644 --- a/db/seeds.rb +++ b/db/seeds.rb @@ -23,7 +23,8 @@ def random_record_for_org(org, klass) # Base Items # ---------------------------------------------------------------------------- -BaseItem.seed_base_items +require 'seed_base_items' +seed_base_items # ---------------------------------------------------------------------------- # NDBN Members diff --git a/lib/seed_base_items.rb b/lib/seed_base_items.rb new file mode 100644 index 0000000000..027aaa986f --- /dev/null +++ b/lib/seed_base_items.rb @@ -0,0 +1,19 @@ +def seed_base_items + # 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.find_or_create_kit_base_item! +end diff --git a/spec/factories/organizations.rb b/spec/factories/organizations.rb index 945075bb5d..9b6a0f7051 100644 --- a/spec/factories/organizations.rb +++ b/spec/factories/organizations.rb @@ -34,6 +34,7 @@ # account_request_id :integer # ndbn_member_id :bigint # +require 'seed_base_items' FactoryBot.define do factory :organization do @@ -61,7 +62,7 @@ trait :with_items do after(:create) do |instance, evaluator| - BaseItem.seed_base_items if BaseItem.count.zero? # seeds 45 base items if none exist + 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 From 7d25b3aeb4ee4f4b55e7d58d374c39b7f77cbb1a Mon Sep 17 00:00:00 2001 From: Jimmy Li Date: Tue, 24 Sep 2024 02:33:53 +0000 Subject: [PATCH 13/17] REFACTOR use specify -> instead of it -> in test descriptions in item_spec.rb --- spec/models/item_spec.rb | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/spec/models/item_spec.rb b/spec/models/item_spec.rb index 78e8b1378e..2569a0b675 100644 --- a/spec/models/item_spec.rb +++ b/spec/models/item_spec.rb @@ -47,7 +47,7 @@ expect(subject.class).to respond_to :class_filter end - it "->by_size returns all items with the same size, per their BaseItem parent" do + specify "->by_size returns all items with the same size, per their BaseItem parent" do size4 = create(:base_item, size: "4", name: "Size 4 Diaper") size_z = create(:base_item, size: "Z", name: "Size Z Diaper") @@ -57,7 +57,7 @@ expect(Item.by_size("4").length).to eq(2) end - it "->housing_a_kit returns all items which belongs_to (house) a kit" do + specify "->housing_a_kit returns all items which belongs_to (house) a kit" do name = "test kit" kit_params = attributes_for(:kit, name: name) kit_params[:line_items_attributes] = [{item_id: create(:item).id, quantity: 1}] # shouldn't be counted @@ -68,7 +68,7 @@ expect(Item.housing_a_kit.first.name = name) end - it "->loose returns all items which do not belongs_to a kit" do + specify "->loose returns all items which do not belongs_to a kit" do name = "A" item = create(:item, name: name, organization: organization) @@ -80,7 +80,7 @@ expect(Item.loose.first.name = name) end - it "->alphabetized retrieves items in alphabetical order" do + specify "->alphabetized retrieves items in alphabetical order" do item_c = create(:item, name: "C") item_b = create(:item, name: "B") item_a = create(:item, name: "A") @@ -90,7 +90,7 @@ expect(Item.alphabetized.map(&:name)).to eq(alphabetized_list) end - it "->active shows items that are still active" do + specify "->active shows items that are still active" do inactive_item = create(:line_item, :purchase).item item = create(:item) inactive_item.deactivate! From 94030f75aefe8cfa6ecc4e6155da4c8e18bb630f Mon Sep 17 00:00:00 2001 From: Jimmy Li Date: Thu, 17 Oct 2024 22:28:21 +0000 Subject: [PATCH 14/17] FIX typos, move to Seeds module --- .../admin/base_items_controller.rb | 4 ++-- db/seeds.rb | 10 ++++----- lib/seed_base_items.rb | 19 ----------------- lib/seeds.rb | 21 +++++++++++++++++++ spec/factories/organizations.rb | 4 ++-- .../children_served_report_service_spec.rb | 1 - 6 files changed, 30 insertions(+), 29 deletions(-) delete mode 100644 lib/seed_base_items.rb create mode 100644 lib/seeds.rb diff --git a/app/controllers/admin/base_items_controller.rb b/app/controllers/admin/base_items_controller.rb index b7489d706a..e423c08b49 100644 --- a/app/controllers/admin/base_items_controller.rb +++ b/app/controllers/admin/base_items_controller.rb @@ -42,9 +42,9 @@ def show def destroy @base_item = BaseItem.includes(:items).find(params[:id]) - if (@base_item.id = KitCreateService.find_or_create_kit_base_item!.id) + if @base_item.id == KitCreateService.find_or_create_kit_base_item!.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 + elsif @base_item.items.empty? && @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?" diff --git a/db/seeds.rb b/db/seeds.rb index fd7f8545d7..c99dc7415a 100644 --- a/db/seeds.rb +++ b/db/seeds.rb @@ -23,8 +23,8 @@ def random_record_for_org(org, klass) # Base Items # ---------------------------------------------------------------------------- -require 'seed_base_items' -seed_base_items +require 'seeds' +Seeds.seed_base_items # ---------------------------------------------------------------------------- # NDBN Members @@ -856,12 +856,12 @@ def seed_quantity(item_name, organization, storage_location, quantity) # Users invitation status # ---------------------------------------------------------------------------- # Mark users `invitation_status` as `accepted` -# +# # Addresses and resolves issue #4689, which can be found in: # https://github.com/rubyforgood/human-essentials/issues/4689 -User.where(invitation_token: nil).each do |user| +User.where(invitation_token: nil).each do |user| user.update!( invitation_sent_at: Time.current, invitation_accepted_at: Time.current ) -end \ No newline at end of file +end diff --git a/lib/seed_base_items.rb b/lib/seed_base_items.rb deleted file mode 100644 index 027aaa986f..0000000000 --- a/lib/seed_base_items.rb +++ /dev/null @@ -1,19 +0,0 @@ -def seed_base_items - # 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.find_or_create_kit_base_item! -end diff --git a/lib/seeds.rb b/lib/seeds.rb new file mode 100644 index 0000000000..f5cd4537ce --- /dev/null +++ b/lib/seeds.rb @@ -0,0 +1,21 @@ +module Seeds + def self.seed_base_items + # 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.find_or_create_kit_base_item! + end +end diff --git a/spec/factories/organizations.rb b/spec/factories/organizations.rb index 9b6a0f7051..f5fa0aad71 100644 --- a/spec/factories/organizations.rb +++ b/spec/factories/organizations.rb @@ -34,7 +34,7 @@ # account_request_id :integer # ndbn_member_id :bigint # -require 'seed_base_items' +require 'seeds' FactoryBot.define do factory :organization do @@ -62,7 +62,7 @@ trait :with_items do after(:create) do |instance, evaluator| - seed_base_items if BaseItem.count.zero? # seeds 45 base items if none exist + Seeds.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 diff --git a/spec/services/reports/children_served_report_service_spec.rb b/spec/services/reports/children_served_report_service_spec.rb index 14e6446bd1..88b66dc536 100644 --- a/spec/services/reports/children_served_report_service_spec.rb +++ b/spec/services/reports/children_served_report_service_spec.rb @@ -42,7 +42,6 @@ create_list(:line_item, 5, :distribution, quantity: 200, item: disposable_item, itemizable: dist) create_list(:line_item, 5, :distribution, quantity: 300, item: non_disposable_item, itemizable: dist) end - # within_time total distributed i infant_distribution = create(:distribution, organization: organization, issued_at: within_time) toddler_distribution = create(:distribution, organization: organization, issued_at: within_time) From 44c42bedf97abdf9f4b8f2206f017c2a729a855f Mon Sep 17 00:00:00 2001 From: Jimmy Li Date: Thu, 17 Oct 2024 23:24:51 +0000 Subject: [PATCH 15/17] FIX clarify org admins can't view or edit base items in specs --- .../admin/base_items_requests_spec.rb | 49 ++++++++++--------- 1 file changed, 27 insertions(+), 22 deletions(-) diff --git a/spec/requests/admin/base_items_requests_spec.rb b/spec/requests/admin/base_items_requests_spec.rb index 1208415fdb..733d820832 100644 --- a/spec/requests/admin/base_items_requests_spec.rb +++ b/spec/requests/admin/base_items_requests_spec.rb @@ -1,5 +1,5 @@ RSpec.describe "Admin::BaseItems", type: :request do - let(:organization) { create(:organization) } + let(:organization) { create(:organization, :with_items) } let(:user) { create(:user, organization: organization) } let(:super_admin) { create(:super_admin, organization: organization) } let(:organization_admin) { create(:organization_admin, organization: organization) } @@ -18,59 +18,64 @@ end end - # TODO aren't organization_admins not allowed to view base items? - # also, some of these tests are sending organization.id instead of BaseItem.id as args context "When logged in as an organization admin" do before do sign_in(organization_admin) end describe "GET #new" do - it "returns http success" do + it "denies access and redirects" do get new_admin_base_item_path - expect(response).to have_http_status(:found) + expect(flash[:error]).to eq("Access Denied.") + expect(response).to redirect_to(dashboard_path) end end describe "POST #create" do - it "redirects" do - post admin_base_items_path(organization: attributes_for(:organization)) - expect(response).to be_redirect + it "denies access and redirects" do + post admin_base_items_path(id: BaseItem.first.id) + expect(flash[:error]).to eq("Access Denied.") + expect(response).to redirect_to(dashboard_path) end end describe "GET #index" do - it "returns http success" do + it "denies access and redirects" do get admin_base_items_path - expect(response).to have_http_status(:found) + expect(flash[:error]).to eq("Access Denied.") + expect(response).to redirect_to(dashboard_path) end end describe "GET #show" do - it "returns http success" do - get admin_base_item_path(id: organization.id) - expect(response).to have_http_status(:found) + it "denies access and redirects" do + get admin_base_item_path(id: BaseItem.first.id) + expect(flash[:error]).to eq("Access Denied.") + expect(response).to redirect_to(dashboard_path) end end describe "GET #edit" do - it "returns http success" do - get edit_admin_base_item_path(id: organization.id) - expect(response).to have_http_status(:found) + it "denies access and redirects" do + get edit_admin_base_item_path(id: BaseItem.first.id) + expect(flash[:error]).to eq("Access Denied.") + expect(response).to redirect_to(dashboard_path) end end describe "PUT #update" do - it "redirect" do - put admin_base_item_path(id: organization.id, organization: { name: "Foo" }) - expect(response).to be_redirect + it "denies access and redirects" do + put admin_base_item_path(id: BaseItem.first.id) + expect(flash[:error]).to eq("Access Denied.") + expect(response).to redirect_to(dashboard_path) end end describe "DELETE #destroy" do - it "redirects" do - delete admin_base_item_path(id: organization.id) - expect(response).to be_redirect + it "denies access and redirects" do + delete admin_base_item_path(id: BaseItem.first.id) + expect(flash[:error]).to eq("Access Denied.") + expect(response).to redirect_to(dashboard_path) end end end From 0415e49de2b788a33c36c8cc2603736cd5a07a90 Mon Sep 17 00:00:00 2001 From: Jimmy Li Date: Fri, 22 Nov 2024 01:01:14 +0000 Subject: [PATCH 16/17] update bundler version --- Gemfile.lock | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Gemfile.lock b/Gemfile.lock index e105eb0cec..52b32ee585 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -798,4 +798,4 @@ DEPENDENCIES webmock (~> 3.24) BUNDLED WITH - 2.5.21 + 2.5.23 From a861839e9bd172aca6e26dbf1329d2d6815b6dec Mon Sep 17 00:00:00 2001 From: Jimmy Li Date: Fri, 22 Nov 2024 01:05:53 +0000 Subject: [PATCH 17/17] remove extra base item route rspecs --- .../admin/base_items_requests_spec.rb | 48 ------------------- 1 file changed, 48 deletions(-) diff --git a/spec/requests/admin/base_items_requests_spec.rb b/spec/requests/admin/base_items_requests_spec.rb index 733d820832..b32fa7cf33 100644 --- a/spec/requests/admin/base_items_requests_spec.rb +++ b/spec/requests/admin/base_items_requests_spec.rb @@ -23,22 +23,6 @@ sign_in(organization_admin) end - describe "GET #new" do - it "denies access and redirects" do - get new_admin_base_item_path - expect(flash[:error]).to eq("Access Denied.") - expect(response).to redirect_to(dashboard_path) - end - end - - describe "POST #create" do - it "denies access and redirects" do - post admin_base_items_path(id: BaseItem.first.id) - expect(flash[:error]).to eq("Access Denied.") - expect(response).to redirect_to(dashboard_path) - end - end - describe "GET #index" do it "denies access and redirects" do get admin_base_items_path @@ -46,37 +30,5 @@ expect(response).to redirect_to(dashboard_path) end end - - describe "GET #show" do - it "denies access and redirects" do - get admin_base_item_path(id: BaseItem.first.id) - expect(flash[:error]).to eq("Access Denied.") - expect(response).to redirect_to(dashboard_path) - end - end - - describe "GET #edit" do - it "denies access and redirects" do - get edit_admin_base_item_path(id: BaseItem.first.id) - expect(flash[:error]).to eq("Access Denied.") - expect(response).to redirect_to(dashboard_path) - end - end - - describe "PUT #update" do - it "denies access and redirects" do - put admin_base_item_path(id: BaseItem.first.id) - expect(flash[:error]).to eq("Access Denied.") - expect(response).to redirect_to(dashboard_path) - end - end - - describe "DELETE #destroy" do - it "denies access and redirects" do - delete admin_base_item_path(id: BaseItem.first.id) - expect(flash[:error]).to eq("Access Denied.") - expect(response).to redirect_to(dashboard_path) - end - end end end