-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Inventory items condition #232
Inventory items condition #232
Conversation
nothing important here yet. I'm still looking around so I'm doing syntax refactoring at least.. |
Early prototype of model's
|
x | good | average | bad |
---|---|---|---|
A | >=20 | 100 - 199 | <100 |
B | >=100 | 50 - 99 | <50 |
C | >=50 | 25 - 49 | <49 |
Maybe we should add new attr on inventory model and let user sets properties of this on each item (html rank select?)... but than rank enum (dropdown) will be useless.
} else { | ||
return 'average'; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
related code
@turboMaCk, I appreciate the fact that you tried to sort this out w/o the advantage of good requirements work from me, and I'm sorry that was the case as well. Looking at this, I'm thinking that the issue of the stock position or "condition" of an inventory item is a little more nuanced. We have a report that calculates the number of days left of stock in a given item that uses past usage to predict a future stock out. It's called the Days Supply Left In Stock. So ideally, the quantity would be replaced by the calculated number of days left in stock (something like estimatedDaysOfStock). So to do this properly, we would ideally like that to be the const on line 68 of app/models/inventory.js. Since that field doesn't exist yet, I think we can merge this PR, but them I'd like to propose that (if you're willing) you take on this issue: HospitalRun/hospitalrun-server#6 Does that make sense? |
Dont worry. This is a naive prototype. I just open PR to start discussion on this. This one will take a longer in my opinion. There will be a lot of tweaking I think. |
9bff6d8
to
4e3f44f
Compare
@tangollama So it's done. There is temporary hardcoded value for Let me know about settings (days for each rank to indicate each condition) It's already rebased to current master. |
a2c8027
to
da78c7d
Compare
const multiplier = rankToMultiplier(this.get('rank')); | ||
|
||
return getCondition(estimatedDaysOfStock, multiplier); | ||
}), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
implementation on model
da78c7d
to
aff2b0d
Compare
@turboMaCk Wow that was fast. I'm taking a look now at merging this PR in. |
I was working on it at the time you push new commits to master. I'm online and ready to work on any improvements based on your decision right now so let me know what you think. It will be nice to push this forward and move to next step soon. |
@turboMaCk I agree. I am going to merge this and if we need to change settings, we can do that later. |
🤘 ⚡ |
…est-table-test-improvements test(medication-request-table): improvements
related: #209
work in progress