-
-
Notifications
You must be signed in to change notification settings - Fork 800
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
Feature to exclude/include consumable price in BOM price range - FR #7603 #7787
base: master
Are you sure you want to change the base?
Conversation
rocheparadox
commented
Aug 1, 2024
- A checkbox is added in the BOM panel in parts section to decide if the consumables' price have to be added to the BOM pricing
- A listener is attached to the checkbox to refresh the BOM table data when a change in the checkbox is detected.
An onInput event is added for fields in forms that gets triggered everytime an input is detected in the field
…nventree#7603 - A checkbox is added in the BOM panel in parts section to decide if the consumables' price have to be added to the BOM pricing - A listener is attached to the checkbox to refresh the BOM table data when a change in the checkbox is detected.
… to the BOM pricing
✅ Deploy Preview for inventree-web-pui-preview canceled.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #7787 +/- ##
==========================================
- Coverage 83.48% 77.42% -6.07%
==========================================
Files 1127 1113 -14
Lines 50225 58622 +8397
Branches 1718 1440 -278
==========================================
+ Hits 41932 45387 +3455
- Misses 7855 12858 +5003
+ Partials 438 377 -61
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@SchrodingersGat , Is there something else to be done from my side to get these changes merged? Thanks. |
Hi @rocheparadox sorry I have not had a chance to review this yet. This approach is only going to impact the front-end representation, not the actual calculation (and storing) of BOM pricing. I think that you should: 1. Add new global setting InvenTree/src/backend/InvenTree/common/models.py Line 1636 in e0a8784
Add a new setting to determine if consumable items should be included in pricing 2. Update Calculation Update the InvenTree/src/backend/InvenTree/part/models.py Line 2021 in e0a8784
|
@SchrodingersGat , I will look into it. Thanks for the detailed explanation. |
maybe it would be also good to get this feature to PUI, because when CUI is dropped, this will be no longer available. |
@SchrodingersGat should this target 0.16.0 or can we move it to 0.17.x? |
@SchrodingersGat, Would it not be better, from a design standpoint, to provide this option as a part specific local decision? Doesn't adding this as a global setting restrict the users, to the selected option, throughout all BOM calculations? What if the users need to include the consumbale as part of their BOM pricing for one part and want to exclude in another? |
@rocheparadox maybe - but at the moment our pricing management system does have provision for a "per part" option like this. We could certainly extend the pricing system, but, it will quickly become complex and everyone will want a slightly different approach... This is where I think handing such decisions off to a custom pricing plugin system would make more sense. If you have any ideas or input on how this should go, I'd be interested to hear them! |
@SchrodingersGat , I believe that you are mentioning Part.models.PartPricing as the existing pricing management system for the Part. InvenTree/src/backend/InvenTree/part/models.py Line 2508 in 70a52c9
Nevertheless, I was thinking about adding a field called include_consumbales to that class and factoring that field in the BOM pricing calculations. However, as you mentioned, pricing itself would vary according to different users and it might be better left as a plugin development option. I am open to implementing either the global setting or this. I think, you know the user requirement better. |