-
-
Notifications
You must be signed in to change notification settings - Fork 858
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
Add Expiry Date on Receive Line Item #8867
Add Expiry Date on Receive Line Item #8867
Conversation
✅ Deploy Preview for inventree-web-pui-preview ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
weird - my pre-commit checks passed locally, but fail in actions. Any suggestions? |
Thank you for implementing this @jacobfelknor! Regarding the pre-cmmit issue: not really sure what caused it - could also not get it to change anything on my setup. I simply reset that file for now to keep this PR going. Dependencies tend to be difficult in a lot of surprising ways. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #8867 +/- ##
==========================================
- Coverage 85.23% 84.99% -0.25%
==========================================
Files 1177 1177
Lines 51804 52221 +417
Branches 2094 2101 +7
==========================================
+ Hits 44156 44386 +230
- Misses 7119 7310 +191
+ Partials 529 525 -4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@jacobfelknor nice work, implementation looks very clean! FunctionalityTested it locally, seems to work very nicely indeed! Unit TestingI have made some suggestions to tweak two of the changed code lines which will fix the CI issues. An "empty" date must be Default ValuesWhen specifying the "expiry date" via the UI form - it would be useful to pull out the "default value" from the part information. This is stored in the Hidden if DisabledFor the UI side of things, please ensure that the UI elements are hidden if the |
Sounds good, I will work on these changes now |
const [expiryDateOpen, expiryDateHandlers] = useDisclosure(false, { | ||
onOpen: () => { | ||
// check the default part expiry. Assume expiry is relative to today | ||
const defaultExpiry = record.part_detail?.default_expiry; |
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 am assuming the number of days specified in default_expiry
should be relative to the receive date.
I'm not well-versed in formatting javascript dates... but coming from Python this doesn't feel great. Open to suggestion here if there is a better way
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.
Could it be as simple as adding N days to the current date, right?
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... that is what I'm doing... but the formatting is what gets so verbose (lines 311-314). Does the form require me to format it in "YYYY-MM-DD"
?
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.
We already have dayjs
dependency in the frontend code, so you could do something like:
// Generate a date 10 days in the future
const futureDate = dayjs().add(10, 'day').format('YYYY-MM-DD');
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 like that much better. Thanks for the tip. Updated
@jacobfelknor thanks for the great work here :) |
Addresses #6176
Add option in PUI receive line item form for adding an expiry.
Update test
PurchaseOrderReceive.test_valid
to include anexpiry_date