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

QR code in invoice #845

Open
wants to merge 5 commits into
base: stable
Choose a base branch
from

Conversation

christopherlam
Copy link
Contributor

after $sudo apt install qrencode:

#include <qrencode.h> does not seem to work:

/usr/bin/ld: lib/gnucash/libgnc-gnome-utils.so: undefined reference to `QRcode_encodeString'
/usr/bin/ld: lib/gnucash/libgnc-gnome-utils.so: undefined reference to `QRcode_free'

@jralls
Copy link
Member

jralls commented Dec 24, 2020

That would be because you didn't tell the linker where to find the symbol. You do that by adding libqrencode to gnc-gnome-utils's target_link_libraries command in CMakeLists.txt.

You also need to check for qrencode being available in the root CMakeLists.txt, update Readme.dependencies in the root directory and add module installation in gnucash-on-windows, gnucash-on-osx, and gnucash-on-flatpak.

@fellen
Copy link
Member

fellen commented Dec 24, 2020

You also need to check for qrencode being available in the root CMakeLists.txt, update Readme.dependencies in the root directory and add module installation in gnucash-on-windows, gnucash-on-osx, and gnucash-on-flatpak.

It seems to be in most important distributions: https://repology.org/project/qrencode/versions.
As changing dependencies in maint is not recommend, you could also consider to make it optional.

@christopherlam christopherlam force-pushed the maint-qrcode branch 2 times, most recently from bc67158 to f1bbc5e Compare January 3, 2021 14:22
@christopherlam
Copy link
Contributor Author

This requires gcc -lqrencode - how to add linker flag?

@christopherlam
Copy link
Contributor Author

Will it be:

if (FOUND_QRENCODE)
  set (CMAKE_C_FLAGS "-lqrencode ${CMAKE_C_FLAGS}")
endif ()

@jralls
Copy link
Member

jralls commented Jan 4, 2021

No, you add it to the list in the target's target_link_libraries call. Since qrencode is also built with CMake you shouldn't need the -l, you should be able to use qrencode.

@christopherlam christopherlam changed the title Maint qrcode QR code in invoice Jan 5, 2021
@christopherlam
Copy link
Contributor Author

christopherlam commented Jan 5, 2021

This branch has the C glue code to call libqrencode
It also has guile code to interpret list of booleans to SVG.

Missing:

  • appropriate CMake flags to link qrencode when compiling gnc-menu-extensions.c -- we'd eventually use a separate file
  • template for string to incorporate information (eg invoice amounts, bank/routing numbers etc) to send to libqrencode and receive data

Example
image

@christopherlam christopherlam force-pushed the maint-qrcode branch 6 times, most recently from 44a2029 to 2313abe Compare January 9, 2021 04:18
@christopherlam
Copy link
Contributor Author

@gjanssens this is now functional.
See the string-template is hard-coded.
The EPC QR code https://en.wikipedia.org/wiki/EPC_QR_code standard requires IBAN which is not currently a gnucash option.
My thought: introduce IBAN book company option, and introduce company QR template with templated language eg {IBAN} {CURRENCY_ISO} {TOTAL} fields as another company option.
Any other ideas?

@gjanssens
Copy link
Member

This will become a nice addition to our invoice printing functionality.

@gjanssens this is now functional.
See the string-template is hard-coded.
The EPC QR code https://en.wikipedia.org/wiki/EPC_QR_code standard requires IBAN which is not currently a gnucash option.
My thought: introduce IBAN book company option, and introduce company QR template with templated language eg {IBAN} {CURRENCY_ISO} {TOTAL} fields as another company option.
Any other ideas?

There are two bits of required information that are currently not stored: IBAN and BIC. I agree they can be part of the book company info. The currency should be invoice currency. We don't need to store that as a company parameter.

From the specification it seems to me the only free form fields in the QR template are the reference and information fields. All other fields have strictly defined content. Unless you are considering other formats which follow different specifications ? In that case we will probably want a templating system similar to the check printing one. GnuCash can provide a number of standard formats and allow the user to create custom ones. But I'd wait with that until the need arises.
For now it's a matter of deciding what information we want in the two "freeform" fields. For reference I'd propose to use invoice number. For information we could use invoice notes ? Feel free to consider options of course.

As for the technical side of this PR:
I agree with @fellen we should not add required dependencies on maint. The program should continue build and work without the QR functionality if the library is missing. That means you will need to conditionally enable or disable this feature based on the presence of the library. In theory you have build time or run time dependencies and one could make either optional. In practice however I think we can claim that if the program is built with qrencode it requires qrencode to run. So you should tweak our build system such that it tests for the presence of the qrencode package. If present, build with the qr code functionality enabled. If missing disable the qr code functionality and print a message about this. That give builders interested in the functionality a hint to how they can enable it.

A few hints:

  • the QREncode library exposes a package config file, so you can use pkg_check_modules (QRENCODE IMPORTED_TARGET qrencode) to test for the presence of the library (see pkg_check_modules This also sets a number of environment variables you can use later in the build system.
  • it will probably make your life easier if you move the C function that uses the QREncode library into a separate source file. If you do you can conditionally include that file as a source file in CMakeLists.txt based on the value of the generated variable QRENCODE_FOUND.
  • Similarly you can use the generated import target PkgConfig::QRENCODE to set the proper linker details in target_link_libraries.
  • It gets a bit nastier from there: you can only expose this function to scheme if it's defined of course, so in gnome-utils.i you'll have to check somehow if the function is available before wrapping it. I don't think we have code in place already to cater for such a check. A similar trick could be used here: use a separate interface file to expose the qrencode function, only generate that wrapper based on the value of QRENCODE_FOUND and only add the generated swig file as a source file based on the value of QRENCODE_FOUND.
  • Similarly, the option you have added to invoice.scm to present a qr code should only be enabled (or visible) if gnucash is built with qrcode support. If we manage to fix the gnome-utils.i file test above you could use that file to expose a variable to scheme code you can test on for the presence of the qr encode functionality. I suppose scheme can even test for the presence of a function directly. So you could test for the existence of guile function gnc-qrcode-encodestring directly and enable/disable or show/hide the option based on the result.

As you can see, quite a few bits to change in order to make this an optional feature. If that's too much, the alternative is to only introduce this in master. I am totally fine with adding a new mandatory dependency there.

Note also that regardless of your target branch you can only make this mandatory when our build systems for MacOS, Windows and Flatpak are updated to also install or build qrencode.

@gjanssens
Copy link
Member

And in addition our ci scripts need updates to install qrencode before building gnucash. If we go the optional route, I'd only install qrencode in a few of our build environments to check if test succeed both with or without qrencode.

@christopherlam christopherlam force-pushed the maint-qrcode branch 2 times, most recently from 47c4442 to 1743d83 Compare January 9, 2021 13:11
@christopherlam
Copy link
Contributor Author

christopherlam commented Jan 9, 2021

This will become a nice addition to our invoice printing functionality.

There are two bits of required information that are currently not stored: IBAN and BIC. I agree they can be part of the book company info. The currency should be invoice currency. We don't need to store that as a company parameter.

Ok yes two string options must be created. Note IBAN maximum 34 chars, BIC max 11 chars.

From the specification it seems to me the only free form fields in the QR template are the reference and information fields. All other fields have strictly defined content. Unless you are considering other formats which follow different specifications ? In that case we will probably want a templating system similar to the check printing one. GnuCash can provide a number of standard formats and allow the user to create custom ones. But I'd wait with that until the need arises.
For now it's a matter of deciding what information we want in the two "freeform" fields. For reference I'd propose to use invoice number. For information we could use invoice notes ? Feel free to consider options of course.

From my understanding it's reference or information. For reference invoice ID seems natural.

As for the technical side of this PR:
I agree with @fellen we should not add required dependencies on maint. The program should continue build and work without the QR functionality if the library is missing. That means you will need to conditionally enable or disable this feature based on the presence of the library. In theory you have build time or run time dependencies and one could make either optional. In practice however I think we can claim that if the program is built with qrencode it requires qrencode to run. So you should tweak our build system such that it tests for the presence of the qrencode package. If present, build with the qr code functionality enabled. If missing disable the qr code functionality and print a message about this. That give builders interested in the functionality a hint to how they can enable it.

A few hints:
(snip)

I'd suggest gnc-qrencode.c and gnc-qrencode-stub.c -- both expose gnc_qrcode_encodestring but the latter will always return SCM_BOOL_F

if (HAVE_QRENCODE)
  target_link_libraries (qrencode)
  add_library (qr-encode.c)
else ()
  add_library (qr-encode-stub.c)
endif ()

Similarly, the option you have added to invoice.scm to present a qr code should only be enabled (or visible) if gnucash is built with qrcode support. If we manage to fix the gnome-utils.i file test above you could use that file to expose a variable to scheme code you can test on for the presence of the qr encode functionality. I suppose scheme can even test for the presence of a function directly. So you could test for the existence of guile function gnc-qrcode-encodestring directly and enable/disable or show/hide the option based on the result.

For simplicity I'd vote to allow QR option in invoice.scm (otherwise saved-reports etc will complain), and if gnc-qrcode-encodestring does not return valid QR data then the header will have textual 'QRencode library not available' with output to tracefile.

@gjanssens
Copy link
Member

From the specification it seems to me the only free form fields in the QR template are the reference and information fields. All other fields have strictly defined content. Unless you are considering other formats which follow different specifications ? In that case we will probably want a templating system similar to the check printing one. GnuCash can provide a number of standard formats and allow the user to create custom ones. But I'd wait with that until the need arises.
For now it's a matter of deciding what information we want in the two "freeform" fields. For reference I'd propose to use invoice number. For information we could use invoice notes ? Feel free to consider options of course.

From my understanding it's reference or information. For reference invoice ID seems natural.

No, there are actually two supported reference types, structured or unstructured. And in addition you can have a free form information field. The structured reference follows the ISO 11649 format.

I'd suggest gnc-qrencode.c and gnc-qrencode-stub.c -- both expose gnc_qrcode_encodestring but the latter will always return SCM_BOOL_F

if (HAVE_QRENCODE)
  target_link_libraries (qrencode)
  add_library (qr-encode.c)
else ()
  add_library (qr-encode-stub.c)
endif ()

That's ok in principle. I assume you were using pseudocode here ?

Similarly, the option you have added to invoice.scm to present a qr code should only be enabled (or visible) if gnucash is built with qrcode support. If we manage to fix the gnome-utils.i file test above you could use that file to expose a variable to scheme code you can test on for the presence of the qr encode functionality. I suppose scheme can even test for the presence of a function directly. So you could test for the existence of guile function gnc-qrcode-encodestring directly and enable/disable or show/hide the option based on the result.

For simplicity I'd vote to allow QR option in invoice.scm (otherwise saved-reports etc will complain), and if gnc-qrcode-encodestring does not return valid QR data then the header will have textual 'QRencode library not available' with output to tracefile.

Is it technically difficult to hide the option when qrcode is not available ? I don't mean make it non-existent, just hidden. The other option is to keep it around but disable it when qrencode is not found. That way it still exists but the user can't set it. A tooltip would then be helpful to explain why the option is disabled. The main difference between your and my approach is that in your case the error message only appears after the user has configured the report, which leads to frustration. If you can't set the option in the first place that saves the user a roundtrip to the options if it was set incorrectly.

@christopherlam christopherlam force-pushed the maint-qrcode branch 3 times, most recently from 53dec16 to ccba1ea Compare January 10, 2021 13:43
@christopherlam
Copy link
Contributor Author

christopherlam commented Jan 10, 2021

Have moved into gnc-qrencode.c and gnc-encode-stub.c. Both of them import gnc-qrencode.h which defines the exported function. For simplicity I've changed the 'no libqrencode' return value to SCM_EOL.

The invoice report currently places QR code in invoice header, as a new multichoice option for date/company-details/customer-details/picture. Alternatively we could add a new option "QR code" which will obligatorily show in top-right corner, and whose multichoice options are 'None' 'EPC QR Code' for further expansion. Having a new option will be much easier to selectively hide when libqrencode is not available.--> Done

I can't figure out CMakeLists changes to load either qrencode package + gnc-qrencode.c or gnc-qrencode-stub.c. Assistance requested...

@christopherlam christopherlam force-pushed the maint-qrcode branch 2 times, most recently from d2a6610 to bd85ca7 Compare January 11, 2021 13:51
@fellen
Copy link
Member

fellen commented Jul 10, 2021

And in addition our ci scripts need updates to install qrencode before building gnucash. If we go the optional route, I'd only install qrencode in a few of our build environments to check if test succeed both with or without qrencode.

@gjanssens how about converting your tips from this PR into a wiki page Adding_Dependencies?

@jralls
Copy link
Member

jralls commented Jul 10, 2021

How about a one-line wiki page:

Adding Dependencies

Don't.

We already have too many dependencies and every added one makes GnuCash harder to build and more dependent on the whims of distribution packagers and in the case of single-contributor libraries like libqrencode the whims and survival of that one contributor. Note that it's been almost 10 months since the last commit to that repo.

@fellen
Copy link
Member

fellen commented Jul 10, 2021

Adding Dependencies

Don't.

I hoped it would scare off in most cases. ;-)
[Edit:]
And in this case IIRC some countries in the East of the EU are forcing the use QR codes on invoices.

@fellen
Copy link
Member

fellen commented Feb 13, 2022

https://lists.gnucash.org/pipermail/gnucash-de/2022-February/012321.html:
From 1. Oktober 2022 QR codes are also required in CH.

@christopherlam
Copy link
Contributor Author

Here's one of the QR contents from the PDF. They also mandate precise layout containing the QR code.

SPC
0200
1
CH4431999123000889012
S
Max Muster & Söhne
Musterstrasse
123
8000
Seldwyla
CH







2500.25
CHF
S
Simon Muster
Musterstrasse 
1
8000
Seldwyla
CH
QRR
210000000003139471430009017

EPD

Name AV1: UV;UltraPay005;12345
Name AV2: XY;XYService;54321

@fellen
Copy link
Member

fellen commented Feb 13, 2022

I assume you are talking about
https://www.paymentstandards.ch/dam/downloads/style-guide-en.pdf

They put the payment part below the invoice or on the next page. That is also common here in DE. The german form descriptions are in
https://die-dk.de/media/files/Richtlinie-ZV-Vordrucke-2016-DK_finale_Fassung_DK_Homepage.pdf with an update
https://die-dk.de/media/files/Merkblatt_2017-Anpassung_der_Richtlinien_V11.01.2018_QOulHUn.pdf
QR code is almost at the end of the main document described.

As feared the formats are very different.

@christopherlam
Copy link
Contributor Author

As feared the formats are very different.

Not a problem in this branch. The format it's extensible by modifying code, or even maybe in the future via options.

@fellen
Copy link
Member

fellen commented Feb 14, 2022

Not a problem in this branch. The format it's extensible by modifying code, or even maybe in the future via options.

Perhaps the printing of the payment part is easier integrated in the cheque printing facility. Most german and also other european bank forms are based on an A6 landscape form.

@luzpaz
Copy link
Contributor

luzpaz commented Sep 26, 2022

What's the status on this PR. This is a fantastic feature

@luzpaz
Copy link
Contributor

luzpaz commented Nov 14, 2022

bump

@jralls jralls changed the base branch from maint to master January 24, 2023 22:44
@jralls jralls marked this pull request as draft January 24, 2023 22:58
@jralls
Copy link
Member

jralls commented Jan 24, 2023

The options parts need to be rewritten in C++.
libqrencode appears to have been abandoned with the last commit 2020-09-27. We should consider taking the source into borrowed and including its test suite in our CI.

@christopherlam
Copy link
Contributor Author

QRcode_encodeString 4.11 has changed something and this isn't producing valid QR data anymore. Anyway rebased onto master, and using c++options. If libqrencode is added into borrowed then I'll try debug again.

@jralls jralls changed the base branch from master to stable March 27, 2023 18:25
@christopherlam christopherlam force-pushed the maint-qrcode branch 2 times, most recently from 14916e9 to 07225b3 Compare March 28, 2023 16:00
@christopherlam christopherlam marked this pull request as ready for review March 28, 2023 16:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants