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

Feature/copy #3749

Merged
merged 1 commit into from
Jul 10, 2020
Merged

Feature/copy #3749

merged 1 commit into from
Jul 10, 2020

Conversation

LukasHirt
Copy link
Collaborator

@LukasHirt LukasHirt commented Jul 8, 2020

Description

We've added copy action to the files list. It is possible now to copy folders and files.
The copy is executed via location picker.

Related Issue

How Has This Been Tested?

  • acceptance tests

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

Open tasks:

  • Rebase after Feature/move #3739 gets merged (might be needed to clean up a bit commits because they got mixed during reading onto the mentioned branch)

@LukasHirt LukasHirt added the Category:Enhancement Add new functionality label Jul 8, 2020
@LukasHirt LukasHirt self-assigned this Jul 8, 2020
@LukasHirt
Copy link
Collaborator Author

@kulmann @PVince81 I think the review will be easier after #3739 gets merged because currently both changes are mixed in here.

@LukasHirt
Copy link
Collaborator Author

PR rebased and ready for review

| "question?" | "folder-with-question?" |
| "&and#hash" | "folder-with-&and#hash" |

@skipOnOCIS @issue-3755
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

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

Looks good overall.

See my comment about "nice to have" test

apps/files/src/components/FilesAppBar.vue Show resolved Hide resolved
isRowDisabled(resource) {
const isBeingMoved = this.resources.some(item => item === resource.path)

return resource.type !== 'folder' || !resource.canCreate() || isBeingMoved
Copy link
Contributor

Choose a reason for hiding this comment

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

nice, so your are indeed checking the perm on the target folder 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

would be good to add a test for the case(s) where a target folder is not targettable due to missing permissions (received read-only share)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've opened a ticket for collecting the next scenarios to test for copy and move. Should I add this in there or implement it already in this PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

later is fine, please label the ticket "QA-team" and ping Artur

Copy link
Contributor

Choose a reason for hiding this comment

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

later... but within the sprint would be good because else I don't think we can consider the task "done"

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.

Really nice! 🚀 🎆

@@ -139,6 +140,12 @@ module.exports = {
*/
move: function(target) {
Copy link
Member

Choose a reason for hiding this comment

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

Just seeing that the target param is not used in the function body, so you could remove it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Must have overlooked this. Nice catch!

Fix promise and properly handle errors

Add acceptance tests

Merge move and copy in locationPicker tests

Add batch copy

Add copy file action in tests

Fix copy error message

Add permissions check for copy in public links

Add permissions check for batch copy action in public links

Fix copy file action in tests

Fix error message in copy tests

Add changelog

Add translations context and adjust guides for copy

Skip public links test in oCIS

Remove target from move
@LukasHirt LukasHirt merged commit da4a85b into master Jul 10, 2020
@LukasHirt LukasHirt deleted the feature/copy branch July 10, 2020 08:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category:Enhancement Add new functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Multi-select for copy/move Copy files to another location
3 participants