Scanned barcode shoud set the card barcode, not the ID when they differ

[mvglasow]

By default, Catima assumes card ID and barcode to be the same. Scan the barcode, Catima will detect its format, read the code and store the barcode and card ID. So far, so good.

When the barcode and card ID differ, the workflow is as follows (as of version 2.25.1):

• Scan the barcode, or enter the ID manually and choose a barcode format. Your input will be used as the ID and barcode value.
• In the drop-down list for the barcode value, choose to set the barcode value manually.

Not sure how I’m expected proceed from there. Personally, I scan the barcode first, copy the ID, select to enter the barcode manually, then paste the scanned and copied ID into the barcode, then change the ID – but I find this workflow less than perfect. I would expect to scan the barcode and enter the ID manually (if it differs) – not the other way round.

Suggested improvement:

• Scanning a barcode (or entering it manually) will always set the barcode.
• When creating a new card, or changing the barcode for a card where the ID matches the barcode, scanning the barcode will also set the ID.
• After this step, the user can choose to manually enter an ID which differs from the barcode. This could be accomplished by changing the ID field into a drop-down one (similar to the barcode field now), where the user can select “Same as barcode” or “Enter manually”. The barcode field would then be a simple text field, disabled when the barcode type is set to “No barcode”.

This would save me a few steps when importing a new card with an ID which differs from the QR code.

[TheLastProject]

Yeah, looking at it more closely I agree the flow is the wrong way around and all actions should by default affect "Barcode value", not "Card ID".

I fear this might need a database migration to properly fix (as the cardId is always stored but the barcodeId may be NULL in the database), but your idea and suggested flow sounds correct to me.

[TheLastProject]

(Tagging with "severity: major" as it can make setting a custom card ID very unfriendly and "common: occasional" as most users don't care but many stores do use a different ID than value)

[obfusk]

Yeah, looking at it more closely I agree the flow is the wrong way around and all actions should by default affect "Barcode value", not "Card ID".

Yeah. I think I mentioned this before. I can probably think of a fix that doesn't require a migration. But the latter is probably cleaner.

[obfusk]

Logically, I think it should be that barcodeID is what gets set by scanning a card. And cardID can be used to override that if needed.

But we could have the UI set the barcodeID instead of the cardID. And when saving, check if the cardID is identical to the barcodeID or empty; if so, we set the cardID to the barcodeID and the barcodeID to null. That way we avoid a DB migration whilst having the UI be more logical.

I can make a PR for either option.

[mvglasow]

I fear this might need a database migration to properly fix (as the cardId is always stored but the barcodeId may be NULL in the database)

That could be worked around in code:

• when creating a new card, store the ID in cardid and set barcodeId to null
• when the user chooses to enter an ID that differs from the barcode, copy cardId to barcodeId, then overwrite cardId with the new value entered by the user
• when barcodeId is null, scanning a barcode sets cardId, else it sets barcodeId

It would increase the complexity of the code somewhat, though. Migration to a new DB scheme is also somewhat non-trivial due to constraints of ALTER TABLE – there doesn’t seem to be a direct way to modify nullability constraints on an existing column. Adding a new column and dropping the old one would change the order of columns and is incompatible with older (pre-2005) SQLite versions. The cleanest way is probably to recreate the table and re-add its existing data. Something like:

ALTER TABLE cards RENAME TO cards_old;

-- recreate cards table; note that cardid no longer has a NOT NULL constraint, while barcodeid does 
CREATE TABLE "cards" (
    "_id"   INTEGER,
    "store" TEXT NOT NULL,
    "note"  TEXT NOT NULL,
    "expiry"    INTEGER,
    "balance"   TEXT NOT NULL DEFAULT '0',
    "balancetype"   TEXT,
    "headercolor"   INTEGER,
    "cardid"    TEXT,
    "barcodeid" TEXT NOT NULL,
    "barcodetype"   TEXT,
    "starstatus"    INTEGER DEFAULT '0',
    "lastused"  INTEGER DEFAULT '0',
    "zoomlevel" INTEGER DEFAULT '100',
    "archive"   INTEGER DEFAULT '0',
    "validfrom" INTEGER,
    PRIMARY KEY("_id" AUTOINCREMENT)
);

INSERT INTO cards
SELECT
    _id, store, note, expiry, balance, balancetype, headercolor,
    CASE WHEN barcodeId IS NULL THEN NULL ELSE cardid END AS cardid,
    IFNULL(barcodeid, cardid) AS barcodeid,
    barcodetype, starstatus, lastused, zoomlevel, archive, validfrom
FROM cards_old;

DROP TABLE cards_old;

Still somewhat complex, but needs to be done only once and only if there is legacy data. Then there’s just the UI changes, which should be straightforward. On the long run, I guess that would keep the code more straightforward and less confusing.

[obfusk]

That could be worked around in code

That's what I suggested yes :)

Still somewhat complex, but needs to be done only once and only if there is legacy data

We'd also need to deal with the changed format in the importer. It's cleaner but a lot more work. Not sure if it's worth it. Up to @TheLastProject to decide :)

[TheLastProject]

Sorry for the extremely delayed response, life got hectic and responding to this ended up somewhere on my todo list.

While I generally dislike doing database migrations and prefer to avoid them whenever possible (simply because making a single mistake there can completely destroy user data without any way to recover it) I do feel a database change is the cleaner solution here and probably the route we should take. A migration like this can be tested and will long-term cause less complexity than working around things in code, even if it's more pain at first.

Switching the barcode and card ID around would indeed also affect the importer (as old imports may have the barcode id "in the wrong field"), affect the exporter (we'd need an export format version bump) and affect Gadgetbridge integration (although it would be fairly minor and easy to communicate with them, just bump the major version and tell them to read barcodeid instead of cardid on the new major version of the content provider). All in all it would be a group of minor breaking changes, nothing too scary, just would need thorough testing and good communication with the Gadgetbridge devs.