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 cash tracking session event targets for POS Compliance #2567

Open
wants to merge 2 commits into
base: unstable
Choose a base branch
from

Conversation

fatbattk
Copy link
Contributor

Background

Part of, https://github.com/Shopify/pos-next-react-native/issues/51030

Solution

  • Rename event-initiated extension targets per https://github.com/Shopify/pos-next-react-native/issues/51181
  • Add 3 cash tracking session targets for POS Compliance.
    • pos.cash-tracking-session-start.event.observe
    • pos.cash-tracking-session-cancel.event.observe
    • pos.cash-tracking-session-complete.event.observe
  • Added preliminary inputs for Cash session object.
  • Add to docs.

🎩

  • TODO: how?

Checklist

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

This comment has been minimized.

@fatbattk fatbattk force-pushed the add-pos-compliance-extension-targets branch from c2e0fcd to 793e51a Compare January 20, 2025 15:52
@fatbattk fatbattk requested a review from vctrchu January 20, 2025 15:53
@fatbattk fatbattk force-pushed the add-pos-compliance-extension-targets branch from 793e51a to bf42c5b Compare January 21, 2025 01:34
@fatbattk fatbattk requested a review from Alex-Palad January 21, 2025 01:34
@fatbattk fatbattk force-pushed the add-pos-compliance-extension-targets branch from bf42c5b to 31b4b2c Compare January 21, 2025 11:36
Copy link
Contributor

@vctrchu vctrchu left a comment

Choose a reason for hiding this comment

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

LGTM, the naming this aligns with the POS targets sheet. We can add more properties as you expand the scope

@fatbattk fatbattk force-pushed the add-pos-compliance-extension-targets branch 2 times, most recently from 4155b9b to d1e91ea Compare January 21, 2025 18:42
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.

Sorry for RC, PR looks good, just wanted to raise the alarm on a decision we're also making for the PurchaseCompleteInput as well

}

export interface CashTrackingSessionEndInput extends BaseInput {
id: number;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should move this into an object as such:

export interface CashTrackingSessionEndInput extends BaseInput {
   cashTrackingSessionEnd: {
      id
      /// other fields
   }
}

Copy link
Contributor

Choose a reason for hiding this comment

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

And actually, are these same fields going to be reuseable on other targets? If so maybe we can create a more generically named type, that we can reuse for CashTrackingSesisonEndInput, CashTrackingSessionStartInput, etc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nice, cleaner from BaseInput, updated!
these should be limited to cash tracking sessions, will abstract out if useful elsewhere.

@fatbattk fatbattk force-pushed the add-pos-compliance-extension-targets branch from d1e91ea to f89d0e1 Compare January 22, 2025 02:58
@fatbattk fatbattk requested a review from js-goupil January 22, 2025 03:02
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.

4 participants