Incoming shipments in MovedBoxes data #2208
Conversation
pylipp
force-pushed
the
incoming-shipments-in-movedboxes-data
branch
from
dac1879 to
27d89d6
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2208 +/- ##
=======================================
Coverage 74.43% 74.43%
=======================================
Files 276 276
Lines 19067 19068 +1
Branches 1744 1744
=======================================
+ Hits 14192 14193 +1
Misses 4831 4831
Partials 44 44
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| shipment sh | ||
| ON | ||
| d.shipment_id = sh.id AND | ||
| d.removed_on IS NULL AND |
There was a problem hiding this comment.
these should be on a WHERE statement for clarity - I would keep the JOINS together and the WHERE statement at the end as that's the convention. Unless you have a reason to do things that way?
There was a problem hiding this comment.
I thought the operation would be more performant when the filtering of WHERE is already included in the JOIN conditions
There was a problem hiding this comment.
No, it doesn't work like that - it's actually an important conceptual difference that ties into indexing and Pk selection, and I recommend you look at the tech behind join optimization if you're going to try to write SQL that way.
| sh.receiving_started_on IS NOT NULL AND | ||
| sh.canceled_on IS NULL AND | ||
| sh.state != %s | ||
| JOIN products sp ON sp.id = d.source_product_id |
There was a problem hiding this comment.
why an inner join for source products and a left join for target products?
There was a problem hiding this comment.
The goal is to show data of all boxes in the shipment that's being received, regardless of being reconciled or not.
While the shipment is in receiving state, boxes that have not yet been reconciled, have NULL target product. Using an inner join for target products would exclude those boxes.
The source product however is always not NULL because it's copied and set at the moment the box the added to the shipment on the source side (i.e. when a shipment detail is created)
| -- Collect information about all boxes sent to the specified base as target, that | ||
| -- were not removed from the shipment during preparation | ||
| SELECT | ||
| DATE(dps.receiving_started_on) AS moved_on, |
There was a problem hiding this comment.
This is the shipment receive start date. Food for thought - should we use the shipment receive start date, or the reconciliation date? Because technically none of the contents of the shipment have been verified.
There was a problem hiding this comment.
I see, you want to avoid that data is being displayed that is supposed to be in the shipment but has not been reconciled. I like the idea, it would also simplify the entire query quite a lot (no need for distinguished reconciled and not-yet reconciled boxes).
Just one scenario that could be a bit confusing: assume reconciliation is being start on April 30th and finished on May 1st. When users apply a time range filter on the statviz page to show only April data, they won't see the boxes reconciled in May included, and they might think "why is only half of the shipment displayed that I received in April"
There was a problem hiding this comment.
Agreed: use detail.received_on date, and only include reconciled boxes
| UNION ALL | ||
| -- Collect information about all boxes sent to the specified base as target, that |
There was a problem hiding this comment.
I also think for the sankey to be coherent, we will need to see the created boxes / items for stuff registered. Is this already in the larger stats queries?
There was a problem hiding this comment.
In the same query for the Sankey, we've already included boxes that are created in a Donated location (we assume these boxes have been gone out to a different org, and are not part of the warehouse anymore).
Do you mean that in the Sankey you want to have an "incoming flow" of created boxes from the left? I agree that this is more consistent once we show boxes from incoming shipments, too. However then shouldn't we also include the count of boxes that are stationary to the warehouse? (I'm thinking of a simple Sankey showing 100 created InStock boxes coming from the left, and 10 shipped boxes leaving to the right. Either the user thinks "alright, this means, I have 90 InStock boxes currently", or they wonder "what's with the difference of 90 boxes?", or maybe they don't wonder at all 😄 )
There was a problem hiding this comment.
Yes, exactly! I'm thinking we keep an inflows of already instock items as well. Though as I think of it, we probably need a visual sankey mockup to see what's effective.
There was a problem hiding this comment.
Agreed: think of data structure to include incoming shipments in CreatedBoxes query
| JOIN products sp ON sp.id = d.source_product_id | ||
| LEFT JOIN products tp ON tp.id = d.target_product_id | ||
| ) dps -- details/products/sizes | ||
| JOIN camps c ON c.id = dps.source_base_id |
There was a problem hiding this comment.
Hmm. Currently when a base or org is deleted, we delete also the name in the DB right? So if a partner leaves, we'll simply have a null org in this data?
There was a problem hiding this comment.
No. When a partner org or base is deactivated, we set the deleted_on timestamp but keep the name.
There was a problem hiding this comment.
In that case I want to bring this up in standup as well since we've had issues with the statistics in the past
There was a problem hiding this comment.
@aerinsol I forgot to bring this up. I'm not sure which issues you mean since we do keep the base name after deactivating.
Thanks @aerinsol for the valuable feedback. Please confirm for 2. and 3. below
|
|
I confirm 2, I think for 3 I need to understand the current shape of the data first. We can discuss during standup. |
2 participants
@aerinsol feel free to check out the SQL query