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 pos.prepare-receipt.inject extension target #2597

Merged
merged 2 commits into from
Jan 29, 2025

Conversation

Alex-Palad
Copy link
Contributor

@Alex-Palad Alex-Palad commented Jan 27, 2025

Background

Part 1/3 of https://github.com/Shopify/retail-hq-store-management/issues/743
Resolves https://github.com/Shopify/retail-hq-store-management/issues/744

Solution

Use this doc to add new event-initiated extension target

  • Add pos.prepare-receipt.inject target for POS Compliance.
  • Add preliminary inputs and intents for PrepareReceipt object.
  • Add to docs.

🎩

  • ...

Checklist

  • I have 🎩'd these changes
  • I have updated relevant documentation

@Alex-Palad Alex-Palad requested a review from Copilot January 27, 2025 20:14
@Alex-Palad Alex-Palad marked this pull request as ready for review January 28, 2025 01:22
Comment on lines 12 to 15
payments: {
paymentMethod: PaymentMethod;
amount: number;
}[];
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@js-goupil This is something to align with checkout-complete

Comment on lines 3 to 10
export enum PrepareReceiptAdditionalLineType {
Header = 'Header',
Text = 'Text',
List = 'List',
// eslint-disable-next-line @shopify/typescript/prefer-pascal-case-enums
QRCode = 'QRCode',
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is existing ComplianceLines that we can parse. (Except we now use type to detect an error message

@Alex-Palad Alex-Palad force-pushed the compliance/pos.prepare-receipt.inject-target branch 2 times, most recently from 96ace32 to e465645 Compare January 28, 2025 02:21
import {TransactionCompleteInput} from './TransactionCompleteInput';

export interface PrepareReceiptInput {
prepareReceipt: TransactionCompleteInput['transactionComplete'];
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It uses the same input data as TransactionComplete
We still need separate interface to be flexible, and modify this interface without affecting CheckoutComplete

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we opt for a PrepareReceiptInput extends TransactionCompleteInput instead? Alternatively we can move the contents of TransactionCompleteInput into a separate interface called PosCheckout and both inputs can extend that and be empty for now

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactored

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

@js-goupil js-goupil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some comments

@@ -0,0 +1,19 @@
import {BaseIntent} from './BaseIntent';

export enum PrepareReceiptAdditionalLineType {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

enums have caused us a lot of headaches with typescript when exporting/importing on the POS side. Can we switch to a type instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replaced

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Alex-Palad Alex-Palad requested a review from js-goupil January 28, 2025 19:04
| 'List'
| 'QRCode'
| 'Error';
export interface PrepareReceiptAdditionalLine {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a line above here please

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also can we add some inline documentation here? Or was this going to be a follow up task? Remember that the in line docs automatically get compiled into the developer docs, so it's super helpful

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added some inline doc with examples, but we will need a dedicated page with pictures to show how different lines look
62f2ea6

@vctrchu vctrchu requested a review from NathanJolly January 29, 2025 05:14
@@ -61,6 +61,21 @@ Refer to the [migration guide](/docs/api/pos-ui-extensions/migrating) for more i
- Removed in POS version: N/A
- Release day: 1/6/2025
### Features
- Added support for the ${TargetLink.PosPrepareReceiptInject} target.
Copy link
Contributor

@vctrchu vctrchu Jan 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been out of the loop for dates but shouldn't this be under the 2025.04 release? Also this addition is duplicated, we have two 2025.01 sections in the version doc now

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, thank you
Fixed

@Alex-Palad Alex-Palad force-pushed the compliance/pos.prepare-receipt.inject-target branch from b3c23ad to a677b87 Compare January 29, 2025 16:31
@Alex-Palad Alex-Palad requested a review from vctrchu January 29, 2025 16:32
@Alex-Palad
Copy link
Contributor Author

/shipit

@Alex-Palad Alex-Palad merged commit 08ea78a into unstable Jan 29, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants