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

[full-ci] Enhancement: Implement Copy/Move Conflict Dialog #6994

Merged
merged 7 commits into from
Jun 3, 2022
Merged

[full-ci] Enhancement: Implement Copy/Move Conflict Dialog #6994

merged 7 commits into from
Jun 3, 2022

Conversation

lookacat
Copy link
Contributor

@lookacat lookacat commented May 17, 2022 •
edited by pascalwengerter
Loading

@update-docs
Copy link

update-docs bot commented May 17, 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 force-pushed the enhancement-copy-move-conflict-dialog branch 4 times, most recently from e769221 to 43db91a Compare May 20, 2022 09:18
@lookacat
Copy link
Contributor Author

Needs owncloud/owncloud-design-system#2149 to be linked

@lookacat lookacat force-pushed the enhancement-copy-move-conflict-dialog branch 2 times, most recently from 703313d to 740349b Compare May 20, 2022 12:02
kulmann
kulmann requested changes May 20, 2022
View reviewed changes
Copy link
Member

@kulmann kulmann left a comment

Choose a reason for hiding this comment

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

I like where this is going 🤩 some conceptual questions in the comments.

packages/web-app-files/src/views/Personal.vue Outdated Show resolved Hide resolved
packages/web-app-files/src/helpers/resource/move.ts Show resolved Hide resolved
packages/web-app-files/src/helpers/resource/move.ts Outdated Show resolved Hide resolved
@lookacat lookacat force-pushed the enhancement-copy-move-conflict-dialog branch from f9bdf99 to c14b6d7 Compare May 27, 2022 08:46
@lookacat lookacat marked this pull request as ready for review May 27, 2022 08:56
@ownclouders
Copy link
Contributor

ownclouders commented May 30, 2022 •
edited
Loading

Results for oC10Files2 https://drone.owncloud.com/owncloud/web/25998/18/1

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

webUIFilesSearch-search_feature-L178.png

webUIFilesSearch-search_feature-L178.png

@lookacat lookacat changed the title Enhancement: Implement Copy/Move Conflict Dialog [full-ci] Enhancement: Implement Copy/Move Conflict Dialog May 30, 2022
@lookacat lookacat force-pushed the enhancement-copy-move-conflict-dialog branch from 8090c4a to 2018ce0 Compare May 30, 2022 13:02
@lookacat lookacat requested a review from kulmann May 30, 2022 13:28
@lookacat lookacat force-pushed the enhancement-copy-move-conflict-dialog branch from d103b4b to 4a4d57e Compare June 2, 2022 12:00
pascalwengerter
pascalwengerter requested changes Jun 2, 2022
View reviewed changes
Copy link
Contributor

@pascalwengerter pascalwengerter left a comment

Choose a reason for hiding this comment

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

Works fine and is a great improvement!

Only suggestion I have is a differentiation when it's only one file (left) - either use ngettext and a separate translation, or remove the checkbox entirely?

Screenshot 2022-06-02 at 13 07 30

changelog/unreleased/enhancement-copy-move-conflict-dialog Outdated Show resolved Hide resolved
@lookacat
Copy link
Contributor Author

lookacat commented Jun 2, 2022

Checkbox for single conflict is removed, good idea 👍🏼

@lookacat lookacat force-pushed the enhancement-copy-move-conflict-dialog branch from 25af14e to 12150d0 Compare June 2, 2022 12:45
@pascalwengerter pascalwengerter force-pushed the enhancement-copy-move-conflict-dialog branch from 12150d0 to 6a65224 Compare June 2, 2022 13:55
pascalwengerter
pascalwengerter approved these changes Jun 2, 2022
View reviewed changes
Copy link
Contributor

@pascalwengerter pascalwengerter left a comment

Choose a reason for hiding this comment

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

I think it's good to merge now from my POV

packages/web-runtime/package.json Outdated Show resolved Hide resolved
@lookacat @pascalwengerter
WIP
58b0f92
@lookacat @pascalwengerter
Make basic conflict resolving possible / Refactor drag & drop logic
f96eb13
@lookacat @pascalwengerter
Add merge / replace button
2661d7c
@lookacat @pascalwengerter
Implement keep both button
12b6c2f 
Add translations, fix minor issues

Bump SDK

Remove "merge" folders

Add changelog, Bump ODS

Fix yarn.lock

Fix existing unittests

Add new unittests

Bump ODS

Set list files request depth to 1

Update changelog, Remove checkbox for single conflict
@pascalwengerter pascalwengerter force-pushed the enhancement-copy-move-conflict-dialog branch 2 times, most recently from 86c9530 to 11a5804 Compare June 2, 2022 16:29
@pascalwengerter pascalwengerter force-pushed the enhancement-copy-move-conflict-dialog branch from 11a5804 to d187d8b Compare June 2, 2022 16:30
@kulmann kulmann mentioned this pull request Jun 3, 2022
Closed
25 tasks
@sonarcloud
Copy link

sonarcloud bot commented Jun 3, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 3 Code Smells

53.1% 53.1% Coverage
0.0% 0.0% Duplication

kulmann
kulmann requested changes Jun 3, 2022
View reviewed changes
Copy link
Member

@kulmann kulmann left a comment

Choose a reason for hiding this comment

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

Found one issue when moving inside subfolders of the respective webdav root (see comment).
Potential followups:

  • trigger the conflict dialog for copy & move from location picker (only create followup ticket please. implementation would probably be too hacky right now)
  • repeating the same action again leads to naming conflicts that are not handled: (1) move "asdf" to "target/asdf" and select "keep both" -> creates "target/asdf copy" when "target/asdf" already existed before. (2) again, move "asdf" to "target/asdf" and select "keep both" -> tries to create "target/asdf copy", but fails because it already exists from previous move with "keep both".

packages/web-app-files/src/helpers/resource/move.ts Outdated
// Collect all conflicting resources
const allConflicts = []
for (const resource of resourcesToMove) {
const potentialTargetWebDavPath = join(webDavPrefix, targetFolder.path, resource.path)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const potentialTargetWebDavPath = join(webDavPrefix, targetFolder.path, resource.path)
const potentialTargetWebDavPath = join(webDavPrefix, targetFolder.path, resource.name)

kulmann
kulmann approved these changes Jun 3, 2022
View reviewed changes
Copy link
Member

@kulmann kulmann left a comment

Choose a reason for hiding this comment

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

Nice!

@kulmann kulmann merged commit 7759ce4 into master Jun 3, 2022
@delete-merged-branch delete-merged-branch bot deleted the enhancement-copy-move-conflict-dialog branch June 3, 2022 10:02
@lookacat lookacat mentioned this pull request Jun 3, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pascalwengerter pascalwengerter pascalwengerter approved these changes

@kulmann kulmann kulmann approved these changes

@fschade fschade Awaiting requested review from fschade

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

Successfully merging this pull request may close these issues.
4 participants
@lookacat @ownclouders @kulmann @pascalwengerter