Skip to content
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

Fix upload download problems #12779

Closed
wants to merge 5 commits into from
Closed

Fix upload download problems #12779

wants to merge 5 commits into from

Conversation

alperozturk96
Copy link
Collaborator

@alperozturk96 alperozturk96 commented Apr 1, 2024 •
edited
Loading

  • Tests written, or not not needed

This PR Fixes Following Problems

  • Sync icon visibility during download
  • Accessing after download attempt to etag throws null point exception
  • Remove file dialog actions order is wrong for downloaded and normal file
  • File download

Note

The Spotbugs count increased due to the reintroduction of old code (with improvements) in this PR.

This was referenced Apr 1, 2024
Closed
Closed
alperozturk96
alperozturk96 commented Apr 1, 2024
View reviewed changes
app/src/main/java/com/owncloud/android/datamodel/OCFile.java
@@ -361,15 +361,6 @@ public boolean existsOnDevice() {
return false;
}

public String getFileNameWithoutExtension(String fileName) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Unused function

alperozturk96
alperozturk96 commented Apr 1, 2024
View reviewed changes
app/src/main/java/com/owncloud/android/operations/UploadFileOperation.java
@@ -726,7 +726,7 @@ private RemoteOperationResult encryptedUpload(OwnCloudClient client, OCFile pare
metadata,
token,
client,
metadataExists,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Metadata is already available for version 2, so there is no need to check the metadataExists value for version 2 because this value is only verified for version 1.

alperozturk96
alperozturk96 commented Apr 1, 2024
View reviewed changes
app/src/main/java/com/owncloud/android/operations/DownloadFileOperation.java
@@ -273,7 +273,13 @@ protected RemoteOperationResult run(OwnCloudClient client) {
}
}

if (downloadType == DownloadType.EXPORT) {
if (downloadType == DownloadType.DOWNLOAD && !file.isEncrypted()) {
Copy link
Collaborator Author

@alperozturk96 alperozturk96 Apr 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Code that was removed during end-to-end (e2e) fixes previously caused download issues for e2e. However, after recent improvements, it no longer affects e2e and doesn't disrupt normal file downloads either. Special thanks to @Unpublished for this.

I just added isEncrypted check because if we don't ignore this if block for e2e, file try to be opened as encrypted, therefore user will not able to preview file.

I tested for v1 and v2 encryption, it's working.

Copy link
Contributor

Choose a reason for hiding this comment

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

Confirming normal download works for me with this PR 👍

@Unpublished Unpublished linked an issue Apr 1, 2024 that may be closed by this pull request
Regression: Downloaded files stay in tmp, not in intended location #12775
Closed
4 tasks
@alperozturk96 alperozturk96 force-pushed the bugfix/fix-upload-download-problems branch from 620d522 to e692c15 Compare April 1, 2024 12:21
@github-actions GitHub Actions
Copy link

github-actions bot commented Apr 1, 2024

Codacy

Lint

TypemasterPR
Warnings7171
Errors33

SpotBugs

CategoryBaseNew
Bad practice6768
Correctness6868
Dodgy code350350
Experimental11
Internationalization77
Malicious code vulnerability22
Multithreaded correctness66
Performance5858
Security1919
Total578579

SpotBugs increased!

@github-actions GitHub Actions
Copy link

github-actions bot commented Apr 1, 2024

APK file: https://www.kaminsky.me/nc-dev/android-artifacts/12779.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.

@github-actions GitHub Actions
Copy link

github-actions bot commented Apr 1, 2024

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

@alperozturk96
Fix sync
60304f1 
Signed-off-by: alperozturk <alper_ozturk@proton.me>
@alperozturk96
Fix sync
97ade7c 
Signed-off-by: alperozturk <alper_ozturk@proton.me>
@alperozturk96
Fix wrong button placement for remove files
093d14e 
Signed-off-by: alperozturk <alper_ozturk@proton.me>
@alperozturk96
Fix
057387a 
Signed-off-by: alperozturk <alper_ozturk@proton.me>
@alperozturk96
Fix for e2e
ef9c088 
Signed-off-by: alperozturk <alper_ozturk@proton.me>
@alperozturk96 alperozturk96 force-pushed the bugfix/fix-upload-download-problems branch from e692c15 to ef9c088 Compare April 2, 2024 07:15
@Unpublished Unpublished removed a link to an issue Apr 2, 2024
Regression: Downloaded files stay in tmp, not in intended location #12775
Closed
4 tasks
@alperozturk96
Copy link
Collaborator Author

Duplication of

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

@tobiasKaminsky tobiasKaminsky Awaiting requested review from tobiasKaminsky

@JonasMayerDev JonasMayerDev Awaiting requested review from JonasMayerDev

@ZetaTom ZetaTom Awaiting requested review from ZetaTom

@Unpublished Unpublished Awaiting requested review from Unpublished

Assignees
No one assigned
Labels
3. to review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@alperozturk96 @Unpublished