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

[full-ci] Refactor copy/move, Bugfix: Inhibit move files between spaces and disable copy/move overwrite on self #7652

Merged
merged 20 commits into from
Oct 4, 2022

Conversation

lookacat
Copy link
Contributor

@lookacat lookacat commented Sep 19, 2022

Description

See #6892 (comment)

I also took the time to refactor the existing code.

Related Issue

Screenshots (if appropriate):

image

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

Open tasks:

@lookacat lookacat changed the title Bugfix: Inhibit move files between spaces, disable copy/move overwrite on self Refactor copy/move, Bugfix: Inhibit move files between spaces and disable copy/move overwrite on self Sep 19, 2022
@lookacat lookacat force-pushed the bugfix-move-files-between-spaces branch from 895be1c to dddee65 Compare September 25, 2022 21:50
@lookacat lookacat marked this pull request as ready for review September 25, 2022 21:50
@lookacat lookacat force-pushed the bugfix-move-files-between-spaces branch from db1f7fc to 67edb8a Compare September 25, 2022 21:59
@lookacat lookacat changed the title Refactor copy/move, Bugfix: Inhibit move files between spaces and disable copy/move overwrite on self [full-ci] Refactor copy/move, Bugfix: Inhibit move files between spaces and disable copy/move overwrite on self Sep 27, 2022
@lookacat lookacat force-pushed the bugfix-move-files-between-spaces branch from b2795ae to 7a7891c Compare September 29, 2022 13:42
@ownclouders
Copy link
Contributor

ownclouders commented Sep 30, 2022

Results for oCISWebdavLockProtection https://drone.owncloud.com/owncloud/web/28796/53/1

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

webUIWebdavLockProtection-move_feature-L59.png

webUIWebdavLockProtection-move_feature-L59.png

webUIWebdavLockProtection-move_feature-L60.png

webUIWebdavLockProtection-move_feature-L60.png

@lookacat lookacat force-pushed the bugfix-move-files-between-spaces branch 3 times, most recently from 86323aa to 45beec3 Compare September 30, 2022 13:32
@JammingBen
Copy link
Collaborator

JammingBen commented Oct 4, 2022

In general, I'm not too sure about the upload.ts helper. It feels a bit weird to inject so many "things". Also, we already have the useUploadHelpers composable. Maybe that would be a better fit?

Edit: The onFilesSelected could look something like this:

onFilesSelected(files: File[]) {
      const uppyResources: UppyResource[] = this.inputFilesToUppyFiles(files)
      const quotaExceeded = this.checkQuotaExceeded(uppyResources)
      if (quotaExceeded) {
        return this.$uppyService.clearInputs()
      }

      const filesToUpload = this.getFilesToUpload(...)
      if (!filesToUpload.length) {
        return this.$uppyService.clearInputs()
      }

      return this.handleUppyFileUpload(this.space, this.item, filesToUpload)
}

The methods checkQuotaExceeded and getFilesToUpload are in the uploadHelpersComposable. getFilesToUpload just wraps the conflict handling, but only returns the files to be uploaded.

.eslintrc.js Outdated Show resolved Hide resolved
@lookacat
Copy link
Contributor Author

lookacat commented Oct 4, 2022

@JammingBen the idea with the upload.ts was that we have all conflict resolving logic at the same place/same folder. When I refactored this it was all done in CreateAndUpload thats why i tried to extract it. But I can put it in composables if thats the better fit.

@JammingBen
Copy link
Collaborator

@JammingBen the idea with the upload.ts was that we have all conflict resolving logic at the same place/same folder. When I refactored this it was all done in CreateAndUpload thats why i tried to extract it. But I can put it in composables if thats the better fit.

Let's keep it that way for now 🙂 In the future we should refactor all that upload-qutoa-conflict-stuff, because it is spread all over the place. But that's something after GA I suppose.

@lookacat lookacat force-pushed the bugfix-move-files-between-spaces branch from d3f4448 to 3ebcbcc Compare October 4, 2022 10:37
@sonarcloud
Copy link

sonarcloud bot commented Oct 4, 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 7 Code Smells

54.0% 54.0% Coverage
0.0% 0.0% Duplication

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.

👍

@kulmann kulmann merged commit 985c55a into master Oct 4, 2022
@delete-merged-branch delete-merged-branch bot deleted the bugfix-move-files-between-spaces branch October 4, 2022 13:37
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.

Copy/move only works with personal space
5 participants