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

Improve synchronization of items and their kits -- active status #3348

Open
2 tasks
cielf opened this issue Jan 26, 2023 · 12 comments
Open
2 tasks

Improve synchronization of items and their kits -- active status #3348

cielf opened this issue Jan 26, 2023 · 12 comments
Labels
BLOCKED This issue is blocked by another

Comments

@cielf
Copy link
Collaborator

cielf commented Jan 26, 2023

Summary

When you change the active status of a kit, that should be reflected in its corresponding item, and vice versa

Why

Reduce bank confusion (and support questions)

Details

When you create a kit, a corresponding item is also created. Currently The item and the kit can be deactivated/reactivated independently.
The active status of the kit and the item should always be the same.

  • If you deactivate an item, then deactivate the kit also.
  • If you deactivate a kit, then deactivate the item also
  • Similarly for reactivation.

Criteria for completion

  • Kits and Items active status are kept the same
  • Tests confirming this behaviour
@cielf cielf added the Help Wanted Groomed + open to all! label Jan 26, 2023
@fchatterji
Copy link
Contributor

Hey, I'd like to work on this issue if possible :)

@dorner
Copy link
Collaborator

dorner commented Feb 19, 2023

It's yours!

@fchatterji
Copy link
Contributor

Hey @dorner @cielf , I checked the code. As the kit status and item status must always be the same, I would propose keeping just one database field, the item status. The kit status can then be delegated to item status. Because why have two database fields when they contain the same information.

There's a snag though: in the current code, kits must have at least one line item (there is a validation for it), but they must not have an item (there is no validation), even though items and line items are linked. So you can theoretically create a kit, with a line item pointing to an item on a different kit. Is that intended?

If it's a feature, then the only way I see to resolve the issue is to use multiple callbacks on create and update, on both kits and items, to ensure sync.

@dorner
Copy link
Collaborator

dorner commented Feb 20, 2023

That sounds like it's an oversight rather than a feature. @cielf can you confirm?

@cielf
Copy link
Collaborator Author

cielf commented Feb 20, 2023

Remembering that kits contain items, but there is an item that contains/represents the whole kit -- and it's hard to talk about both. So I'm not 100% sure that I get the question... here goes.

From the business point of view, you should be able to have the same item appear in different kits. For example, you might have a kit that has 50 newborn diapers and some wipes, and have another kit that has 40 size 6 diapers, wipes, and a book. In both cases, that kit would have the item "wipes".

Does that clear things up?

For clarity -- when we're talking about keeping the item and the kit synchronized we are talking about the item that "is" the kit, rather than the items that are pointed to by the line items In the kit.

@fchatterji
Copy link
Contributor

@cielf Hmm, ok sorry for the unclear question. Let me try again :)

When i talk about item and kit i talk about the "item" and "kit" models in rails. The fields you want synchronized are item.active and kit.active. Best solution seems to me to just keep one field in the database, to avoid a complex sync. The kit and item model have a one-on-one relationship. So we could keep item.active. To get kit.active, you would just look at the corresponding item.active field.

This would work, except that you can create a kit without an item. Question is, is it normal to be able to create a kit without an item? And Iḿ asking because a kit must have at least one line item (there is a validation for it in the kit model, line 51), so it seems logical to have also an item.

Hoping this makes it clearer. Or am I just overthinking this and we could add callbacks to synchronise the two fields?

@cielf
Copy link
Collaborator Author

cielf commented Feb 22, 2023

Ah. Are you saying that a kit can be created without an item through the user interface? Or just that the model allows it. If the latter (but not the former) I would term it an oversight. If you're saying we can create kits without items through the app, I'll need to take a deeper look to answer your question.

Your logic does not take into account that there are two different ways that kits relate to items. One is that there is an item that represents the kit, the other are the items that the kit "contains". I'm 99% sure that the line items relate to the latter, and the item, the former. Does that track?

@fchatterji
Copy link
Contributor

fchatterji commented Feb 22, 2023

@cielf Yes i was talking about the rails models, it is not possible in the interface, it returns an error. I'll try to fix the oversight.

Thanks for the info, I understand the confusion, yeah I was only talking about the item representing the kit.

@github-actions
Copy link
Contributor

github-actions bot commented Mar 5, 2023

This issue has been inactive for 252 hours (10.50 days) and will be automatically unassigned after 108 more hours (4.50 days).

@github-actions
Copy link
Contributor

This issue has been inactive for 372 hours (15.50 days) and is past the limit of 360 hours (15.00 days) so is being unassigned.

@cielf cielf added BLOCKED This issue is blocked by another and removed Help Wanted Groomed + open to all! labels Apr 18, 2023
@cielf
Copy link
Collaborator Author

cielf commented Apr 18, 2023

All things Kit are currently Blocked -- we are considering a redesign of the whole Kit-and-Kaboodle, and hence are putting things on hold.

@jimmyli97
Copy link
Contributor

jimmyli97 commented Aug 2, 2024

This was mostly resolved by #3982 - rspecs testing that active/inactive status are kept the same are missing, but that might be moot soon since I'm making good progress on #3707 and planning on tackling the rest of #3652

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BLOCKED This issue is blocked by another
Projects
None yet
Development

No branches or pull requests

4 participants