Conversation
❌ Deploy Preview for inventree-web-pui-preview failed.
|
matmair
left a comment
There was a problem hiding this comment.
Very good start; looking at this we might want to change how a few parts of the code base work, this seems much to complicated for someone new to wrap their head around. Thanks for doing so still
| ), | ||
| # API endpoints for transfer orders | ||
| path( | ||
| 'to/', |
There was a problem hiding this comment.
I think we should strive for a more human-readable API so transfer-order would be better here IMO
There was a problem hiding this comment.
I tend to agree. I matched the pattern already present, such as so/ for Sales Orders, ro/ for Return Orders, etc. I don't think I should change all API paths, so would you rather me match existing patterns or have Transfer Order diverge from the pattern but start more human-readable?
There was a problem hiding this comment.
Currently starting to write API docs in inventree/project-adr#4 that make things like this clearer; I think starting to diverge would be good. One less thing that we will need to break
src/backend/InvenTree/part/models.py
Outdated
| return sum([ | ||
| self.build_order_allocation_count(**kwargs), | ||
| self.sales_order_allocation_count(**kwargs), | ||
| self.transfer_order_allocation_count(**kwargs), |
There was a problem hiding this comment.
are we sure we want to include this here?
There was a problem hiding this comment.
I'm not sure. This starts to creep in my question about whether we want to consider stock allocated to a Transfer Order as "available" or not, which also impacts the "required quantity"
There was a problem hiding this comment.
It is an interesting question. I think that stock which is "in transfer" is not necessarily "unavailable" - merely moving from one location to another?
Alternatively if they are "unavailable" until the transfer order is complete, this also could make sense because it precludes them being assigned elsewhere (until the transfer is completed)
There was a problem hiding this comment.
Here's an argument against including "reduced availability" logic for Transfer Orders.....
The spot it hurts most is actually not at the part allocation counts, referenced here, but at the stock item allocation count in StockItemSerializer.annotate_queryset(). The part level allocation counts at least are "released" once the order is no longer "pending", as defined by TransferOrderStatusGroups and Part.transfer_order_allocations()
However, if I don't clear allocations, the available stock remains reduced even after the order completes from the point of view of the stock item.
@staticmethod
def annotate_queryset(queryset):
....
# Annotate the queryset with the total allocated to sales orders
queryset = queryset.annotate(
allocated=Coalesce(
SubquerySum('sales_order_allocations__quantity'), Decimal(0)
)
# THIS IS THE PROBLEMATIC LINE!
# this count isn't 'released' after the TO completes,
# forever reducing available qty for a stock item that has been through a TO
+ Coalesce(SubquerySum('transfer_order_allocations__quantity'), Decimal(0))
+ Coalesce(SubquerySum('allocations__quantity'), Decimal(0))
)And since in the current implementation, the TransferOrderAllocation is the only thing attaching a StockItem to a TransferOrder, if I clear allocations on complete, I lose the reference for which stock was transferred from this order.
In lieu of a more sophisticated immutable tracking on the transfer order - which I'm beginning to think is more scope than I'm willing to commit to in this PR - I think excluding TO allocations from any availability calculations is best, and then revisit when/if the immutable tracking exists.
|
I'll be on vacation for the next 10 days, so won't be working on this until I'm back. Before spending any more serious time on it when I return, I'd like to hear from your perspective what should be my priorities before this can be merged. I've detailed some of my thoughts in the PR description. The basic functionality present is probably enough for me to start using it on my instance - maybe behind a feature flag - but I know there's a lot of housekeeping left to do before it could be considered "official." |
Documentation@jacobfelknor this will need to also have comprehensive docs added, under the "stock" master docs location :) |
SchrodingersGat
left a comment
There was a problem hiding this comment.
@jacobfelknor this is a really good implementation, great effort!
src/backend/InvenTree/part/models.py
Outdated
| return sum([ | ||
| self.build_order_allocation_count(**kwargs), | ||
| self.sales_order_allocation_count(**kwargs), | ||
| self.transfer_order_allocation_count(**kwargs), |
There was a problem hiding this comment.
It is an interesting question. I think that stock which is "in transfer" is not necessarily "unavailable" - merely moving from one location to another?
Alternatively if they are "unavailable" until the transfer order is complete, this also could make sense because it precludes them being assigned elsewhere (until the transfer is completed)
60ad50a to
46e42b0
Compare
This PR should still be considered a draft, but has been opened in order to facilitate review.
It adds a Transfer Order to InvenTree which is based on other order types, such as a Sales Order.
Done:
TRANSFERORDER_ENABLED,TRANSFERORDER_REFERENCE_PATTERN, andTRANSFERORDER_REQUIRE_RESPONSIBLEDifferences between Transfer Order and other Orders
Items I'd still like to explore
Adding a "consume" option which consumes stock against an order rather than moving it.
Immutable tracking on the transfer.
Similarly to BO's "consume stock" feature, it may be nice to execute the Transfer Order in smaller increments. We could add a "transfer now" feature to allocated stock, doing partial transfers before the entire order is complete
add a Transfer Order Allocations table to the stock item Allocations panel
unit testing
documentation
General questions
IN_STOCK_FILTERwould need to account for transfer orders in this caseTesting