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

POS: Receipt Printer #1964

Merged
merged 10 commits into from
Feb 23, 2024
Merged

POS: Receipt Printer #1964

merged 10 commits into from
Feb 23, 2024

Conversation

Talej
Copy link
Contributor

@Talej Talej commented Jan 28, 2024

Description

Receipt printing functionality for android. Ended up using react-native-print package with a HTML template for receipt layout. The layout should pretty closely match the details shown on the 'Order' view for a paid order.

A print receipt button is shown on the payment completed view and also on the order view for paid orders if:

  1. It's an android device
  2. invoice has been paid
  3. receipt printing isn't disabled in settings

react-native-print doesn't have any way of detecting if there is a printer ahead of calling the print dialog so to handle this I added the disable printer option under the POS settings.

I thought it could possibly also be useful to allow the receipt template HTML to be configuration in the POS settings - although given it is HTML this could be somewhat painful the edit on a device so wanted your input on this...

This pull request is categorized as a:

  • New feature
  • Bug fix
  • Code refactor
  • Configuration change
  • Locales update
  • Quality assurance
  • Other

Checklist

  • I’ve run yarn run tsc and made sure my code compiles correctly
  • I’ve run yarn run lint and made sure my code didn’t contain any problematic patterns
  • I’ve run yarn run prettier and made sure my code is formatted correctly
  • I’ve run yarn run test and made sure all of the tests pass

Testing

If you modified or added a utility file, did you add new unit tests?

  • No, I’m a fool
  • Yes
  • N/A

I have tested this PR on the following platforms (please specify OS version and phone model/VM):

  • Android
  • iOS

I have tested this PR with the following types of nodes (please specify node version and API version where appropriate):

  • Embedded LND
  • LND (REST)
  • LND (Lightning Node Connect)
  • Core Lightning (c-lightning-REST)
  • LndHub
  • [DEPRECATED] Core Lightning (Spark)
  • [DEPRECATED] Eclair

Locales

  • I’ve added new locale text that requires translations
  • I’m aware that new translations should be made on the ZEUS Transfix page and not directly to this repo

Third Party Dependencies and Packages

  • Contributors will need to run yarn after this PR is merged in
  • 3rd party dependencies have been modified:
    • verify that package.json and yarn.lock have been properly updated
    • verify that dependencies are installed for both iOS and Android platforms

Other:

  • Changes were made that require an update to the README
  • Changes were made that require an update to onboarding

@Talej
Copy link
Contributor Author

Talej commented Jan 28, 2024

Screenshot_20240127-135755
Screenshot_20240127-135917
Screenshot_20240127-135946

@kaloudis kaloudis changed the title Receipt Printer POS: Receipt Printer Jan 28, 2024
@kaloudis kaloudis added this to the v0.8.2 milestone Jan 28, 2024
@kaloudis
Copy link
Contributor

I wonder if it's worth distinguishing Print Invoice / Print Receipt depending on where we are in the flow

@Talej
Copy link
Contributor Author

Talej commented Jan 30, 2024

I wonder if it's worth distinguishing Print Invoice / Print Receipt depending on where we are in the flow

Do you mean have a Print Invoice on the order screen before it is paid and Print Receipt after paid?

@kaloudis
Copy link
Contributor

I wonder if it's worth distinguishing Print Invoice / Print Receipt depending on where we are in the flow

Do you mean have a Print Invoice on the order screen before it is paid and Print Receipt after paid?

Precisely, and perhaps we add the payment pre-image on the receipt to distinguish them and act as a bit of payment proof

@kaloudis
Copy link
Contributor

Another nit: the header. Instead of Tax Receipt maybe we leverage the store name (not sure if it's currently exposed in the Standalone POS config - if not would be nice to leverage like we do for Square and add to the invoice memo as well as the receipt title here)

@Talej
Copy link
Contributor Author

Talej commented Feb 5, 2024

  • Added merchant name setting for standalone POS
  • Added merchant name in print title, falls back on Tax Receipt / Tax Invoice
  • Added Print Invoice option on un-paid orders
  • Added preimage to paid receipt
  • Fixed issue with missing payment details

package.json Outdated
@@ -111,6 +111,7 @@
"react-native-notifications": "5.1.0",
"react-native-os": "aprock/react-native-os#5/head",
"react-native-permissions": "3.8.0",
"react-native-print": "^0.11.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

let's lock in the version number here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

views/Order.tsx Outdated
? merchantName
: isPaid
? 'Tax Receipt'
: 'Invoice';
Copy link
Contributor

Choose a reason for hiding this comment

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

We need these titles localized

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

@kaloudis
Copy link
Contributor

kaloudis commented Feb 9, 2024

Nice job only showing the printer setting for Android, but I think it would be better to make it opt in rather than opt out as most merchants likely won't have a printer module.

@Talej
Copy link
Contributor Author

Talej commented Feb 10, 2024

Nice job only showing the printer setting for Android, but I think it would be better to make it opt in rather than opt out as most merchants likely won't have a printer module.

Makes total sense. Flipped the setting from disable to enable.

locales/en.json Outdated
"pos.views.Settings.PointOfSale.authWarning": "Warning: no password or PIN set",
"pos.views.Settings.PointOfSale.backendWarning": "Warning: currently only LND nodes are able to mark orders as paid",
"pos.views.Settings.PointOfSale.currencyError": "Error: currency must be set first",
"pos.print.taxReceipt": "Tax Receipt",
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: can we just make this Receipt instead of Tax Receipt

@kaloudis
Copy link
Contributor

@Talej can you provide an example of what these receipts look like when the merchant provides their store name in settings?

@Talej
Copy link
Contributor Author

Talej commented Feb 15, 2024

@Talej can you provide an example of what these receipts look like when the merchant provides their store name in settings?

proof-of-storename

IMG_1147

Copy link
Contributor

@shubhamkmr04 shubhamkmr04 left a comment

Choose a reason for hiding this comment

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

Screenshot_1708081156
Screenshot_1708081167

It would be better if we give a paddingBottom here.

Copy link
Contributor

@shubhamkmr04 shubhamkmr04 left a comment

Choose a reason for hiding this comment

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

Screenshot_1708081031
Also can we also handle the case so the user don't get this error if the device is not connected to a printer.

@Talej
Copy link
Contributor Author

Talej commented Feb 19, 2024

Screenshot_1708081031 Also can we also handle the case so the user don't get this error if the device is not connected to a printer.

This actually looks like the package isn't loading for you rather than anything to do with not connecting to a printer.

I have tried with various devices and even if there is no printer it still works without error (it gives an option to export as PDF). I have added in some extra checks to prevent the error but would be useful if you could debug a bit further to see why it wasn't loading.

@shubhamkmr04
Copy link
Contributor

This actually looks like the package isn't loading for you rather than anything to do with not connecting to a printer.

I have tried with various devices and even if there is no printer it still works without error (it gives an option to export as PDF). I have added in some extra checks to prevent the error but would be useful if you could debug a bit further to see why it wasn't loading.

Ah, got it! I managed to pinpoint the issue on my end, and now I'm getting the option to download PDFs. Thanks!

@@ -116,7 +116,8 @@ export default class OrderView extends React.Component<OrderProps, OrderState> {
const { changeUnits, units } = UnitsStore;
const fiat = settings.fiat;
const disableTips: boolean = settings?.pos?.disableTips || false;
const enablePrinter: boolean = settings?.pos?.enablePrinter || false;
const enablePrinter: boolean =
(settings?.pos?.enablePrinter && RNPrint) || false;
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess we can remove this condition now, so the user can see the PRINT button even if the printer is not connected and use it to download the receipt.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

possibly can't hurt to leave it in there just in case

Copy link
Contributor

@shubhamkmr04 shubhamkmr04 left a comment

Choose a reason for hiding this comment

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

tACK

Copy link
Contributor

@kaloudis kaloudis left a comment

Choose a reason for hiding this comment

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

LGTM. Great job, @Talej!

@kaloudis kaloudis merged commit a24349e into ZeusLN:master Feb 23, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants