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
Fixes #38757 - pass time values on CSV importer #39372
Fixes #38757 - pass time values on CSV importer #39372
Conversation
Validate datestrings and convert Unix timestamps to datestrings with UTC timezone specified.
Hi , Apart from reviewing the code changes, please make sure to review the testing instructions as well. You can follow this guide to find out what good testing instructions should look like: |
@AndrewJDawes Thanks again. When we ask contributors to make changes to a PR, we are not asking for a new PR. We would like to see the changes made to the PR being reviewed. Having the discussion of change all located on the same PR is useful to us when the change is X years old and we need to refresh on why the change was made. Secondly, people test every PR that makes functional changes so we need testing instructions on those PRs. In this case, we'll need to ensure the a wider range of date formats work in multiple regional settings so I'll research that a bit and add the testing instructions. |
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.
@AndrewJDawes Thanks for the help. This looks and tested great. I'll need to add a changelog then let it re-run through CI once the one suggestion is addressed before approving.
// If the value is a valid date string, return as is. | ||
return $value; | ||
} | ||
// If value is not valid Unix timestamp or date string, return null. |
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 you move this return to after the try/catch block so the funtcion always falls back to explicitly returning a null
?
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.
There is also one code sniff issue that needs to be addressed.
Thanks for explaining this to me, @rrennick ! Appreciate your help on this - this has helped me learn a good deal about how to contribute. I implemented your feedback on this same PR. Would you please let me know if there is anything else I should do before this can be merged? |
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.
Would you please let me know if there is anything else I should do before this can be merged?
This looks great @AndrewJDawes . Thanks for the help.
132aba1
into
woocommerce:trunk
Validate datestrings and convert Unix timestamps to datestrings with UTC timezone specified.
Submission Review Guidelines:
Changes proposed in this Pull Request:
Add time support to
date on sale from
anddate on sale to
to product CSV import.Closes #38757 .
How to test the changes in this Pull Request:
Using the WooCommerce Testing Instructions Guide, include your detailed testing instructions:
trunk
wp shell
5- Product should be on sale as follows
2023-08-01 00:00
to2023-08-02 00:00
2023-08-01 00:00
to2023-08-01 00:00
6- Switch to this branch
7- Re-import the CSV with the modify existing checked
8- Repeat step 4. Product should be on sale as follows
2023-08-01 09:00
to2023-08-02 21:00
2023-08-01 09:00
to2023-08-01 21:00
2023-08-01 09:00
to2023-08-01 21:00
strtotime()
aboveChangelog entry
Significance
Type
Message Allow passing time values on product CSV importer.
Comment