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

Add missing kit rspecs, DRY up kit base items and report service #4665

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

jimmyli97
Copy link
Contributor

@jimmyli97 jimmyli97 commented Sep 24, 2024

Split off from PR #4585
Various refactors and documentation changes

Description

  • Rename item.kits scope to item.housing_a_kit
  • DRYs up kit base_item seeding
  • Removes duplicated code and rspec from ChildrenReportService
  • Add rspecs to:
    • prevent kit base_item deletion
    • test item.is_in_kit?, item.loose, item.housing_a_kit
  • Add documentation

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • Documentation update

How Has This Been Tested?

passes test suite

* Clearer name
* Add rspecs to test :housing_a_kit and :loose scopes
… 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
* 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)
* Rename to snake case
* Add docs noting base items will be changed
@@ -31,47 +31,8 @@ def average_children_monthly
total_children_served / 12.0
end

def disposable_diapers_from_kits_total
organization_id = @organization.id
Copy link
Contributor Author

@jimmyli97 jimmyli97 Sep 24, 2024

Choose a reason for hiding this comment

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

(continuing conversation from #4585)

these lines were removed because they're unused in this service and duplicated in AcquisitionReportService (probably the author copied AcquisitionReportService as a template when creating this service and forgot to delete them)

@cielf cielf requested a review from dorner September 24, 2024 12:52
@cielf
Copy link
Collaborator

cielf commented Sep 28, 2024

@jimmyli97 FYI: We had an urgent fix that required all the senior contributors this week , so we didn't get to look at this again. Hopefully this week will go better.

@@ -40,7 +42,9 @@ def show

def destroy
@base_item = BaseItem.includes(:items).find(params[:id])
if @base_item.items.any? && @base_item.destroy
if (@base_item.id = KitCreateService.find_or_create_kit_base_item!.id)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Assignment inside a condition is pretty prone to bugs. Can we separate them out into two lines?

if @base_item.items.any? && @base_item.destroy
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
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't this be @base_item.items.empty?

@@ -0,0 +1,19 @@
def 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.

Top-level functions look kind of weird in a Rails app. Can this be wrapped in a module?

It worked in rails_helper because those kind of helper methods are fine if used specifically for test setup.

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
Copy link
Collaborator

Choose a reason for hiding this comment

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

Pretty sure they can't. @cielf ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Org admins can't see the base items pages.

They see a list of base items that they pick from when setting up items, and there are places they can filter by base item, but they can't view or edit the base item information.

@@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

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

accidental commit?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants