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

Check if sync item is actually a folder before processing it #6780

Merged
merged 6 commits into from
Jul 5, 2024
Merged

Check if sync item is actually a folder before processing it #6780

merged 6 commits into from
Jul 5, 2024

Conversation

camilasan
Copy link
Member

@camilasan camilasan commented May 24, 2024 •
edited
Loading

  • The rename check was not happening since since it was hitting "Move without permission to rename base file"
  • The file was then processed as a new file

Possible workaround for #6549 and #4958

@camilasan camilasan requested a review from mgallien June 5, 2024 13:19
@camilasan camilasan changed the title When files are in groupfolders, rename check is done too late. Before processing rename, check if file is already in the database Jun 5, 2024
@camilasan camilasan force-pushed the bugfix/groupfolder-rename branch 2 times, most recently from 651e8ee to 0a7732f Compare June 5, 2024 13:22
@camilasan camilasan requested a review from claucambra June 5, 2024 13:22
@camilasan camilasan marked this pull request as ready for review June 5, 2024 13:22
@camilasan camilasan force-pushed the bugfix/groupfolder-rename branch 3 times, most recently from 00caa76 to 0831f1e Compare June 5, 2024 13:28
@camilasan camilasan changed the title Before processing rename, check if file is already in the database Check if file is already in database before processing rename Jun 5, 2024
mgallien
mgallien requested changes Jun 5, 2024
View reviewed changes
Copy link
Collaborator

@mgallien mgallien left a comment

Choose a reason for hiding this comment

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

please add automated tests
without automated tests, I am not sure I can understand the effect this will have

src/libsync/discovery.cpp Outdated Show resolved Hide resolved
mgallien
mgallien requested changes Jun 24, 2024
View reviewed changes
Copy link
Collaborator

@mgallien mgallien left a comment

Choose a reason for hiding this comment

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

see my comment

test/testsyncmove.cpp Show resolved Hide resolved
@camilasan
Copy link
Member Author

/backport to stable-3.13

@camilasan camilasan modified the milestone: 3.13.1 Jun 25, 2024
@camilasan camilasan force-pushed the bugfix/groupfolder-rename branch 2 times, most recently from 5e11661 to 008d531 Compare June 25, 2024 14:52
@camilasan camilasan changed the title Check if file is already in database before processing rename Check if sync item is actually a folder before processing it Jun 28, 2024
mgallien
mgallien requested changes Jul 5, 2024
View reviewed changes
src/libsync/discovery.cpp Outdated Show resolved Hide resolved
@camilasan
Check if file is already in database before processing rename.
591808f 
- The rename check was not happening since it was hitting error "Move without permission to rename base file"
- The file was then processed as a new file
- Also check #6535.

Possible workaround for #6549.

Signed-off-by: Camila Ayres <[email protected]>
@camilasan
Add test to detect file renames in external storage.
4afc033 
Signed-off-by: Camila Ayres <[email protected]>
@camilasan
Improve logs for rename check.
9b2780c 
Signed-off-by: Camila Ayres <[email protected]>
@camilasan
Remove commented out code.
0965dea 
Signed-off-by: Camila Ayres <[email protected]>
@camilasan
Only check for external storage permissions when it is a folder.
e38a9f1 
Signed-off-by: Camila Ayres <[email protected]>
@camilasan
Fix renaming tests for files under group folders.
0160cec 
Signed-off-by: Camila Ayres <[email protected]>
@nextcloud-desktop-bot
Copy link

AppImage file: nextcloud-PR-6780-0160cec256ed72736a3ef83228aaea7918d82e2f-x86_64.AppImage

To test this change/fix you can simply download above AppImage file and test it.

Please make sure to quit your existing Nextcloud app and backup your data.

@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Jul 5, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
E Maintainability Rating on New Code (required ≥ A)
15 New Code Smells (required ≤ 0)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

@mgallien mgallien merged commit 25acde4 into master Jul 5, 2024
12 of 14 checks passed
@mgallien mgallien deleted the bugfix/groupfolder-rename branch July 5, 2024 11:57
@backportbot backportbot bot mentioned this pull request Jul 5, 2024
Merged
@joshtrichards joshtrichards mentioned this pull request Aug 14, 2024
Open
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mgallien mgallien mgallien approved these changes

@claucambra claucambra Awaiting requested review from claucambra

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@camilasan @nextcloud-desktop-bot @mgallien