X Tutup
Skip to content

fix: implement missing getSize method in SharedFile.php#36778

Merged
phil-davis merged 2 commits intomasterfrom
bugfix/filesize-webdav-public-api
Jan 24, 2020
Merged

fix: implement missing getSize method in SharedFile.php#36778
phil-davis merged 2 commits intomasterfrom
bugfix/filesize-webdav-public-api

Conversation

@C0rby
Copy link
Contributor

@C0rby C0rby commented Jan 17, 2020 •
edited
Loading

Description

Implement the method getSize in SharedFile.php

Related Issue

Motivation and Context

The webdav-api for public files couldn't get the correct file size
since SharedFile.php did not implement the getSize method.

How Has This Been Tested?

To test execute a PROPFIND request for a shared file via the webdav public-files API
curl -s --user user:password --basic http://localhost:8080/remote.php/dav/public-files/HXEGrChNif8AWBq -X PROPFIND

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Database schema changes (next release will require increase of minor version instead of patch)
  • 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:
  • Changelog item, see TEMPLATE

@C0rby C0rby self-assigned this Jan 17, 2020
@claassistantio
Copy link

claassistantio commented Jan 17, 2020 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@mmattel
Copy link
Contributor

mmattel commented Jan 19, 2020

@phil-davis this might be of interest regarding tests.
Is this something that should be included in 10.4 or must wait for a next release?

@mmattel mmattel requested review from micbar and phil-davis January 20, 2020 11:23
@mmattel
Copy link
Contributor

mmattel commented Jan 20, 2020

@phil-davis @micbar added you to the reviewers.

@phil-davis
Copy link
Contributor

phil-davis commented Jan 20, 2020

@micbar do we try and do this for 10.4?

If so, then could add an acceptance test to cover the expected PROPFIND response for such public link requests.

@owncloud owncloud deleted a comment from update-docs bot Jan 21, 2020
@phil-davis phil-davis force-pushed the bugfix/filesize-webdav-public-api branch from c3a5512 to 91e59ba Compare January 21, 2020 03:39
@phil-davis
Copy link
Contributor

phil-davis commented Jan 21, 2020 •
edited
Loading

squashed the commits and rebased
assigned myself to make some acceptance test

@phil-davis phil-davis assigned phil-davis and unassigned phil-davis Jan 21, 2020
@phil-davis
Copy link
Contributor

phil-davis commented Jan 23, 2020

Unassigned myself and added the QA-team label.
@dpakach is going to allocate someone to do acceptance test(s)

@dpakach dpakach assigned C0rby and haribhandari07 and unassigned C0rby Jan 23, 2020
phil-davis
phil-davis reviewed Jan 24, 2020
View reviewed changes
David Christofas and others added 2 commits January 24, 2020 16:29
@phil-davis
fix: implement missing getSize method in SharedFile.php
371fce6 
  The webdav-api for public files couldn't get the correct file size
since `SharedFile.php` did not implement the `getSize` method.

Fixes #36741
@jasson99 @phil-davis
Add acceptance test to get the size of the file
a20834d
@phil-davis phil-davis force-pushed the bugfix/filesize-webdav-public-api branch from 63034a6 to a20834d Compare January 24, 2020 10:44
@phil-davis phil-davis mentioned this pull request Jan 24, 2020
Closed
@phil-davis
Copy link
Contributor

phil-davis commented Jan 24, 2020

The acceptance tests here cover just the fixed getcontentlength in a PROPFIND
I raised issue #36821 to add tests to better cover data items returned by PROPFIND requests more generally.

@phil-davis phil-davis mentioned this pull request Jan 24, 2020
Closed
11 tasks
@phil-davis phil-davis merged commit a048f62 into master Jan 24, 2020
@delete-merged-branch delete-merged-branch bot deleted the bugfix/filesize-webdav-public-api branch January 24, 2020 12:06
@phil-davis phil-davis mentioned this pull request Jan 25, 2020
Merged
11 tasks
@owncloud owncloud deleted a comment from codecov bot Feb 17, 2020
@kiranparajuli589
Copy link
Contributor

kiranparajuli589 commented Feb 17, 2020 •
edited
Loading

I raised issue #36821 to add tests to better cover data items returned by PROPFIND requests more generally.

@phil-davis I donot understand properly, what sorts of data-items you intend to? Please elaborate here so that I could add some scenarios...

@phil-davis
Copy link
Contributor

phil-davis commented Feb 20, 2020

Some stuff I can see:
WebDavPropertiesContext.php

	 * @When user :username gets the following properties of file/folder :path using the WebDav PropFind API
public function userGetsFollowingPropsWithNamespaceOfFileUsingWebDavAPI(...

That seems to be unused. It calls WebDavHelper::propfindWithMultipleProps
Maybe that is a useful step and method?

There is also:

	 * @When /^user "([^"]*)" gets the following properties of (?:file|folder|entry) "([^"]*)" using the WebDAV API$/
public function userGetsPropertiesOfFolder(...

What is the difference of these 2 methods? Why do we have 2 methods?

@phil-davis
Copy link
Contributor

phil-davis commented Feb 20, 2020 •
edited
Loading

We have tests that check properties like:
apiComments/comments.feature checks properties like oc:comments-href oc:comments-count oc:comments-unread
apiShareOperations/getWebDAVSharePermissions.feature checks ocs:share-permissions
Other feature files check d:lockdiscovery attributes, d:resourcetype attributes, oc:share-types, oc:privatelink, d:quota-available-bytes

There are probably quite a few "interesting" other properties returned.

e.g. https://doc.owncloud.com/server/developer_manual/webdav_api/comments.html

There are lots of properties returned about a comment. We can make some API test scenarios that verify that the properties do come...

And for the other APIs that return lists of properties, including the "general" webDAV API.

@kiranparajuli589
Copy link
Contributor

kiranparajuli589 commented Feb 28, 2020 •
edited
Loading

Some stuff I can see:
WebDavPropertiesContext.php

	 * @When user :username gets the following properties of file/folder :path using the WebDav PropFind API
public function userGetsFollowingPropsWithNamespaceOfFileUsingWebDavAPI(...

That seems to be unused. It calls WebDavHelper::propfindWithMultipleProps
Maybe that is a useful step and method?

There is also:

	 * @When /^user "([^"]*)" gets the following properties of (?:file|folder|entry) "([^"]*)" using the WebDAV API$/
public function userGetsPropertiesOfFolder(...

What is the difference of these 2 methods? Why do we have 2 methods?

@phil-davis Only difference between these two methods is they use different depth level for PROPFIND requests and different versions of davpath.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@phil-davis phil-davis phil-davis approved these changes

@PVince81 PVince81 Awaiting requested review from PVince81

@DeepDiver1975 DeepDiver1975 Awaiting requested review from DeepDiver1975

@micbar micbar Awaiting requested review from micbar

+1 more reviewer

@jasson99 jasson99 jasson99 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@C0rby C0rby

@jasson99 jasson99

Labels

4 - To release dev:acceptance-tests QA:team

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Webdav public files API issue with file and folder sizes

7 participants

@C0rby @claassistantio @mmattel @phil-davis @kiranparajuli589 @jasson99 @haribhandari07
X Tutup