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

Feature to exclude/include consumable price in BOM price range - FR #7603 #7787

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

rocheparadox
Copy link
Contributor

  • 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.
Copy link

netlify bot commented Aug 1, 2024

Deploy Preview for inventree-web-pui-preview canceled.

Name Link
🔨 Latest commit ac3b2b4
🔍 Latest deploy log https://app.netlify.com/sites/inventree-web-pui-preview/deploys/66ca21ce0be5520008d280da

Copy link

codecov bot commented Aug 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.42%. Comparing base (d68d52b) to head (ac3b2b4).
Report is 77 commits behind head on master.

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     
Flag Coverage Δ
backend 85.38% <ø> (+0.14%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@rocheparadox
Copy link
Contributor Author

@SchrodingersGat , Is there something else to be done from my side to get these changes merged? Thanks.

@SchrodingersGat
Copy link
Member

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

'PRICING_ACTIVE_VARIANTS': {

Add a new setting to determine if consumable items should be included in pricing

2. Update Calculation

Update the get_bom_price_range method, to optionally include or exclude consumable parts:

def get_bom_price_range(self, quantity=1, internal=False, purchase=False):

@rocheparadox
Copy link
Contributor Author

@SchrodingersGat , I will look into it. Thanks for the detailed explanation.

@wolflu05
Copy link
Contributor

wolflu05 commented Aug 8, 2024

maybe it would be also good to get this feature to PUI, because when CUI is dropped, this will be no longer available.

@matmair
Copy link
Member

matmair commented Aug 9, 2024

@SchrodingersGat should this target 0.16.0 or can we move it to 0.17.x?

@matmair matmair added enhancement This is an suggested enhancement or new feature old user interface Issues with Old User interface labels Aug 9, 2024
@SchrodingersGat SchrodingersGat added this to the 0.17.0 milestone Aug 9, 2024
@rocheparadox
Copy link
Contributor Author

rocheparadox commented Aug 14, 2024

@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?

@SchrodingersGat
Copy link
Member

@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!

@rocheparadox
Copy link
Contributor Author

@SchrodingersGat , I believe that you are mentioning Part.models.PartPricing as the existing pricing management system for the Part.

class PartPricing(common.models.MetaMixin):

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement This is an suggested enhancement or new feature old user interface Issues with Old User interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants