CatimaLoyalty / Android

Catima, a Loyalty Card & Ticket Manager for Android
https://catima.app
GNU General Public License v3.0
849 stars 152 forks source link

Bug: import loops infinitely if last import failed #661

Closed Altonss closed 2 years ago

Altonss commented 2 years ago

I tried to import a manually created file that respects the vauchervault format but with a code_93 barcode, which isn't supported by vauchervault and which I have not added in the importer. But the import stays stuck in an infinite loop trying to import the json file! This behavior is not very "clean" as we neither know the future export formats of vauchervault, nor if the file the user wants to import has not been "corrupted" with a code 93 barcode ^^ The best fix should just be to raise an error if a certain barcode is not recognised by the import function.

TheLastProject commented 2 years ago

That is what it's supposed to do though: https://github.com/TheLastProject/Catima/blob/186c2d1d56787339519e62d0543d6cbc7806e27c/app/src/main/java/protect/card_locker/importexport/VoucherVaultImporter.java#L99-L100

Please supply a Voucher Vault json file that this bug can be triggered with.

Altonss commented 2 years ago
[
  {
    "uuid": "faecb247-b99d-466b-83e3-cb1f38af1f3c",
    "description": "Smartfuel",
    "code": "12345",
    "codeType": "CODE128",
    "expires": null,
    "removeOnceExpired": true,
    "balance": null,
    "color": "ORANGE"
  },
  {
    "uuid": "3a7362d9-ef19-4338-9e38-bad00beaf9d6",
    "description": "Subway",
    "code": "12345",
    "codeType": "QR",
    "expires": null,
    "removeOnceExpired": true,
    "balance": null,
    "color": "YELLOW"
  },
  {
    "uuid": "faecb247-b99d-466b-83e3-cb1f38af1f3d",
    "description": "Smartfuel",
    "code": "12345",
    "codeType": "CODE93",
    "expires": null,
    "removeOnceExpired": true,
    "balance": null,
    "color": "ORANGE"
  },
  {
    "uuid": "faecb247-b99d-466b-83e3-cb1f38af1f3e",
    "description": "Smartfuel",
    "code": "12345",
    "codeType": "CODE39",
    "expires": null,
    "removeOnceExpired": true,
    "balance": null,
    "color": "ORANGE"
  }
]
TheLastProject commented 2 years ago

The issue seems to be that after a failed import, the next import will get stuck in an infinite loop. So there is some issue with handling a failed import.

Altonss commented 2 years ago

I did some more testing: the infinite loop happens after the second import. The first time it aborts correctly and if I try it a second time it loops

Altonss commented 2 years ago

With the fix of #678, I don't get this bug anymore.

Altonss commented 2 years ago

I thought about a possible enhancement related to this issue. Instead of just quitting the whole import, we could import the cards that are "compatible"/"readable" by the import script and just don't import the once that fails. After the import a popup could show up saying: "1 card could not be imported because of a formatting error".

TheLastProject commented 2 years ago

I thought about a possible enhancement related to this issue. Instead of just quitting the whole import, we could import the cards that are "compatible"/"readable" by the import script and just don't import the once that fails. After the import a popup could show up saying: "1 card could not be imported because of a formatting error".

Can you please turn this into a new GitHub issue? Then we can focus here on fixing this bug and improve the whole import process further later :)

Altonss commented 2 years ago

Can you please turn this into a new GitHub issue? Then we can focus here on fixing this bug and improve the whole import process further later :)

Of course, this was just a side-thought xD