-
Notifications
You must be signed in to change notification settings - Fork 10.7k
Add the Receipts Rendering Engine #43502
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
Conversation
…sses. The order receipt template file, order-receipt.php, is for now a dummy. It needs to be replaced with the real template. Also included: a small modification in Server.php that allows registering REST API controller classes living inside the src directory.
Test Results SummaryCommit SHA: 164b09f
To view the full API test report, click here. To view the full E2E test report, click here. To view all test reports, visit the WooCommerce Test Reports Dashboard. |
53afa71 to
0269ed8
Compare
Includes a call to the Stripe API to get the transaction and credit card information.
0269ed8 to
a4d41b4
Compare
33e7466 to
23ac2e4
Compare
- Use subtotal instead of total for product line items (so that discounts aren't included) - Include shipping taxes in the tax total
|
Hi @coreymckrill, 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: |
coreymckrill
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.
Looks good overall, and tests well (thanks for the very thorough testing instructions)! Left a few inline comments, plus here are a couple more general thoughts/questions:
- Since you can retrieve an existing file using the POST endpoint without the
force_newparameter, what is the use case of the GET endpoint? It looks like they both require the same permissions. - I kind of mentioned this inline too, but it seems like ideally the client shouldn't have to use the
expiration_dateparameter when generating a receipt under normal circumstances, because the default would work for most use cases. Is 1 day the right default here? - Something I just thought of related more generally to the Transient File Engine: should the directories the engine generates for storing the files have .htaccess and (empty) index.html files in them? We do that for the wc-logs directory, I think with the idea that those files prevent direct browsing and listing of the files in the directory...
plugins/woocommerce/src/Internal/ReceiptRendering/ReceiptRenderingEngine.php
Outdated
Show resolved
Hide resolved
plugins/woocommerce/src/Internal/ReceiptRendering/ReceiptRenderingEngine.php
Outdated
Show resolved
Hide resolved
plugins/woocommerce/src/Internal/ReceiptRendering/ReceiptRenderingEngine.php
Show resolved
Hide resolved
| 'line_items' => $line_items_info, | ||
| 'payment_method' => $order->get_payment_method_title(), | ||
| 'notes' => array_map( 'get_comment_text', $order->get_customer_order_notes() ), | ||
| 'payment_info' => $this->get_woo_pay_data( $order ), |
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.
Not sure this is the right spot in the code to do it, but it seems like we should have filter(s) that allow other payment systems to use this receipt functionality and add their data.
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.
The specification for this project was the support for Woo Payments specifically. We can safely extend/modify the code/add hooks in the future to support other payment gateways if needed (the code is Internal).
plugins/woocommerce/src/Internal/ReceiptRendering/Templates/order-receipt.php
Show resolved
Hide resolved
plugins/woocommerce/src/Internal/ReceiptRendering/Templates/order-receipt.php
Show resolved
Hide resolved
plugins/woocommerce/src/Internal/TransientFiles/TransientFilesEngine.php
Outdated
Show resolved
Hide resolved
plugins/woocommerce/src/Internal/TransientFiles/TransientFilesEngine.php
Show resolved
Hide resolved
plugins/woocommerce/src/Internal/ReceiptRendering/ReceiptRenderingRestController.php
Show resolved
Hide resolved
They aren't exactly the same. The POST endpoint will generate a new receipt, even without
That's a good point, I can add this as a migration in this pull request. |
- Remove duplicate controller registration in Server.php - Fix linting issues in receipt template - Add meta charset declaration in receipt template - Extract CSS fixed values to constants in ReceiptRenderingEngine
- If the directory already exists, these are created in a migration. - Otherwise, these will be created together with the directory when needed.
Right. But when would you ever use the GET endpoint if you can just use the POST and get the same result? Is there a scenario where you would want to check if a receipt file exists, but not generate it if it doesn't exist? |
coreymckrill
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.
👍 Looks great! I tried deleting the whole woocommerce_transient_files directory and then generating a new receipt, and the .htaccess and index.html files were successfully created along with the directory.
Per discussion elsewhere, I'm approving this but not merging.
|
Looks like there is one stuck CI check, "Evaluate e2e test results". I'm going to try closing this and reopening to see if that fixes is. |
That endpoint is added for consistency: it would be weird to have an entity that can be created via API but doesn't have a "non-destructive" (or "non-constructive" in this case) way to get it or verify if it exists (we have for everything else: orders, products, coupons...). The overhead in code (and development time) for the extra endpoint is negligible. |
Changes proposed in this Pull Request:
This pull request builds an engine for generating printable order receipts based on the transient files engine: the generated receipts will be transient files with a public unauthenticated URL. The design of the receipts mimicks the one for the receipts shown by the WooCommerce mobile applications.
More specifically, this pull request adds:
ReceiptRenderingEngineclass with two methods:generate_receiptandget_existing_receipt. These are wrappers aroundTransientFilesEngine.ReceiptRenderingRestControllerclass that provides two REST API endpoints, oneGETand onePOST, both having the URL/orders/<order id>/receipt. The difference is that thePOSTendpoint will create a new receipt if one doesn't exist already, while theGETendpoint won't. (A receipt exists if it has been created via either direct call toReceiptRenderingEngine::generate_receiptor thePOSTendpoint, and has not expired yet)In order to match existing receipts with orders, a new order meta key is introduced,
_receipt_file_name, whose value is the name of the last receipt (transient) file created for the order. Note that the file pointed by this meta value might not exist anymore (the transient files cleanup procedure for expired files doesn't delete associated order meta keys), the engine takes this in account when searching existing receipts.The
POSTendpoint accepts three optional query string arguments:expiration_date: Formatted as yyyy-mm-dd.expiration_days: A number, 0 is today, 1 is tomorrow, etc.force_new: Defaults to false, if true, creates a new receipt even if one already exists for the order.If neither
expiration_datenorexpiration_daysare supplied, the default is expiration_days = 1 (so the receipt expires the following day). Remember that the expiration time for transient files is 23:59:59 of the expiration date.This pull request also introduces a a bit of additional infrastructure for the REST API in general:
Server.phpto allow creating REST API controller classes in thesrcdirectory (the dependency injection engine is used to locate them).RestApiControllerBaseclass (insrc/Internal) that should be used as the base class for new REST API controller classes. TheReceiptRenderingRestControllerclass inherits from it.This extra infrastructure is added to support our policy of not adding any new code in
includeswhenever possible.Additionally, this pull request introduces an addition to the original transient files engine: an
.htaccessfile with contentdeny for alland and emptyindex.htmlfile will be created in the default directory for transient files (in a migration if the directory already exists, at the moment it's created if not) in order to prevent listing the directory contents.How to test the changes in this Pull Request:
To test the engine in full, an order that has the following is required:
Additionally, the order should be paid using a credit card with WooCommerce Payments.
Since testing with WooCommerce Payments isn't trivial (it's not possible in a local environment), a mechanism is added to ease testing. If the
WCPAY_DEV_MODEconstant exists with a value oftrueand the order has a meta entry with the key_wcpay_payment_details, the value of this meta entry (after being JSON decoded) will be used as if it was the "payment_details" key in the data returned by the Stripe "get intent" API endpoint (containing at least the "card_present" key), which is what the engine normally does in order to retrieve the payment information for the order (taking the intent id from the_intent_idorder meta item).Thus, if you have successfully created an order in a non-local site and have paid it with WooCommerce Payments, you can get the intent information from the order as follows:
wp eval 'echo json_encode(WC_Payments::get_payments_api_client()->get_intent(wc_get_order(<order_id>)->get_meta("_intent_id")));'...and then save the value in an
_intent_datameta entry for an existing order locally (remember to defineWCPAY_DEV_MODEtoo if you don't have WooCommerce Payments installed locally, or you don't have it in dev mode).Alternatively, you can create a meta entry with artificial intent data from a
wp shellas follows (the intent data is simplified to contain only the information required for the receipt):Once the order is in place, you can test the code API as follows:
wp eval 'echo wc_get_container()->get(Automattic\WooCommerce\Internal\ReceiptRendering\ReceiptRenderingEngine::class)->generate_receipt(<order id>, null, true);'You will get a file name, which you can translate to a full path with:
wp eval 'echo wc_get_container()->get(Automattic\WooCommerce\Internal\TransientFiles\TransientFilesEngin e::class)->get_transient_file_path("<file name>");'Verify that the file actually exists with
ls <full file path>; or directly see the file contents withcat <full file path>, or by opening it in a text editor.You can also get the public URL of the file with:
wp eval 'echo wc_get_container()->get(Automattic\WooCommerce\Internal\TransientFiles\TransientFilesEngin e::class)->get_public_url("<file name>");'Then open the URL in a browser to see the receipt.
The
nullpassed togenerate_receiptmeans "default expiration" (so the following day), try passing a date in yyyy-mm-dd format instead. Thetruepassed after that means "force the generation of a new receipt", try to pass it asfalseand you'll geat the same file name with every call after the first one.Now, as for the REST API endpoints, you can use PostMan or curl (assuming you have a basic authentication plugin in place):
curl -X POST -u <user>:<password> http://localhost:8034/wp-json/wc/v3/orders/<order id>/receipt?force_new=trueYou should be able to paste the value of
receipt_urlfrom the received JSON in the address bar of your browser, and then you'll see the receipt.As with the case of the code API, you can try setting
force_newtofalse, or addingexpiration_dateorexpiration_daysto the query string.Also, try with the GET endpoint and verify that you get a 404 error if no receipt exist for the order. Remember that "receipt exists for an order" means that a
_receipt_file_namemeta entry exists for the order and the transient file pointed by the entry exists.Changelog entry
Significance
Type
Message
Comment