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

[tests-only][full-ci] add test to check activities of destination file and folder by moving a file using file-id #10409

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

prashant-gurung899
Copy link
Contributor

Description

This PR adds tests to check activities of destination file and folder after moving a file using it's file-id into a folder.

Scenario: check activities of destination folder after moving a file into it using file-id
Scenario: check activities of destinaton file and folder after moving a 0 byte file using file-id
Scenario: check activities of destinaton file and folder after moving a file by renaming destination file using file-id
Scenario: check activities of destinaton file and folder after moving a 0 byte file by renaming destination file using file-id

Related Issue

Motivation and Context

How Has This Been Tested?

  • locally
  • CI

Screenshots (if appropriate):

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 only (no source changes)

Checklist:

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

Copy link

sonarcloud bot commented Oct 24, 2024

$fileDestination = $this->escapePath($destinationFile) . '/' . $this->escapePath($sourceFile);
} else {
} elseif ($actionType === 'renamed' || $actionType === 'moved') {
Copy link
Member

@saw-jan saw-jan Oct 25, 2024

Choose a reason for hiding this comment

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

is it required? and why

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, because for following scenario we have to directly rename the file during the MOVE request:

Scenario: check activities of destinaton file and folder after moving a file by renaming destination file using file-id

In this particular step:

And user "Alice" has moved file "textfile.txt" into "FOLDER/renamed.txt" inside space "Personal" using file-id "<<FILEID>>" 

If we didn't do it then it would return 204 instead of 201
See: https://drone.owncloud.com/owncloud/ocis/40432/38/5

cc @saw-jan

Copy link
Member

Choose a reason for hiding this comment

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

hmm, I think there should be another better way. I know these steps are confusing.

Copy link
Contributor

@anon-pradip anon-pradip Oct 29, 2024

Choose a reason for hiding this comment

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

Actually, what's happening is that:
What we already have is:

...
if ($actionType === 'copied' || $actionType === 'moved') {
			$fileDestination = $this->escapePath($destinationFile) . '/' . $this->escapePath($sourceFile);
		} else {
			$fileDestination = $destinationFile;
		}
...

And for the steps like:

    And user "Alice" has copied file "textfile.txt" into "newFolder" inside space "Personal" using file-id "<<FILEID>>"

we are not appending the file name after the container i.e., folder name (like this: newFolder/textfile.txt). So, it gives:

$fileDestination: "newFolder/textfile.txt" and
$headers: Destination="https://localhost:9200/remote.php/dav/spaces/98b0f059-cf07-4bac-8528-bcaed059edcb$6ab70a1e-4a57-4542-9f76-eb9982a37da5/newFolder/textfile.txt"

But for scenarios like:

    And user "Alice" has moved file "textfile.txt" into "New Folder/textfile.txt" inside space "Personal" using file-id "<<FILEID>>"

we are already appending the file name after the container i.e., folder name (like this: New Folder/textfile.txt). So it gives:

$fileDestination = "New%20Folder/textfile.txt/textfile.txt" and
$headers: Destination="https://localhost:9200/remote.php/dav/spaces/98b0f059-cf07-4bac-8528-bcaed059edcb$6826391a-448e-42b1-9ef4-cfb39c0c4ac4/New%20Folder/textfile.txt/textfile.txt"

which is not expected.

So
EITHER
we can modify the copy steps; appending the file name after container and we can totally omit above mentioned code snippet
OR
we can modify the move steps; removing the file name after container and we can have the existing above mentioned code snippet
OR
we can go with changes made in this PR
OR we should dig more to find another solution

cc @saw-jan

Copy link
Contributor

@anon-pradip anon-pradip Oct 29, 2024

Choose a reason for hiding this comment

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

I think, modifying the copy steps and omitting the snippet :

if ($actionType === 'copied' || 'moved') {
			$fileDestination = $this->escapePath($destinationFile) . '/' . $this->escapePath($sourceFile);
		} else {
			$fileDestination = $destinationFile;
		}

will be a better choice because in other test cases also, the file name is appended after its container like:

And user "Alice" has copied file "/textfile0.txt" to "/subfolder/textfile1.txt"

And also there is only one scenario for copy using the method (userHasCopiedOrMovedFileInsideSpaceUsingFileId) that contains above snippet.

Copy link
Member

Choose a reason for hiding this comment

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

IMO, the scenario steps need to be written clearly.
Example:

# append source <file> to the destination path (<folder>/<file>)
... (copies|moves) a file "<file>" into folder "<folder>" inside space "<space>"

# do not append source <file> to the destination path (<newfile>)
... (copies|moves|renames) a file "<file>" to "<newfile>" inside space "<space>"

# do not append source <file> to the destination path (<folder>/<newfile>)
... (copies|moves|renames) a file "<file>" to "<folder>/<newfile>" inside space "<space>"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants