Skip to content
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 the Receipts Rendering Engine #43502

Merged
merged 18 commits into from Feb 19, 2024
Merged

Conversation

Konamiman
Copy link
Contributor

@Konamiman Konamiman commented Jan 11, 2024

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:

  • A ReceiptRenderingEngine class with two methods: generate_receipt and get_existing_receipt. These are wrappers around TransientFilesEngine.
  • A ReceiptRenderingRestController class that provides two REST API endpoints, one GET and one POST, both having the URL /orders/<order id>/receipt. The difference is that the POST endpoint will create a new receipt if one doesn't exist already, while the GET endpoint won't. (A receipt exists if it has been created via either direct call to ReceiptRenderingEngine::generate_receipt or the POST endpoint, 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 POST endpoint 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_date nor expiration_days are 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:

  • A modification in Server.php to allow creating REST API controller classes in the src directory (the dependency injection engine is used to locate them).
  • A RestApiControllerBase class (in src/Internal) that should be used as the base class for new REST API controller classes. The ReceiptRenderingRestController class inherits from it.

This extra infrastructure is added to support our policy of not adding any new code in includes whenever possible.

Additionally, this pull request introduces an addition to the original transient files engine: an .htaccess file with content deny for all and and empty index.html file 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:

  • At least one simple and one variable product.
  • At least two discount coupons applied.
  • Fees.
  • Shipping costs.
  • Taxes.
  • Customer order notes.

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_MODE constant exists with a value of true and 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_id order 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_data meta entry for an existing order locally (remember to define WCPAY_DEV_MODE too 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 shell as follows (the intent data is simplified to contain only the information required for the receipt):

$o=wc_get_order(<order id>);
$o->update_meta_data('_wcpay_payment_details', '{"card_present":{"brand":"visa","last4":"1234","receipt":{"account_type":"credit","application_preferred_name":"Stripe Credit","dedicated_file_name":"A000000003101001"}}}');
$o->save_meta_data();

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 with cat <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 null passed to generate_receipt means "default expiration" (so the following day), try passing a date in yyyy-mm-dd format instead. The true passed after that means "force the generation of a new receipt", try to pass it as false and 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=true

You should be able to paste the value of receipt_url from 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_new to false, or adding expiration_date or expiration_days to 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_name meta entry exists for the order and the transient file pointed by the entry exists.

Changelog entry

  • Automatically create a changelog entry from the details below.

Significance

  • Patch
  • Minor
  • Major

Type

  • Fix - Fixes an existing bug
  • Add - Adds functionality
  • Update - Update existing functionality
  • Dev - Development related task
  • Tweak - A minor adjustment to the codebase
  • Performance - Address performance issues
  • Enhancement - Improvement to existing functionality

Message

Comment

…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.
@Konamiman Konamiman self-assigned this Jan 11, 2024
@github-actions github-actions bot added the plugin: woocommerce Issues related to the WooCommerce Core plugin. label Jan 11, 2024
Copy link
Contributor

github-actions bot commented Jan 11, 2024

Test Results Summary

Commit SHA: 164b09f

Test 🧪Passed ✅Failed 🚨Broken 🚧Skipped ⏭️Unknown ❔Total 📊Duration ⏱️
API Tests25900202610m 37s
E2E Tests301001303146m 58s

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.

Includes a call to the Stripe API to get the transaction and
credit card information.
@Konamiman Konamiman marked this pull request as ready for review January 19, 2024 11:37
- Use subtotal instead of total for product line items
  (so that discounts aren't included)
- Include shipping taxes in the tax total
@Konamiman Konamiman requested review from a team and coreymckrill and removed request for a team February 2, 2024 16:20
Copy link
Contributor

github-actions bot commented Feb 2, 2024

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:
https://github.com/woocommerce/woocommerce/wiki/Writing-high-quality-testing-instructions

Copy link
Collaborator

@coreymckrill coreymckrill left a 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_new parameter, 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_date parameter 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...

'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 ),
Copy link
Collaborator

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.

Copy link
Contributor Author

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).

@Konamiman
Copy link
Contributor Author

Since you can retrieve an existing file using the POST endpoint without the force_new parameter, what is the use case of the GET endpoint? It looks like they both require the same permissions.

They aren't exactly the same. The POST endpoint will generate a new receipt, even without force_new, if one doesn't exist already. The GET endpoint will never generate a new receipt. This is consistent with the intended semantics of the verbs.

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...

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.
@coreymckrill
Copy link
Collaborator

They aren't exactly the same. The POST endpoint will generate a new receipt, even without force_new, if one doesn't exist already. The GET endpoint will never generate a new receipt. This is consistent with the intended semantics of the verbs.

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?

Copy link
Collaborator

@coreymckrill coreymckrill left a 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.

@coreymckrill
Copy link
Collaborator

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.

@coreymckrill coreymckrill reopened this Feb 8, 2024
@Konamiman
Copy link
Contributor Author

But when would you ever use the GET endpoint if you can just use the POST and get the same result?

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.

@Konamiman Konamiman merged commit 89c6fbb into trunk Feb 19, 2024
79 checks passed
@Konamiman Konamiman deleted the add-receipt-rendering-engine branch February 19, 2024 11:03
@Konamiman Konamiman added this to the 8.7.0 milestone Feb 19, 2024
@github-actions github-actions bot added the needs: analysis Indicates if the PR requires a PR testing scrub session. label Feb 19, 2024
@Stojdza Stojdza added focus: rest api Issues related to WooCommerce REST API. needs: internal testing Indicates if the PR requires further testing conducted by Solaris status: analysis complete Indicates if a PR has been analysed by Solaris and removed needs: analysis Indicates if the PR requires a PR testing scrub session. labels Feb 20, 2024
@nigeljamesstevenson nigeljamesstevenson removed the focus: rest api Issues related to WooCommerce REST API. label Feb 20, 2024
@Stojdza Stojdza added the contains: rest api change Indicates if the PR contains a REST API change. label Feb 20, 2024
@nigeljamesstevenson nigeljamesstevenson added the release: highlight Issues that have a high user impact and need to be discussed/paid attention to. label Feb 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contains: rest api change Indicates if the PR contains a REST API change. needs: internal testing Indicates if the PR requires further testing conducted by Solaris plugin: woocommerce Issues related to the WooCommerce Core plugin. release: highlight Issues that have a high user impact and need to be discussed/paid attention to. status: analysis complete Indicates if a PR has been analysed by Solaris
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants