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

Adjustments to Adult Incontinence #4361

Merged

Conversation

cielf
Copy link
Collaborator

@cielf cielf commented May 15, 2024

Resolves #4090

Description

Moving adult diapers from disposables to adult_incontinence
Stop double counting adult wipes (now in other)
adjusted the diapers in kit code to match

Type of change

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

How Has This Been Tested?

Just automatic tests.

@cielf cielf marked this pull request as draft May 15, 2024 20:46
@cielf cielf changed the title WIP Adjustments to Adult Incontinence Adjustments to Adult Incontinence May 16, 2024
@cielf cielf marked this pull request as ready for review May 16, 2024 20:24
}

scope :adult_incontinence, -> {
joins(:base_item)
.where(items: { partner_key: %w(adult_incontinence underpads liners) })
.or(where("items.partner_key LIKE '%adult%' AND items.partner_key NOT LIKE '%cloth%'"))
.or(where("items.partner_key LIKE '%adult%' AND items.partner_key NOT LIKE '%cloth%' AND items.partner_key NOT LIKE '%wipes'"))
.or(where("lower(base_items.category) LIKE '%adult%' AND lower(base_items.category) NOT LIKE '%wipes%'"))
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 starting to get really complex... can we document these scopes with examples? I am so ready for this and BaseItem to be gone so we can manage categories on the Item level.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's a reasonable request. Or at least to say what we believe the business rules to be... There is still some ambiguity and I don't know why we are sometimes using the partner_key.

Copy link
Collaborator Author

@cielf cielf May 18, 2024

Choose a reason for hiding this comment

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

I'm also going to say that fixing all the scopes is out of scope of this PR. g (It is, and yet....)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Have added some long-winded commentary, and sent off a couple of additional queries/suggestions to the business.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've got some more input from NDBN, so hold off on the final review for the moment. Should get some changes in tomrorrow.

@cielf cielf marked this pull request as draft May 21, 2024 02:52
@cielf
Copy link
Collaborator Author

cielf commented May 27, 2024

I'm going to expand this to include everything from my recent conversations around this with NDBN. Yes, it is a bit of scope creep. There was much clarification (/change) of where things should be, but we're able to make things a bit simpler as a result. The big things are: making liners two base items -- one for menstrual (which should be the default for what we have already, and one for AI) and clarifying things we either weren't counting or were double counting.

@cielf
Copy link
Collaborator Author

cielf commented May 27, 2024

Hey @dorner -- I've addressed a lot of the ambiguities and outright errors in these scopes. We should now be in a state where everything gets counted, but only gets counted once. I think we might not need quite so much commenting now!

@cielf cielf marked this pull request as ready for review May 27, 2024 16:01
Copy link
Collaborator

@dorner dorner left a comment

Choose a reason for hiding this comment

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

I'm going to trust your business logic here because there's no way I can follow this, even with the simplification. 😃 I think a file got added by mistake though.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this wasn't supposed to be checked in?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You're right on that. Will fix today.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Bonus, I get to learn how to do this!

@dorner dorner merged commit f6e0b3d into rubyforgood:main May 31, 2024
20 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.

Move adult diapers into adult incontinence
2 participants