-
Notifications
You must be signed in to change notification settings - Fork 20
9737 add basic inbound shipments report #9805
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
base: v2.14.0-RC
Are you sure you want to change the base?
Conversation
This reverts commit 198cec7.
β¦ is done (sigh!)
Bundle size differenceComparing this PR to
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds a new "Inbound Shipments" report that displays received inbound shipment lines with filtering by supplier and date range. The report is cloned from the existing outbound shipments report with key modifications to filter by RECEIVED/VERIFIED status only and temporarily set the sub-context to "Other".
Key changes:
- New inbound shipments report template with cost and shipment details
- GraphQL query filtering by RECEIVED/VERIFIED status to exclude in-progress shipments
- Data conversion logic to calculate totals grouped by supplier and item
- Added English locale message explaining the report behavior
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
standard_reports/inbound_shipments/latest/src/template.html |
HTML template for displaying inbound shipment data in a table format |
standard_reports/inbound_shipments/latest/src/style.css |
Styling for the report template with landscape A4 page layout |
standard_reports/inbound_shipments/latest/src/query.graphql |
GraphQL query to fetch inbound shipments filtered by RECEIVED/VERIFIED status |
standard_reports/inbound_shipments/latest/report-manifest.json |
Report metadata and configuration |
standard_reports/inbound_shipments/latest/convert_data_js/webpack.config.js |
Webpack configuration for building the data conversion module |
standard_reports/inbound_shipments/latest/convert_data_js/tsconfig.json |
TypeScript compiler configuration |
standard_reports/inbound_shipments/latest/convert_data_js/src/utils.ts |
Data processing logic to group and calculate totals for invoice lines |
standard_reports/inbound_shipments/latest/convert_data_js/src/types.ts |
TypeScript type definitions for input/output data structures |
standard_reports/inbound_shipments/latest/convert_data_js/src/convert_data.ts |
Entry point for data conversion function |
standard_reports/inbound_shipments/latest/convert_data_js/package.json |
Package dependencies and build script configuration |
standard_reports/inbound_shipments/latest/argument_schemas/arguments_ui.json |
UI schema for report filter controls (supplier, date range) |
standard_reports/inbound_shipments/latest/argument_schemas/arguments.json |
JSON schema for report argument validation |
client/packages/common/src/intl/locales/en/common.json |
Added help text explaining the inbound shipments report |
Not that's fine the translators will pick it up once it's merged and they've had time to look.
Yes, it's a manual step when modifying a report at this stage.
I agree, Shipped in particular would be good, as I think it would be handy to know what was on it's way, rather than what's currently being processed. Looks like right now shipped is excluded, not sure if that was intentional?
It sounds like there's some discussion on that other issue about what they should be. I think we can leave it as |
jmbrunskill
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Adrian.
I think the grouping approach is confusing, although I can see it is lifted from the Outbound Shipments one.
@mark-prins can you comment, is this valid feedback? Should we also fix the outbound shipments one if this is?
It took me a while to figure out what was going on, and that's with access to the code too. It seemed like it was just buggy at first but now I think I understand the intention...
Only the Total Cost gets grouped, but wouldn't it be more helpful to see Total Packs and Total Units for the shipments as well?
It's not clear what the Total cost relates to, as it's lumped in with the last row for that item + customer combination. Which makes it look like it relates to that batch some how.
I think a totals row, possibly as an optional parameter for each supplier/item combo would be more useful, then we could also do a line total price without extra confusion.
I think we should also visually distinguish this total cost column or calculation to make the distinction clearer.
A couple of asks:
- Clarify total cost grouping logic, or possibly remove it?
- Commit generated file
- Optional: Add tests
| @@ -0,0 +1,62 @@ | |||
| import { InvoiceConnector, Lines, Output } from "./types"; | |||
|
|
|||
| export const processInvoiceLines = ( | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like don't have any examples in standard reports but it would be nice start getting test cases setup for the standard reports.
Here's an example from the client repo...
https://github.com/msupply-foundation/open-msupply-reports/tree/main/clients/Niger/reception-de-vaccins/latest/convert_data_js/src
I think there might have been some challenges getting the tests to play nice with JS and TS. I have a feeling @roxy-dao fixed it though?
Are you interested in doing this @adrianwb ? Would help with setting the clarity on what the expected output should be...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, tests should be working? Are they broken again?
|
|
||
| invoices.nodes.forEach((invoice) => { | ||
| invoice.lines.nodes.forEach((line) => { | ||
| const groupKey = `${line.item.code}-${invoice.otherPartyName}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this group key might be meant to include batch number? I"m not clear on intended logic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You want total cost for the item regardless of batch, if you group by batch as well then it won't get the correct total cost price for the item. This was what was asked in the Outbound report, and makes sense to bring it over to the Inbound report too.
Fixes #9737
π©π»βπ» What does this PR do?
Cloned the existing outbound shipments report, with a few tweaks
π Any notes for the reviewer?
I've only added the new
messages.how-to-read-inbound-shipments"resource" to the Englishcommon.jsonlocale - do I also need to add it to the other locales or is translation handled elsewhere?Do I need to commit changes to the generated standard reports?
For consistency, the outbound shipments report should probably be filtered in the same way i.e. by Shipped/Delivered/Verified?
Note that the sub-context of this (and possibly outbound shipment report(s) with a sub-context of "Distribution" if we use a generic invoice/transaction sub-context for all) will need updated as part of #9736
π§ͺ Testing
π Documentation
π Reviewer Checklist
The PR Reviewer(s) should fill out this section before approving the PR
Breaking Changes
Issue Review
Tests Pass