Fixed card duplication issue #2349 #2759

matuspetro · 2025-10-07T20:20:31Z

Removed condition that blocked image comparison
Changed comparison logic from ID-based to cardID-based to prevent duplicates on repeated import

TheLastProject · 2025-10-10T15:35:41Z

Added unit tests for Utils class

These tests don't seem relevant to this PR. Can you please split that into another PR, to keep this one focused on the change it is supposed to fix? That makes reviewing much easier.

matuspetro · 2025-10-11T19:55:07Z

@TheLastProject I removed tests from this PR.

TheLastProject · 2025-10-12T15:09:40Z

I'm a bit confused, you seem to just be checking if the existing and to-import card both have images and the same ID? That doesn't sound like it fixes images being detected as different when they aren't. Instead, you're now most likely overwriting images which aren't the same, which would be way worse! Unless I'm misreading your change somehow?

matuspetro · 2025-10-12T19:59:01Z

@TheLastProject here is a video of testing:
https://drive.google.com/file/d/1hFreqeMK3JsWd-ScrqaBVxhjhVKeZTM2/view?usp=sharing

TheLastProject · 2025-10-15T18:55:27Z

Sorry for the low video quality, I had to compress it a lot. But I also tested it and I could fairly easily get your change to cause data loss (overriding an existing card image) as I feared it would:

Screen_recording_20251015_204655.mp4

matuspetro · 2025-10-15T20:57:12Z

@TheLastProject you were right. I looked into it and fixed it. It should work fine now.

TheLastProject · 2025-10-19T17:13:18Z

After looking more deeply at this, I am starting to understand your attempted fix better. I think there is a better solution though.

The whole image checksum stuff is from the days before a LoyaltyCard object could contain images. However, in December last year support for storing Bitmaps directly in the LoyaltyCard object was added. If we simply remove the imageChecksums code in the .png loop and add the image to the LoyaltyCard object generated in the importCSV there, I believe the old duplicate code should already work fine. Then we can remove the imageChecksum code globally.

So I think basically all we need to do is:

Remove the whole imageChecksums stuff
When we find a .png, find the existing loyalty card and add the image to it

That basically would mean throwing the code away though, but it would be a much cleaner fix 😅

I think that also means migrating the string matching to use a Pattern.matcher (https://developer.android.com/reference/java/util/regex/Pattern#matcher(java.lang.CharSequence)) where you can get the card ID and front/back/icon out of the filename

matuspetro · 2025-10-19T21:36:38Z

I did everything as you wanted. I hope it's ready for merging now.

TheLastProject · 2025-10-20T18:55:53Z

It seems the tests are failing 😅

matuspetro · 2025-10-21T21:12:32Z

@TheLastProject

TheLastProject

I haven't tested the new code yet, but I think this flow makes sense, this may be mostly correct.

I do think you have a fair bit of dead code currently. A lot of checking if a card exists, when you actually only have 2 states which you already decided earlier:

Card is the same as a card that already exists (then you just ignore it)
Card is not the same, so needs to be inserted as a new card

So I feel that after cleaning that up this code may very well be correct and done? Of course, I'll have to test to make very sure though, but the general flow does feel correct now!

TheLastProject · 2025-10-22T17:09:50Z

app/src/main/java/protect/card_locker/DBHelper.java

+ public static List<LoyaltyCard> getLoyaltyCardsByCardId(Context context, SQLiteDatabase database, final String cardId) {
+ List<LoyaltyCard> cards = new ArrayList<>();
+
+ Cursor data = database.query(
+ LoyaltyCardDbIds.TABLE,
+ null,
+ whereAttrs(LoyaltyCardDbIds.CARD_ID),
+ withArgs(cardId),
+ null,
+ null,
+ null
+ );
+
+ if (data.moveToFirst()) {
+ do {
+ LoyaltyCard card = LoyaltyCard.fromCursor(context, data);
+ cards.add(card);
+ } while (data.moveToNext());
+ }
+
+ data.close();
+ return cards;
+ }
+


I'm not sure if you confused this with the unique ID before (this is the barcode value basically). If your plan was to limit the amount of loyalty cards to loop over, you could still just use getLoyaltyCardCursor and limit it by the store instead of creating a new function. The DBHelper class is already way too complex, so I'd rather prevent extra complexity there whenever possible.

Yes, I know it's a barcode. I didn't want to compare the imported card with all the others, so I chose the barcode as a filter.

Previously, the code only compared the card with an existing card with the same ID, which led to duplicates. Since the ID is assigned randomly, it must be removed from the comparison.

This part of the code is therefore essential from a performance perspective.

This part of the code is therefore essential from a performance perspective.

I mean, not really, you can also just call one of the existing getLoyaltyCardCursor functions and use the store name as the filter. In fact that would be better, because a cursor lets you loop over the results without loading them all into memory :)

I do agree that removing the database ID from the comparison is correct though.

Let's think about a reality.
How many users want to have multiple cards with same Barcode?
How many users want to have multiple cards with same store name?

Both of these are very rare, but I think that the first one is absolutely illogical.

The reality is that both will be fairly rare, but the second one doesn't need a new function. And thus is cleaner and easier for long-term maintenance of the codebase.

TheLastProject · 2025-10-22T17:10:06Z

app/src/main/java/protect/card_locker/LoyaltyCard.java

 return false;
+ // One is null and the other isn't, so it's not equal


Why is this swapped?

TheLastProject · 2025-10-22T18:07:05Z

app/src/main/java/protect/card_locker/importexport/CatimaImporter.java

- if (!isZipFile) {
+ if (zipFile.isValidZipFile()) {
+ importedData = importZIP(zipFile);
+ }else {


Coding style

Suggested change

}else {

} else {

TheLastProject · 2025-10-22T18:08:58Z

app/src/main/java/protect/card_locker/importexport/CatimaImporter.java

- public ImportedData parseV1(BufferedReader input) throws IOException, FormatException, InterruptedException {
+ public ImportedData importZIP(ZipFile zipFile) throws IOException, FormatException, InterruptedException {
+ FileHeader fileHeader = zipFile.getFileHeader("catima.csv");
+ if (fileHeader == null){


Code style

Suggested change

if (fileHeader == null){

if (fileHeader == null) {

TheLastProject · 2025-10-22T18:12:58Z

app/src/main/java/protect/card_locker/importexport/CatimaImporter.java

 LoyaltyCard existing = DBHelper.getLoyaltyCard(context, database, card.id);
- if (existing == null) {
- DBHelper.insertLoyaltyCard(database, card.id, card.store, card.note, card.validFrom, card.expiry, card.balance, card.balanceType,
- card.cardId, card.barcodeId, card.barcodeType, card.headerColor, card.starStatus, card.lastUsed, card.archiveStatus);
- } else if (!isDuplicate(context, existing, card, existingImages, imageChecksums)) {
- long newId = DBHelper.insertLoyaltyCard(database, card.store, card.note, card.validFrom, card.expiry, card.balance, card.balanceType,
- card.cardId, card.barcodeId, card.barcodeType, card.headerColor, card.starStatus, card.lastUsed, card.archiveStatus);
- idMap.put(card.id, (int) newId);
+
+ if (candidates.isEmpty()) {
+ if (existing != null && existing.id == card.id) {
+ long newId = insertCard(context, database, card, true);
+ idMap.put(card.id, (int) newId);
+ }else{
+ insertCard(context, database, card, false);
+ }
+ }else if (!duplicateFound) {
+ if (existing != null && existing.id == card.id) {
+ long newId = insertCard(context, database, card, true);
+ idMap.put(card.id, (int) newId);
+ }else{
+ insertCard(context, database, card, false);
+ }
 }


This seems a bit weird. You're first checking if the card is a duplicate and after that you check if it matches with an existing card.

Wouldn't it be more logical to just leave this loop if the card is a duplicate, as you don't want to import a duplicate anyway? That would severely simplify this.

Hmm... yes, the outer if is unnecessary. I'll delete it.

The imported card is first compared with potential duplicates. If the card is a duplicate, it is not imported. But if it is not a duplicate, it is checked to see if there is a card with the same ID. If so, it is inserted as a new card with a new ID. If not, it is inserted without a new ID.
This is just so that it passes the tests.

Sounds more like the tests are wrong in this case then? Because that doesn't sound very logical 😅

Maybe, but it works fine.

TheLastProject · 2025-10-22T18:17:42Z

app/src/main/java/protect/card_locker/importexport/CatimaImporter.java

+ private long insertCard(Context context, SQLiteDatabase database, LoyaltyCard card, boolean newCard) throws IOException{
+ long newId = 0;
+ if (newCard){
+ newId = DBHelper.insertLoyaltyCard(database, card.store, card.note, card.validFrom, card.expiry, card.balance, card.balanceType,
+ card.cardId, card.barcodeId, card.barcodeType, card.headerColor, card.starStatus, card.lastUsed, card.archiveStatus);
+ }else {
+ DBHelper.insertLoyaltyCard(database, card.id, card.store, card.note, card.validFrom, card.expiry, card.balance, card.balanceType,
+ card.cardId, card.barcodeId, card.barcodeType, card.headerColor, card.starStatus, card.lastUsed, card.archiveStatus);
+ }


Given you only insert cards now if they don't match an existing card, isn't it always with newCard as True? I don't think you need this if/else anymore?

This was just so that I wouldn't call the same thing four times. No, newCard is not always true. But I remove this in next commit.

I don't understand why newCard will not always be true.

Either it's a duplicate, so you don't insert it.
Or it's not a duplicate, so it gets inserted as a new card.

It is inserted, but not always with new ID. This is logic of old implementation.

TheLastProject · 2025-10-22T18:18:52Z

app/src/main/java/protect/card_locker/importexport/CatimaImporter.java

+ Utils.saveCardImage(context, card.getImageThumbnail(context), card.id, ImageLocationType.icon);
+ Utils.saveCardImage(context, card.getImageFront(context), card.id, ImageLocationType.front);
+ Utils.saveCardImage(context, card.getImageBack(context), card.id, ImageLocationType.back);
+
+ return newId;


This order seems wrong. You are inserting a card into the database and then get a new ID if it's a new card, but save the images onto the old ID. It seems to me like you'd want to have this order:

Save card to database and get ID

Save the images with that new ID

Otherwise, the images may be assigned to the wrong card and still override old images.

As I wrote above, this code will not be in the next commit.

matuspetro · 2025-10-28T12:53:06Z

I think that this issue can be closed. What do you think @TheLastProject?

TheLastProject

I still need to actually test this code again (and I barely have time to look really deeply into it), but two issues remain unsolved.

I think this is very close to being merge-worthy though

TheLastProject · 2025-10-29T16:06:06Z

app/src/main/java/protect/card_locker/DBHelper.java

+ public static List<LoyaltyCard> getLoyaltyCardsByCardId(Context context, SQLiteDatabase database, final String cardId) {
+ List<LoyaltyCard> cards = new ArrayList<>();
+
+ Cursor data = database.query(
+ LoyaltyCardDbIds.TABLE,
+ null,
+ whereAttrs(LoyaltyCardDbIds.CARD_ID),
+ withArgs(cardId),
+ null,
+ null,
+ null
+ );
+
+ if (data.moveToFirst()) {
+ do {
+ LoyaltyCard card = LoyaltyCard.fromCursor(context, data);
+ cards.add(card);
+ } while (data.moveToNext());
+ }
+
+ data.close();
+ return cards;
+ }
+


The reality is that both will be fairly rare, but the second one doesn't need a new function. And thus is cleaner and easier for long-term maintenance of the codebase.

TheLastProject · 2025-10-29T16:07:37Z

app/src/main/java/protect/card_locker/importexport/CatimaImporter.java

+ if (existing != null && existing.id == card.id) {
+ long newId = DBHelper.insertLoyaltyCard(database, card.store, card.note, card.validFrom, card.expiry, card.balance, card.balanceType,
+ card.cardId, card.barcodeId, card.barcodeType, card.headerColor, card.starStatus, card.lastUsed, card.archiveStatus);
+ idMap.put(card.id, (int) newId);
+ Utils.saveCardImage(context, card.getImageThumbnail(context), (int) newId, ImageLocationType.icon);
+ Utils.saveCardImage(context, card.getImageFront(context), (int) newId, ImageLocationType.front);
+ Utils.saveCardImage(context, card.getImageBack(context), (int) newId, ImageLocationType.back);
+ }else{
+ DBHelper.insertLoyaltyCard(database, card.id, card.store, card.note, card.validFrom, card.expiry, card.balance, card.balanceType,
+ card.cardId, card.barcodeId, card.barcodeType, card.headerColor, card.starStatus, card.lastUsed, card.archiveStatus);
+ Utils.saveCardImage(context, card.getImageThumbnail(context), card.id, ImageLocationType.icon);
+ Utils.saveCardImage(context, card.getImageFront(context), card.id, ImageLocationType.front);
+ Utils.saveCardImage(context, card.getImageBack(context), card.id, ImageLocationType.back);
+ }


I still insist this code is simply wrong.

Either the card is a duplicate and thus can be skipped.
Or the card is not a duplicate, and thus is inserted as a new card with a new ID.

There is no in-between state.

Look at your implementation before. It's the same code... Same logic as it was before.

The implementation before was wrong, otherwise it wouldn't need fixing 😅

TheLastProject · 2025-10-31T16:26:45Z

Don't know if this is for Hacktoberfest or not. I don't want you to get "screwed over" for this not being completed in time, so I'll label it hacktoberfest-accepted just in case :) Would be great if the last comments can be looked at, I think this is probably almost correct, but it's clearly already deserved to count from the effort put into it :)

matuspetro · 2025-11-03T19:11:28Z

@TheLastProject I removed additional function getLoyaltyCardsByCardId and I call instead DB cursor as you wanted.

I look at the second bug too. When I removed checking of existing card with the same ID, it created 3 errors in test as last time, but I was not able to fix them. I find out that the problem was in importing cards to group, but I didn't see where. It added card to group, but the card wasn't existing. So number cards in group was higher that in reality. It little bit surprised me, how it's possible that problem is not there when I add this checking. In conclusion, it is good as it is.

I have another work too, so in my point of view this issue is done. If you test it, and you will find any duplication of cards or data lost, please let me know and I will fix it. But if this code will work fine, I will be waiting for merge.

I'm not involved in Hacktoberfest, but I will be very thankful if you will merge this 😊

TheLastProject · 2025-11-04T18:03:25Z

I removed additional function getLoyaltyCardsByCardId and I call instead DB cursor as you wanted.

Sorry, but I really meant the getLoyaltyCardCursor function: DBHelper.getLoyaltyCardCursor(database, card.store, LoyaltyCardArchiveFilter.All)

I look at the second bug too. When I removed checking of existing card with the same ID, it created 3 errors in test as last time, but I was not able to fix them. I find out that the problem was in importing cards to group, but I didn't see where. It added card to group, but the card wasn't existing. So number cards in group was higher that in reality. It little bit surprised me, how it's possible that problem is not there when I add this checking. In conclusion, it is good as it is.

Okay but a group issue makes sense! See the export documentation.

The group part is a list of: cardId,groupId. If cards get inserted as new cards, they will get a new ID, but the "old" ID (the one in the catima.zip) is the one that tells you which group it belongs to. So that is probably why the tests would be failing (though I have to guess, I've never seen the test failures, as you've never posted the actual test error logs or the code that was failing).

I have another work too, so in my point of view this issue is done. If you test it, and you will find any duplication of cards or data lost, please let me know and I will fix it. But if this code will work fine, I will be waiting for merge.

Well, I'm not convinced the code is correct yet, so I don't think it is done and I'm not planning to merge it right now. But if you don't want to keep working on it that's fine, I can take a look at it myself and see if I can figure it out and fix it. Neither of us owes the other free labour :)

matuspetro · 2025-11-04T19:48:43Z

Your idea :)

My implementation...

Do whatever you want...

matuspetro added 2 commits October 7, 2025 16:23

Fixed img comparison

a95f5ce

Fixed issue: duplicity after importing same zip again

4bf0862

matuspetro force-pushed the card-duplication-issue-#2349 branch from a0a0020 to 4bf0862 Compare October 11, 2025 19:41

TheLastProject added the needs changes label Oct 15, 2025

fix rewriting imgs

61e0ca4

TheLastProject linked an issue Oct 19, 2025 that may be closed by this pull request

Card duplication detection during import doesn't always work #2349

Open

replace imageChecksums with bitmapComparing

f2b2f3c

fixed test errors

a832066

TheLastProject reviewed Oct 22, 2025

View reviewed changes

update useless if and some code style

3dece0b

TheLastProject requested changes Oct 29, 2025

View reviewed changes

TheLastProject added the hacktoberfest-accepted label Oct 31, 2025

change getLoyaltyCardsByCardId to getLoyaltyCardCursor

50c29f5

		return false;
		// One is null and the other isn't, so it's not equal

Uh oh!

Fixed card duplication issue #2349 #2759

Are you sure you want to change the base?

Fixed card duplication issue #2349 #2759

Uh oh!

Conversation

matuspetro commented Oct 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

TheLastProject commented Oct 10, 2025

matuspetro commented Oct 11, 2025

TheLastProject commented Oct 12, 2025

matuspetro commented Oct 12, 2025

TheLastProject commented Oct 15, 2025

matuspetro commented Oct 15, 2025

TheLastProject commented Oct 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

matuspetro commented Oct 19, 2025

TheLastProject commented Oct 20, 2025

matuspetro commented Oct 21, 2025

TheLastProject left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matuspetro commented Oct 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

TheLastProject left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheLastProject commented Oct 31, 2025

matuspetro commented Nov 3, 2025

TheLastProject commented Nov 4, 2025

matuspetro commented Nov 4, 2025

Labels

2 participants

matuspetro commented Oct 7, 2025 •

edited

Loading

TheLastProject commented Oct 19, 2025 •

edited

Loading

matuspetro commented Oct 28, 2025 •

edited

Loading