Skip to content

[Bug fix] Check if duplicate exist using both original and modified file's checksum #6169

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

Merged
merged 2 commits into from
Feb 4, 2025

Conversation

parneet-guraya
Copy link
Contributor

Fixes #5800

What changes did you make and why?

Before this, we were only checking checksum of modified file to see if file exists or not. But, we needed for both because when file is uploaded from web interface it stores the original file's checksum, so our check never truly checked for duplicates.

Now, we check both original and modified file's checksum when user chooses a file to upload.

Copy link
Member

@misaochan misaochan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you list the tests that you have performed and on which API and build variant, please? A screencast of your test and a link to the original file that you are testing with would be helpful as well. Thanks!

@parneet-guraya
Copy link
Contributor Author

Could you list the tests that you have performed and on which API and build variant, please?

Yes, as described in the issue's description ->

  • Upload a file from web interface.
  • Now, choose the same image to upload from mobile app. (This will check both original and modified (after EXIF modifications) file's checksum for duplicity)
  • It will prompt you that image already exists.

Tested on both betaDebug and prodDebug

Device: Oneplus 9RT 5G, Android 14

A screencast of your test and a link to the original file that you are testing with would be helpful as well

I could provide the screen recording but it would have been longer first upload in the web interface and then upload in the app (If you still want I can do it). Also, you can see any file would work for testing as long as same file is used both times :-)


Also, this reminds that other way around doesn't work i.e if I upload from mobile first and then take the same file to upload from web interface it wouldn't report any duplication issues. The reason is we upload the file after EXIF modifications from the mobile but when we take the same file to upload from web interface it just check against the original version so, it sees no duplicity.

I'd love your input on what should be the solution for that and also @nicolas-raoul

  • Should the web interface also check for modified version? but user preference about what to redact in the file meta info is in the mobile app preference, so no way web can know what changes have been done.
  • Or we can upload the original checksum too when uploading from the mobile app to the database that have the info about checksum (that we're already using to check if image exists). This way when web check for duplicity with original image's checksum it can be found. Do we have an endpoint that let us upload checksums too?

It was in my TODOs to file this issue but I'm outside so will do later.

@misaochan
Copy link
Member

I could provide the screen recording but it would have been longer first upload in the web interface and then upload in the app (If you still want I can do it).

Yes, I think you could do either of these options:

  • Record the upload via web interface and then via the app, OR
  • Provide the link to the original that was uploaded via web interface, then just screencast the attempt to upload via the app

@parneet-guraya
Copy link
Contributor Author

Record the upload via web interface and then via the app, OR

Sure, here it is.

Screencast.from.2025-02-03.18-42-29.webm

@nicolas-raoul
Copy link
Member

Screencast looks good. 🙂

About the other way (app then website):

A separate enhancement would be to include the checksums into the uploaded image's wikicode, using a template yet to define. Then hopefully in the far future the team in charge of the website would use that data to perform the appropriate checks. I think #5295 talks a bit about this.

@misaochan
Copy link
Member

Thanks @parneet-guraya ! Merging, feel free to submit the aforementioned enhancement as a separate PR. :)

@misaochan misaochan merged commit 43dca1d into commons-app:main Feb 4, 2025
1 check passed
@parneet-guraya parneet-guraya deleted the new-checksum-fix branch February 5, 2025 04:58
Sujal-Gupta-SG pushed a commit to Sujal-Gupta-SG/apps-android-commons that referenced this pull request Feb 5, 2025
…ile's checksum (commons-app#6169)

* check original file's SHA too along with modified one

Signed-off-by: parneet-guraya <[email protected]>

* fix tests

Signed-off-by: parneet-guraya <[email protected]>

---------

Signed-off-by: parneet-guraya <[email protected]>
nicolas-raoul added a commit that referenced this pull request Feb 7, 2025
* Rename .java to .kt

* Migrated ContributionController

* Rename .java to .kt

* Migrated ContributionDao

* Rename .java to .kt

* Migrated ContributionsContract,ContributionFragment,ContributionListAdapter,ContributionsListContract from java to Kotlin

* Rename .java to .kt

* converted/Migrated

* converted/Migrated

* Rename .java to .kt

* Migrated ContributionController

* Rename .java to .kt

* Migrated ContributionDao

* Rename .java to .kt

* Migrated ContributionsContract,ContributionFragment,ContributionListAdapter,ContributionsListContract from java to Kotlin

* Rename .java to .kt

* Show placeholder and display depiction section when no depictions are available (#6163) (#6165)

* corrected

* corrected

* Update MediaDetailFragment.kt

Spelling correction

* Migrated AboutActivity from Java to Kotlin (#6158)

* Rename Constants to Follow Kotlin Naming Conventions

>This PR refactors constant names in the project to adhere to Kotlin's UPPERCASE_SNAKE_CASE naming convention, improving code readability and maintaining consistency across the codebase.

>Renamed the following constants in LoginActivity:
>saveProgressDialog → SAVE_PROGRESS_DIALOG
>saveErrorMessage → SAVE_ERROR_MESSAGE
>saveUsername → SAVE_USERNAME
>savePassword → SAVE_PASSWORD

>Updated all references to these constants throughout the project.

* Update Project_Default.xml

* Refactor variable names to adhere to naming conventions

Renamed variables to use camel case:
-UPLOAD_COUNT_THRESHOLD → uploadCountThreshold
-REVERT_PERCENTAGE_FOR_MESSAGE → revertPercentageForMessage
-REVERT_SHARED_PREFERENCE → revertSharedPreference
-UPLOAD_SHARED_PREFERENCE → uploadSharedPreference

Renamed variables with uppercase initials to lowercase for alignment with Kotlin conventions:
-Latitude → latitude
-Longitude → longitude
-Accuracy → accuracy

Refactored the following variable names:
-NUMBER_OF_QUESTIONS → numberOfQuestions
-MULTIPLIER_TO_GET_PERCENTAGE → multiplierToGetPercentage

* Refactor Dialog View Initialization with Null-Safe Calls

This PR refactors the dialog setup code in CustomSelectorActivity to improve safety and readability by replacing explicit casts with null-safe generic calls for findViewById.

>Replaced explicit casting (as Button and as TextView) with the generic findViewById<T>() method for improved type safety.
>Added null-safety (?.) to avoid potential crashes if a view is not found in the dialog layout.

why changed:-
>Prevents runtime crashes caused by NullPointerException when a view is missing in the layout.

* Refactor Unit Test: Replace Unsafe Casting with Type-Safe Mocking for findViewById

>PR refactors the unit test for NearbyParentFragment by replacing unsafe casting in the findViewById mocking statements with type-safe

>Ensured all findViewById mocks now use a consistent, type-safe format (findViewById<View>(...)) to reduce verbosity and potential casting errors.

>Verified the functionality of prepareViewsForSheetPosition remains unchanged, ensuring no regression in test behavior.

* Update NearbyParentFragmentUnitTest.kt

* Refactor: Rename Constants to Follow CamelCase Naming Convention

>Updated all constant variable names to follow the camelCase naming convention, removing underscores in the middle or end.

>Ensured variable names remain descriptive and align with code readability best practices.

* Replace private val with const val for URL constants in QuizController

* Renaming the constant to use UPPER_SNAKE_CASE

* Renaming the constant to use UPPER_SNAKE_CASE

* Update Done

* **Refactor: Convert `minimumThresholdForSwipe` to a compile-time constant**

* Convert AboutActivity from Java to Kotlin

This PR converts the AboutActivity class from Java to Kotlin

>Testing:
>Verified all functionalities of the AboutActivity, including toolbar setup, intent launches, and dialog interactions, to ensure behavior remains consistent post-conversion.
>Successfully ran unit tests for AboutActivity to confirm the correctness of methods and logic.

* Thank you for the suggestion! Since these methods all take a single View parameter, replacing them with method references is a great way to simplify the code and improve readability. I'll updated the code accordingly. Added a TODO in the code as a reminder to refactor this in the future.

---------

Co-authored-by: Nicolas Raoul <[email protected]>

* Localisation updates from https://translatewiki.net.

* Feat: Make it smoother to switch between nearby and explore maps (#6164)

* Nearby: Add 'Show in Explore' 3-dots menu item

* MainActivity: Add methods to pass extras between Nearby and Explore

* MainActivity: Extend loadFragment() to support passing fragment arguments

* Nearby: Add ability to navigate to Explore fragment on 'Show in Explore' click

* Explore: Read fragment arguments for Nearby map data and update Explore map if present

* Explore: Add 'Show in Nearby' 3-dots menu item. Only visible when Map tab is selected

* Explore: On 'Show in Nearby' click, navigate to Nearby fragment, passing map data as fragment args

* Nearby: Read fragment arguments for Explore map data and update Nearby map if present

* MainActivity: Fix memory leaks when navigating between bottom nav destinations

* Explore: Fix crashes caused by unattached map fragment

* Refactor code to pass unit tests

* Explore: Format javadocs

---------

Co-authored-by: Nicolas Raoul <[email protected]>

* Localisation updates from https://translatewiki.net.

* enhance spammy category filter (#6167)

Signed-off-by: parneet-guraya <[email protected]>

* Localisation updates from https://translatewiki.net.

* correction

* correction

* correction

* GitHub workflow to build betaDebug (#6174)

* [Bug fix] Check if duplicate exist using both original and modified file's checksum (#6169)

* check original file's SHA too along with modified one

Signed-off-by: parneet-guraya <[email protected]>

* fix tests

Signed-off-by: parneet-guraya <[email protected]>

---------

Signed-off-by: parneet-guraya <[email protected]>

* Add multiline input for caption and description (#6173)

* allow multiple lines for description/caption

* make caption multiline too

---------

Co-authored-by: Nicolas Raoul <[email protected]>

* correction

---------

Signed-off-by: parneet-guraya <[email protected]>
Co-authored-by: Akshay Komar <[email protected]>
Co-authored-by: Nicolas Raoul <[email protected]>
Co-authored-by: translatewiki.net <[email protected]>
Co-authored-by: Ifeoluwa Andrew Omole <[email protected]>
Co-authored-by: Parneet Singh <[email protected]>
Co-authored-by: Matija Nalis <[email protected]>
nicolas-raoul pushed a commit that referenced this pull request Feb 26, 2025
…ile's checksum (#6169)

* check original file's SHA too along with modified one

Signed-off-by: parneet-guraya <[email protected]>

* fix tests

Signed-off-by: parneet-guraya <[email protected]>

---------

Signed-off-by: parneet-guraya <[email protected]>
nicolas-raoul added a commit that referenced this pull request Feb 26, 2025
* Rename .java to .kt

* Migrated ContributionController

* Rename .java to .kt

* Migrated ContributionDao

* Rename .java to .kt

* Migrated ContributionsContract,ContributionFragment,ContributionListAdapter,ContributionsListContract from java to Kotlin

* Rename .java to .kt

* converted/Migrated

* converted/Migrated

* Rename .java to .kt

* Migrated ContributionController

* Rename .java to .kt

* Migrated ContributionDao

* Rename .java to .kt

* Migrated ContributionsContract,ContributionFragment,ContributionListAdapter,ContributionsListContract from java to Kotlin

* Rename .java to .kt

* Show placeholder and display depiction section when no depictions are available (#6163) (#6165)

* corrected

* corrected

* Update MediaDetailFragment.kt

Spelling correction

* Migrated AboutActivity from Java to Kotlin (#6158)

* Rename Constants to Follow Kotlin Naming Conventions

>This PR refactors constant names in the project to adhere to Kotlin's UPPERCASE_SNAKE_CASE naming convention, improving code readability and maintaining consistency across the codebase.

>Renamed the following constants in LoginActivity:
>saveProgressDialog → SAVE_PROGRESS_DIALOG
>saveErrorMessage → SAVE_ERROR_MESSAGE
>saveUsername → SAVE_USERNAME
>savePassword → SAVE_PASSWORD

>Updated all references to these constants throughout the project.

* Update Project_Default.xml

* Refactor variable names to adhere to naming conventions

Renamed variables to use camel case:
-UPLOAD_COUNT_THRESHOLD → uploadCountThreshold
-REVERT_PERCENTAGE_FOR_MESSAGE → revertPercentageForMessage
-REVERT_SHARED_PREFERENCE → revertSharedPreference
-UPLOAD_SHARED_PREFERENCE → uploadSharedPreference

Renamed variables with uppercase initials to lowercase for alignment with Kotlin conventions:
-Latitude → latitude
-Longitude → longitude
-Accuracy → accuracy

Refactored the following variable names:
-NUMBER_OF_QUESTIONS → numberOfQuestions
-MULTIPLIER_TO_GET_PERCENTAGE → multiplierToGetPercentage

* Refactor Dialog View Initialization with Null-Safe Calls

This PR refactors the dialog setup code in CustomSelectorActivity to improve safety and readability by replacing explicit casts with null-safe generic calls for findViewById.

>Replaced explicit casting (as Button and as TextView) with the generic findViewById<T>() method for improved type safety.
>Added null-safety (?.) to avoid potential crashes if a view is not found in the dialog layout.

why changed:-
>Prevents runtime crashes caused by NullPointerException when a view is missing in the layout.

* Refactor Unit Test: Replace Unsafe Casting with Type-Safe Mocking for findViewById

>PR refactors the unit test for NearbyParentFragment by replacing unsafe casting in the findViewById mocking statements with type-safe

>Ensured all findViewById mocks now use a consistent, type-safe format (findViewById<View>(...)) to reduce verbosity and potential casting errors.

>Verified the functionality of prepareViewsForSheetPosition remains unchanged, ensuring no regression in test behavior.

* Update NearbyParentFragmentUnitTest.kt

* Refactor: Rename Constants to Follow CamelCase Naming Convention

>Updated all constant variable names to follow the camelCase naming convention, removing underscores in the middle or end.

>Ensured variable names remain descriptive and align with code readability best practices.

* Replace private val with const val for URL constants in QuizController

* Renaming the constant to use UPPER_SNAKE_CASE

* Renaming the constant to use UPPER_SNAKE_CASE

* Update Done

* **Refactor: Convert `minimumThresholdForSwipe` to a compile-time constant**

* Convert AboutActivity from Java to Kotlin

This PR converts the AboutActivity class from Java to Kotlin

>Testing:
>Verified all functionalities of the AboutActivity, including toolbar setup, intent launches, and dialog interactions, to ensure behavior remains consistent post-conversion.
>Successfully ran unit tests for AboutActivity to confirm the correctness of methods and logic.

* Thank you for the suggestion! Since these methods all take a single View parameter, replacing them with method references is a great way to simplify the code and improve readability. I'll updated the code accordingly. Added a TODO in the code as a reminder to refactor this in the future.

---------

Co-authored-by: Nicolas Raoul <[email protected]>

* Localisation updates from https://translatewiki.net.

* Feat: Make it smoother to switch between nearby and explore maps (#6164)

* Nearby: Add 'Show in Explore' 3-dots menu item

* MainActivity: Add methods to pass extras between Nearby and Explore

* MainActivity: Extend loadFragment() to support passing fragment arguments

* Nearby: Add ability to navigate to Explore fragment on 'Show in Explore' click

* Explore: Read fragment arguments for Nearby map data and update Explore map if present

* Explore: Add 'Show in Nearby' 3-dots menu item. Only visible when Map tab is selected

* Explore: On 'Show in Nearby' click, navigate to Nearby fragment, passing map data as fragment args

* Nearby: Read fragment arguments for Explore map data and update Nearby map if present

* MainActivity: Fix memory leaks when navigating between bottom nav destinations

* Explore: Fix crashes caused by unattached map fragment

* Refactor code to pass unit tests

* Explore: Format javadocs

---------

Co-authored-by: Nicolas Raoul <[email protected]>

* Localisation updates from https://translatewiki.net.

* enhance spammy category filter (#6167)

Signed-off-by: parneet-guraya <[email protected]>

* Localisation updates from https://translatewiki.net.

* correction

* correction

* correction

* GitHub workflow to build betaDebug (#6174)

* [Bug fix] Check if duplicate exist using both original and modified file's checksum (#6169)

* check original file's SHA too along with modified one

Signed-off-by: parneet-guraya <[email protected]>

* fix tests

Signed-off-by: parneet-guraya <[email protected]>

---------

Signed-off-by: parneet-guraya <[email protected]>

* Add multiline input for caption and description (#6173)

* allow multiple lines for description/caption

* make caption multiline too

---------

Co-authored-by: Nicolas Raoul <[email protected]>

* correction

---------

Signed-off-by: parneet-guraya <[email protected]>
Co-authored-by: Akshay Komar <[email protected]>
Co-authored-by: Nicolas Raoul <[email protected]>
Co-authored-by: translatewiki.net <[email protected]>
Co-authored-by: Ifeoluwa Andrew Omole <[email protected]>
Co-authored-by: Parneet Singh <[email protected]>
Co-authored-by: Matija Nalis <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Two checksums for the same image - upload via upload wizard and commons app
3 participants