Migrations: add migration to remove (invalid) media folder created by incorrect scan

[AlvaroBrey]

This folder was created in version 3.18.1 - 3.20.2 (inclusive) when moving media files to private storage.

This is caused by MediaScanner not being able to access the private storage, and instead of crashing it creates the (invalid) folder, with folders named the same as the original pictures.

Note: this will only work if the app has been granted storage permission, and the fake images won't disappear from gallery apps / mediaStore until next reboot or media scan.

More info: #9328

• [x] Tests written, or not not needed

[AlvaroBrey]

@tobiasKaminsky please give me your thoughts on whether we should do this or not. I have my doubts, reasoning at https://github.com/nextcloud/android/issues/9328#issuecomment-1157430516

@ezaquarii Can you give me some feedback as to whether this makes sense as a Migration, and whether it should be mandatory or not? I'm not that familiar with the Migrations model

[ezaquarii]

@AlvaroBrey Application of migrations API looks valid to me in this case, as you want to perform one-time change on application upgrade.

One thing that stands out for me is the need of permissions. I guess that the user must have provided this permission prior to creation of the bad media folder, because migration will be applied as no-op otherwise (i.e. marked as applied but no change done). I'm not sure if this is intended behaviour.

If the folder deletion is to be decided by the user, perhaps some other mechanism should be used.

[ezaquarii]

@AlvaroBrey If you'd like to re-try a migration after failure - because user did not provide permission to access storage - this can be done using optional migrations. Check out this test case: https://github.com/nextcloud/android/blob/fbfa7fa746a509be165219d960030cf4a0967eea/app/src/androidTest/java/com/nextcloud/client/migrations/MigrationsManagerTest.kt#L262

[AlvaroBrey]

One thing that stands out for me is the need of permissions. I guess that the user must have provided this permission prior to creation of the bad media folder, because migration will be applied as no-op otherwise (i.e. marked as applied but no change done). I'm not sure if this is intended behaviour.

The invalid folder is only created when uploading a file with "move to Nextcloud folder" selected as an after-upload behaviour; this is only possible with external storage permissions.

The only way this would not work is if the users actively remove the permission after uploading things and before upgrading to the version that contains this migration, but that seems narrow enough that we don't need to handle it.

[AlvaroBrey]

@tobiasKaminsky opinion on this migration?

[RokeJulianLockhart]

@AlvaroBrey, potentially send a notification if the storage permission is not provided, and state why? Most software that is for Android that I know of does this.

[AlvaroBrey]

@AlvaroBrey, potentially send a notification if the storage permission is not provided, and state why? Most software that is for Android that I know of does this.

Not a good idea, as we need the permission to even check the invalid folder exists; so we would be showing that notification to every person without storage permission, even if they don't have the invalid folder. And as stated above, it's much more likely that people with the invalid folder already have the permission, as it can't be created without it.

[codecov[bot]]

Codecov Report

Merging #10400 (8437987) into master (8437987) will not change coverage. The diff coverage is n/a.

:exclamation: Current head 8437987 differs from pull request most recent head 36a758e. Consider uploading reports for the commit 36a758e to get more accurate results

@@            Coverage Diff            @@
##             master   #10400   +/-   ##
=========================================
  Coverage     31.43%   31.43%           
  Complexity     3273     3273           
=========================================
  Files           540      540           
  Lines         40209    40209           
  Branches       5557     5557           
=========================================
  Hits          12638    12638           
  Misses        25675    25675           
  Partials       1896     1896           

[github-actions[bot]]

APK file: https://www.kaminsky.me/nc-dev/android-artifacts/10400.apk



To test this change/fix you can simply download above APK file and install and test it in parallel to your existing Nextcloud app.

[github-actions[bot]]

Codacy

Lint

Type	master	PR
Warnings	85	85
Errors	0	0

SpotBugs

Category	Base	New
Bad practice	29	29
Correctness	46	46
Dodgy code	353	353
Experimental	1	1
Internationalization	9	9
Multithreaded correctness	9	9
Performance	59	59
Security	28	28
Total	534	534

[AlvaroBrey]

Due to the time passed and initial concerns about this migration, I believe this PR is no longer relevant.