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.

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

Fix restore files full path #6938

Closed
wants to merge 2 commits into from
Closed

Fix restore files full path #6938

wants to merge 2 commits into from

Conversation

lookacat
Copy link
Contributor

@lookacat lookacat commented May 11, 2022

Description

See #1753

! SEE LAST COMMENT PLEASE !

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:

@update-docs
Copy link

update-docs bot commented May 11, 2022

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

@lookacat lookacat changed the title WIP Fix restore files full path May 11, 2022
@lookacat lookacat force-pushed the fix-restore-file-path branch from b48833a to 8e0ca21 Compare May 11, 2022 12:32
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

85.7% 85.7% Coverage
0.0% 0.0% Duplication

@lookacat lookacat marked this pull request as ready for review May 11, 2022 12:52
@lookacat
Copy link
Contributor Author

Please see last comment in the linked issue, there is one 'edge' case that I have no idea how to fix :/

@lookacat lookacat requested review from kulmann and JammingBen May 11, 2022 12:54
@ownclouders
Copy link
Contributor

Results for oC10SharingAndTrashbin https://drone.owncloud.com/owncloud/web/25511/34/1

💥 The acceptance tests failed on retry. Please find the screenshots inside ...

webUITrashbinRestore-trashbinRestore_feature-L138.png

webUITrashbinRestore-trashbinRestore_feature-L138.png

webUITrashbinRestore-trashbinRestore_feature-L181.png

webUITrashbinRestore-trashbinRestore_feature-L181.png

webUITrashbinRestore-trashbinRestore_feature-L260.png

webUITrashbinRestore-trashbinRestore_feature-L260.png

💥 The acceptance tests pipeline failed. The build has been cancelled.

Comment on lines +65 to +67
for (const folder of foldersToRestore) {
await this.$client.files.createFolder(buildWebDavFilesPath(this.user.id, folder))
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This will not work with deep nested folders yet. Let's say you have the following folder structure: level1/level2/level3. foldersToRestore would be an array including all folders: ['level1', 'level2', 'level3']. By just looping through them you will create those in a flat hierarchy.

You will have to add the created folders to the path you pass to createFolder(). E.g. first call createFolder('level1'), next createFolder('level1/level2') etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah yes you are right, but want to make sure the edge case commented in the issue is cleared, do you maybe have an idea/understand what the problem there is :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I've commented in #1753 (comment). Quiet a nasty edge case :(

@lookacat lookacat removed the request for review from kulmann May 11, 2022 13:17
@JammingBen
Copy link
Contributor

@lookacat Can this be closed because of #7635?

@lookacat lookacat closed this Oct 27, 2022
@kulmann kulmann deleted the fix-restore-file-path branch September 5, 2024 19:56
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.

3 participants