Android
Android copied to clipboard
CatimaLoyalty
Fixed card duplication issue #2349
- Removed condition that blocked image comparison
- Changed comparison logic from ID-based to cardID-based to prevent duplicates on repeated import
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.
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?
@TheLastProject here is a video of testing: https://drive.google.com/file/d/1hFreqeMK3JsWd-ScrqaBVxhjhVKeZTM2/view?usp=sharing
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:
https://github.com/user-attachments/assets/3a4213ac-7578-4a81-bccf-d369e2fb033b
@TheLastProject you were right. I looked into it and fixed it. It should work fine now.
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 :sweat_smile:
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
I think that this issue can be closed. What do you think @TheLastProject?
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 :)
@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 😊
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 :)