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 indicator fields to partner export #4843

Merged

Conversation

Sukhpreet-s
Copy link
Contributor

@Sukhpreet-s Sukhpreet-s commented Dec 10, 2024

Partial #3067

Description

This PR adds 2 new columns, at the end of existing columns, in the partner export file:

  1. Providing Diapers: "Y" or "N", depending if the partner has at least one distribution (in the last 12 months) with an item of scope :disposable or :cloth_diapers.
  2. Providing Period Supplies: "Y" or "N", depending if the partner has at least one distribution (in the last 12 months) with an item of scope :period_supplies.

Note

The criteria for finding whether a Partner has been provided with "Diapers" is if the Partner has at least one distribution with an item of either :disposable OR :cloth_diapers scope, OR both scopes.

Note

The filter for finding distributions within the last 12 months is based on issued_at attribute of Distribution model.

Example:

Providing Diapers Providing Period Supplies
N N
Y Y
Y Y
Y N
N N
N N
Y Y

Type of change

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Manually and added test cases for:

  1. Adding both columns in the right place/order
  2. If a partner has a distribution with a :disposable item, then it should have "Y" for the Providing Diapers column.
  3. If a partner has a distribution with a :cloth_diapers item, then it should have "Y" for the Providing Diapers column.
  4. If a partner has a distribution with a :period_supplies item, then it should have "Y" for the Providing Period Supplies column.
  5. If a partner has a distribution with items satisfying all the above scopes (:disposable, :cloth_diapers, and :period_supplies) but the distribution is older than 12 months, then it should have "N" for both Providing Diapers and Providing Period Supplies columns.

Copy link
Collaborator

@cielf cielf left a comment

Choose a reason for hiding this comment

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

Hey @Sukhpreet-s Taking a very quick look at this (not yet a real review), I think you missed a point in the requirements:

"Based on discussions with the stakeholder circle, the "Providing Period Supplies" and "Providing Diapers" should be based on whether there have been distributions with period supplies or with diapers in the last 12 months."

The main purpose for this indicator is to support annual reporting in January, so the idea of whether the partner is "currently" being supplied is what we're getting at. I do expect that 98 times out of 100, these will be the same in the wild

@cielf
Copy link
Collaborator

cielf commented Dec 10, 2024

@dorner -- just a note that there will almost certainly be conflicts between this and #4840.

@Sukhpreet-s
Copy link
Contributor Author

👋 @cielf, I’ve updated the indicator logic to ensure it only uses distributions from the last 12 months. When you have a moment, could you please take a quick look to confirm if this aligns with the requirements? Your feedback would be greatly appreciated. 🙏 Thank you! 😊

@Sukhpreet-s Sukhpreet-s force-pushed the 3067-add-indicator-fields-to-partner-export branch from 1ca43d3 to 95154fa Compare December 11, 2024 05:28
cielf
cielf previously requested changes Dec 11, 2024
Copy link
Collaborator

@cielf cielf left a comment

Choose a reason for hiding this comment

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

Was working with wrong version for this. Retesting.

@cielf cielf dismissed their stale review December 11, 2024 17:15

Was working with wrong version.

Copy link
Collaborator

@cielf cielf left a comment

Choose a reason for hiding this comment

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

Passes manual testing (after I brought down the right version!) I noticed one thing in the tests regarding the date of the distribution that should fail, but otherwise over to @dorner for technical comments.

spec/models/partner_spec.rb Show resolved Hide resolved
@cielf cielf requested a review from dorner December 11, 2024 17:38
app/models/distribution.rb Show resolved Hide resolved
@dorner
Copy link
Collaborator

dorner commented Dec 13, 2024

@Sukhpreet-s looks good to me, but there's now a conflict. Can you fix please?

@Sukhpreet-s Sukhpreet-s requested a review from cielf December 13, 2024 05:15
@cielf cielf merged commit 5538cc5 into rubyforgood:main Dec 13, 2024
11 checks passed
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