Calculating disposable diapers from kits

[jadekstewart3]

• I have performed a self-review of my own code,
• I have commented my code, particularly in hard-to-understand areas,
• I have made corresponding changes to the documentation,
• I have added tests that prove my fix is effective or that my feature works,
• New and existing unit tests pass locally with my changes ("bundle exec rake")

-->

Resolves #3989

Description

I made modifications to the acquisition report service and the children served report service to take disposable diapers distributed in kits in the annual report. I have added/ modified tests to reflect the changes to the calculations.

Type of change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Tested in the spec/services/reports/children_served_report_service_spec.rb and spec/services/reports/acquisition_report_service_spec.rb

[cielf]

Hey @jadekstewart3 -- Just documenting the suggestion I passed on in slack to call the non-kit diapers loose diapers, to increase the readability of the whole thing. Maybe use "all" to designate when we are talking about both?

[jadekstewart3]

@cielf I think this is ready for review. I know there is a failing check, but the file in question is passing locally. I'm not 100% sure why its not passing completely

[cielf]

@jadekstewart3 We do have some flakey tests. I'm giving @dorner a mention here, as he has been doing some work in that area.

[cielf]

Hey @jadekstewart3 I'm running to the end of my work on this for today -- but is this back in a "ready for review" state?

[jadekstewart3]

@cielf yes it is, thank you!

[cielf]

Hrm. I just did a light manual test on this as follows: 1/ create a distribution with 50 of 2 different items in it that are classed as disposable diapers, dated last year 2/ choose the 2023 annual report and recalculate it 3/ confirm that there are 100 disposable diapers distributed. 4/ Create a kit that has 50 of each of those 2 items in it. Allocate 3 kits. 5/ Create a distribution of 2 of those kits, dated last year 6/ choose the 2023 annual report and recalculate it 7/ I expect there to be 300 disposable diapers distributed. There are 100.

[cielf]

@jadekstewart3 I'll try to kick the tires on this tomorrow -- the suggested sql looks like it should work to me! Rubocop is complaining, though -- if you have the cycles to address that in the meantime, that'd be great.

[jadekstewart3]

@cielf Hi! I did have to fiddle with the query a bit, but it is returning an integer (YAY!) 😀 It is calculating non disposable items as well, I am having a bit of a time figuring out why. Another set of eyes would be amazing! I pushed up my additional test and the almost working method 👍

[cielf]

@cielf Hi! I did have to fiddle with the query a bit, but it is returning an integer (YAY!) 😀 It is calculating non disposable items as well, I am having a bit of a time figuring out why. Another set of eyes would be amazing! I pushed up my additional test and the almost working method 👍

Is it giving all non disposable items, or just the cloth?

[cielf]

Hrm. I put together a distribution that had a disposable item and a kit with disposable items for testing, and the report only seemed to count the disposable items -- not the ones in the kit. Looking at it in more detail...

[jadekstewart3]

@cielf with the updated test this seems to be working! YAY! My test set up was incorrect it seems! p.s. brakeman seems to be upset with the sql query

[cielf]

@dorner -- brakeman thinks there is a SQL injection issue with the query -- can you speak to that? I'll try to kick the tires on this today or tomorrow.

[dorner]

I think that might be because #{year} and #{organization_id} is in the query via interpolation instead of using $1 like the one I posted.

[cielf]

@jadekstewart3 Hmmm.... doesn't seem to be including the kits in. Here's the sequence I tried. From a fresh seed: 1/ check the disposable diapers in the 2023 report - call this amount A. 2/ Add a kit with 5 Adult Briefs L /XL, and 5 Adult Briefs M/L 3/ Allocate at least 10 of those 4/ Create a distribution in 2023 with 10 of those kits, and 10 Adult Briefs L/XL 5/ Recalculate the report 6/ I expect to see Disposable Diapers distributed as A + 110. I see Disposable Diapers as A + 10 -- so I know it's picking up the Adult Briefs L/XL, but it doesn't seem to be picking up the diapers in the kit.

Thank you for sticking with this through a fairly long haul!

[cielf]

(Pokes into the code a bit) Looks like you might only be using the magic SQL for the children served service, and not for the aquisition_report_service. That would explain it.

[jadekstewart3]

@cielf I am trying to implement the magic sql in the acquisition report and something is funky. Can you take a look? when I test the distributed_disposable_diapers_from_kits it seems to return 100, but in the full report it is returning 1500?

[dorner]

@jadekstewart3 there seems to be some flakiness here - the order of tests seems to matter. If you run it with --seed 43773, both tests fail with 1500. (In other words, if returns the correct quantity of disposable diapers from kits runs first, it fails, but if it runs after the other test, it passes.)

[dorner]

I think this has to do with it:

      disposable_item = organization.items.disposable.first
      non_disposable_item = organization.items.where.not(id: organization.items.disposable).first

Rather than creating data from scratch, you're relying on items that were created by the factory/seed. To be sure that only the data you care about is being created, you probably should clear out all the items and only create the ones you want to see in the report.

[jadekstewart3]

@dorner I am getting a return value of 1500 for the sql query that is calculating distributed disposable diapers from kits. I've gone over my test set up a few times and created the kits and kit items in a similar fashion to the set up in the children_served_report_service_spec, but I cant quite figure out why we are returning 1500 instead of the expected 100.

[cielf]

Hey @jadekstewart3 -- did you look into adding in a clause to in your sql to make sure that the itemizable type for the kit line items is kit?

[jadekstewart3]

@cielf I was messing with it yesterday but still having troubles, but I think I just got it!

[jadekstewart3]

@cielf I think this is working! It looks to be failing on a test file that doesnt have anything to do with the reports! 😃

[cielf]

It passes my functional test (adding a mixed diaper and kit-with-diapers distribution) with flying colours! Hurrah!

[cielf]

Hey @dorner -- Just checking if you have any last comments on this -- you've chimed in, but more on a problem solving than critiquing the code pov. Thanks!

[cielf]

I should probably take this out for one last spin before we merge it. [Edit: Actually -- no -- if it's only comment removal, I don't need to.]

[cielf]

Holding off on the merge until the release of April 7 is done.

[github-actions[bot]]

@jadekstewart3: Your PR Calculating disposable diapers from kits is part of today's Human Essentials production release: 2024.04.14. Thank you very much for your contribution!