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

[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
+809 −3
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions tests/acceptance/bootstrap/SpacesContext.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2065,9 +2065,9 @@ public function userHasCopiedOrMovedFileInsideSpaceUsingFileId(
$sourceFile = \explode("/", $sourceFile);
$sourceFile = \end($sourceFile);
$destinationFile = \trim($destinationFile, "/");
if ($actionType === 'copied' || $actionType === 'moved') {
if ($actionType === 'copied') {
$fileDestination = $this->escapePath($destinationFile) . '/' . $this->escapePath($sourceFile);
} else {
} elseif ($actionType === 'renamed' || $actionType === 'moved') {
Copy link
Member

@saw-jan saw-jan Oct 25, 2024 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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:

ocis/tests/acceptance/features/coreApiFavorites/favorites.feature

Line 154 in 5fc891c

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>"

$fileDestination = $destinationFile;
}

Expand Down
Loading