agritheory / inventory_tools

A collection of features to streamline and enhance inventory management and manufacturing workflows in ERPNext.
https://agritheory.com/documentation/inventory_tools
Other
24 stars 13 forks source link

Work Order Subcontracting #13

Closed HKuz closed 1 year ago

HKuz commented 1 year ago

DRAFT Addresses #2

Updated to-dos:

HKuz commented 1 year ago

@agritheory - I know you're traveling, whenever suits your timing, this is in a state where a demo/discussion to get feedback makes sense.

HKuz commented 1 year ago

Status update: Comments are resolved - I created two BOMs and revised the test data with a few tweaks (and tested that the site re-installs without issue). The UOM conversions from one commit were moved into the Item UOM Conversion Detail table instead. To test this, I created two Pie Crust Service items - one in stock UOMs of Nos, the other in Hours and checked that adjustments to quantities were using the right UOM when WOs were adding or removing finished good qtys.

One head's up, I had to manually adjust the columns in the Subcontracting table on the Purchase Invoice to see all the necessary columns (the Paid_Qty and To_Pay_Qty fields weren't showing automatically).

There are a handful of potential UI refinements that are still outstanding (also noted in code):

agritheory commented 1 year ago

One head's up, I had to manually adjust the columns in the Subcontracting table on the Purchase Invoice to see all the necessary columns (the Paid_Qty and To_Pay_Qty fields weren't showing automatically).

What does "manually adjust" mean?

HKuz commented 1 year ago

@agritheory - manually adjust means I had to click the wheel icon at the side of the table, and tick off those fields to include them (then adjust the widths so everything would fit)

agritheory commented 1 year ago

manually adjust means I had to click the wheel icon at the side of the table, and tick off those fields to include them (then adjust the widths so everything would fit)

@hkuz I added some JS to handle this as well as some changes to the workflow. I've been testing by creating a PO from the Work Order; it seem to go according to plan. Can you give this some testing to see how if works for you? Please do a reinstall and setup to pick up some new details

HKuz commented 1 year ago

@agritheory - new functionality looks good! I removed a couple text artifacts that threw errors. Also, when a new subcontracting PO was created off a WO, I was getting an error that the Item had to be a non-stock one, then couldn't be None. To get around these, I commented out the line that adds the item_code field (so Item will be blank for User to fill in) and put the doc's 'ignore_validate' and 'ignore_mandatory' flags to true. When the filter in subcontracted POs that forces only service items is removed, we should be able to revert the changes I made.

agritheory commented 1 year ago

To get around these, I commented out the line that adds the item_code field (so Item will be blank for User to fill in) and put the doc's 'ignore_validate' and 'ignore_mandatory' flags to true. When the filter in subcontracted POs that forces only service items is removed, we should be able to revert the changes I made.

I think we should consider the fix for #14 as well as a server-side validation in scope of this PR.

Edit: added it to JS, there is no server-side validation.

HKuz commented 1 year ago

Latest push includes:

The validation fix ended up needing the monkey patch in __init__ vs a query change. The override whitelisted function with an in-function monkey patch worked great as long as that got called in a session first by say, adding an item to a form in the UI. However, the get_item_details and problematic validate_item_details get called in the validation process purely on the server side when we create a PO from a WO. So if I ran bench migrate and refreshed the app, then tried creating the PO programmatically off the WO before a client-side call, it used the erpnext versions and would error out. But putting the patch in __init__ handled that case.

agritheory commented 1 year ago

We are going to want to very clearly comment the override in __init__, it's just about the biggest footgun possible.

agritheory commented 1 year ago

@HKuz Please pull / rebase. This now incorporates the changes from version-14. What needs validation now is that the manual tests for this work. Automated tests for the other features pass.

HKuz commented 1 year ago

@agritheory - I retested the latest code and all functionality looked good. I had to make one tweak to the PI stock entry query to ignore entries against a cancelled PO (this came up when a subcontracting PO's WO had stock entries, then I added a new WO to that PO (cancelling and amended it), then when I fetched stock entries in the PI I saw the ones for the original WO displayed for both the cancelled and the amended PO).