Cleanup federation phase 2#32113
Conversation
3ec3a84 to
4cdc45c
Compare
Codecov Report
@@ Coverage Diff @@
## master #32113 +/- ##
============================================
+ Coverage 63.51% 63.91% +0.39%
+ Complexity 18543 18536 -7
============================================
Files 1169 1169
Lines 69722 69808 +86
Branches 1267 1267
============================================
+ Hits 44286 44615 +329
+ Misses 25067 24824 -243
Partials 369 369
Continue to review full report at Codecov.
|
4cdc45c to
979b389
Compare
ec75c39 to
a59c078
Compare
dfb1f2b to
440d9c0
Compare
461c774 to
e2a8ffc
Compare
e2a8ffc to
18b0d02
Compare
PVince81
left a comment
PVince81
left a comment
There was a problem hiding this comment.
👍 looks good to me
see comments for optional changes
| if ($ownerFederatedId === null) { | ||
| $ownerFederatedId = $owner . '@' . $this->cleanupRemote($remote); | ||
| $this->assertIncomingSharingEnabled(); | ||
| $remote = $this->request->getParam('remote', null); |
There was a problem hiding this comment.
shouldn't we specify those as method params instead?
There was a problem hiding this comment.
probably yes, but I did not test it this way
| if (!$share->getPermissions() | ~Constants::PERMISSION_SHARE) { | ||
| $reSharingAllowed = $share->getPermissions() & Constants::PERMISSION_SHARE; | ||
| if (!$reSharingAllowed) { | ||
| return new Result(null, Http::STATUS_BAD_REQUEST); |
There was a problem hiding this comment.
should we add a message here for clarity ?
There was a problem hiding this comment.
Validate all error statuses and messages against the spec is another topic that I don't want to start in this PR
It has enough modifications to be barely readable :)
There was a problem hiding this comment.
indeed, I remember chatting about this with you. let's do this separately
| * @return void | ||
| */ | ||
| public function createShare($shareWith, | ||
| $remote, |
There was a problem hiding this comment.
I guess there's no way currently to pack all this into a nice object ? array doesn't sound that attractive and IShare is likely not suitable.
Just thinking out loud... let's leave this for now
There was a problem hiding this comment.
yes, it can be the IShare object with some additional params
| * | ||
| * @return void | ||
| */ | ||
| public function unshare($id, $token) { |
There was a problem hiding this comment.
reShare vs unshare, why not unShare then ? I guess you just copied the inconsistent names from the original handler :-D
| * @return int | ||
| */ | ||
| public function addShare($remote, $token, $name, $owner, $shareWith, $remoteId) { | ||
| \OC_Util::setupFS($shareWith); |
There was a problem hiding this comment.
assuming $shareWith was already validated before as being an existing user ?
I checked setupFS and it seems it wouldn't throw an exception.
If this is from the old code as is, feel free to leave it for now
There was a problem hiding this comment.
|
@PVince81 let's merge it for now, I'll put a checkbox into the task to address the comments in the next PR. |
|
Stable10: #33027 |
2 participants
Description
Follow up on #32086
Related Issue
https://github.com/owncloud/enterprise/issues/2564
Motivation and Context
Extract business logic from controller, make it reusable.
How Has This Been Tested?
Testing in progress... basic tests are:
Case 1 ✔️
expectations:
[x] owner see that file is shared
[x] recipient see a file and can reshare
expectations:
[x] recipient can NOT reshare any more
expectations:
[x] owner see that file is NOT shared
Case 2 ✔️
expectations:
[x] another user see a reshare
expectations:
[x] recipient can NOT see the file any more
[x] another user can NOT see a reshare
Case 3 ✔️
expectations:
[x] owner see that file is NOT shared
Types of changes
Checklist:
Open tasks: