Skip to content

use local id from webdav #10011

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.

Sign up for GitHub

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

Jump to bottom
Closed
tobiasKaminsky wants to merge 1 commit into from
Closed

use local id from webdav #10011

tobiasKaminsky wants to merge 1 commit into from

Conversation

@tobiasKaminsky
Copy link
Member

@tobiasKaminsky tobiasKaminsky commented Mar 23, 2022

Fixes: #8566
Library: nextcloud/android-library#843

Signed-off-by: tobiasKaminsky [email protected]

  • Tests written, or not not needed

@github-actions
Copy link

github-actions bot commented Mar 23, 2022

blue-Light-IT test failed: https://www.kaminsky.me/nc-dev/android-integrationTests/10011-Screenshot-blue-Light-06-47

@nextcloud-android-bot
Copy link
Collaborator

nextcloud-android-bot commented Mar 23, 2022

stable-IT test failed: https://www.kaminsky.me/nc-dev/android-integrationTests/4379-IT-stable-06-54

@tobiasKaminsky @AlvaroBrey
use local id from webdav
2492e15 
Signed-off-by: tobiasKaminsky <[email protected]>
AlvaroBrey
AlvaroBrey approved these changes Mar 23, 2022
View reviewed changes
app/src/main/java/com/owncloud/android/datamodel/OCFile.java
}

@VisibleForTesting
public String getLocalIdDirect() {
Copy link
Member

@AlvaroBrey AlvaroBrey Mar 23, 2022

Choose a reason for hiding this comment

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

getInternalLocalId, or something like that, may be a better name.

app/src/main/java/com/owncloud/android/providers/FileContentProvider.java
Comment on lines +2476 to +2478
if (!upgraded) {
Log_OC.i(SQL, String.format(Locale.ENGLISH, UPGRADE_VERSION_MSG, oldVersion, newVersion));
}
Copy link
Member

@AlvaroBrey AlvaroBrey Mar 23, 2022

Choose a reason for hiding this comment

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

What's the point of adding this between each upgrade if? It will print the same every time. Also, some upgrades set upgraded=true, but others don't. Looks like this needs cleanup, I will do it after this PR is merged

@github-actions
Copy link

github-actions bot commented Mar 23, 2022

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

qrcode

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

@nextcloud-android-bot
Copy link
Collaborator

nextcloud-android-bot commented Mar 23, 2022

Codacy

Lint

TypemasterPR
Warnings9292
Errors00

SpotBugs (new)

Warning Type Number
Bad practice Warnings 28
Correctness Warnings 78
Experimental Warnings 1
Internationalization Warnings 9
Malicious code vulnerability Warnings 57
Multithreaded correctness Warnings 9
Performance Warnings 67
Security Warnings 29
Dodgy code Warnings 350
Total 628

SpotBugs (master)

Warning Type Number
Bad practice Warnings 28
Correctness Warnings 78
Experimental Warnings 1
Internationalization Warnings 9
Malicious code vulnerability Warnings 57
Multithreaded correctness Warnings 9
Performance Warnings 67
Security Warnings 29
Dodgy code Warnings 350
Total 628

Android-library is not set to master branch in build.gradle

@nextcloud-android-bot
Copy link
Collaborator

nextcloud-android-bot commented Mar 23, 2022

master-IT test failed: https://www.kaminsky.me/nc-dev/android-integrationTests/4392-IT-master-10-16

@github-actions
Copy link

github-actions bot commented Mar 23, 2022

blue-Light-Screenshot test failed, but no output was generated. Maybe a preliminary stage failed.

@github-actions
Copy link

github-actions bot commented Mar 23, 2022

blue-Light-IT test failed: https://www.kaminsky.me/nc-dev/android-integrationTests/10011-Screenshot-blue-Light-10-20

@AlvaroBrey
Copy link
Member

AlvaroBrey commented Mar 23, 2022

Similar trace in all failing tests (including screenshot)

java.lang.StringIndexOutOfBoundsException: length=3; index=8
at java.lang.String.substring(String.java:1971)
at com.owncloud.android.datamodel.OCFile.getLocalId(OCFile.java:560)
at com.owncloud.android.datamodel.FileDataStorageManager.saveFile(FileDataStorageManager.java:238)
at com.owncloud.android.datamodel.FileDataStorageManager.saveNewFile(FileDataStorageManager.java:348)
at com.owncloud.android.datamodel.FileDataStorageManagerIT.testSaveNewFile(FileDataStorageManagerIT.java:349)

AlvaroBrey
AlvaroBrey requested changes Mar 30, 2022
View reviewed changes
Copy link
Member

@AlvaroBrey AlvaroBrey left a comment

Choose a reason for hiding this comment

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

failing tests, see previous comment

@joshtrichards
Copy link
Member

joshtrichards commented Jan 30, 2025

Superceded by #8643

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@AlvaroBrey AlvaroBrey AlvaroBrey requested changes

Assignees

No one assigned

Labels

2. developing

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Use fileId instead of relying on remoteID

5 participants

@tobiasKaminsky @nextcloud-android-bot @AlvaroBrey @joshtrichards