Show pending remote shares at the 'Shared with you' tab #37022
Conversation
|
Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes. |
VicDeo
force-pushed
the
bugfix/3793
branch
8 times, most recently
from
adf5f9d
to
450ceb2
Compare
Codecov Report
@@ Coverage Diff @@
## master #37022 +/- ##
============================================
+ Coverage 64.78% 64.85% +0.06%
- Complexity 19130 19135 +5
============================================
Files 1267 1267
Lines 74912 74893 -19
Branches 1328 1331 +3
============================================
+ Hits 48533 48573 +40
+ Misses 25989 25928 -61
- Partials 390 392 +2
Continue to review full report at Codecov.
|
VicDeo
force-pushed
the
bugfix/3793
branch
5 times, most recently
from
eab3959
to
62f732c
Compare
VicDeo
force-pushed
the
bugfix/3793
branch
2 times, most recently
from
10ac1ed
to
3ce34fe
Compare
VicDeo
changed the title
| $server->getEventDispatcher(), | ||
| $uid | ||
| ), | ||
| $uid |
There was a problem hiding this comment.
I'd prefer to inject the userSession object in order to follow what has been done in other places, but taking into account that the "Manager" being injected uses the uid, it might be problematic: if the userSession changes, the manager would still use the previous user id.
In addition, I'm not sure how it will behave under this "change the user session" scenario with the Filesystem objects being injected.
Worst case, we could end up with a static controller, but being a controller it might not be a big problem
There was a problem hiding this comment.
tried that, not possible - everything breaks down.
| } | ||
|
|
||
| // Make sure the user has no notification for something that does not exist anymore. | ||
| $this->externalManager->processNotification((int) $id); |
There was a problem hiding this comment.
At the moment, it's a bit unclear if the processNotification will be called (or should be called) if the share is accepted.
If the function is automatically called after the share is accepted, include a code comment here.
There was a problem hiding this comment.
Make sure the user has no notification for something that does not exist anymore.
There was a problem hiding this comment.
when the share is accepted we don't care about notification.
| } | ||
|
|
||
| // Make sure the user has no notification for something that does not exist anymore. | ||
| $this->externalManager->processNotification((int) $id); |
There was a problem hiding this comment.
same as above. this is a sanity measure 'for something that does not exist anymore'
| * @return Result | ||
| */ | ||
| public function getAllShares() { | ||
| return $this->getShares(true); |
There was a problem hiding this comment.
Technically, I'd prefer to have a private method such as getSharesPrivate($includePending=false) in order to make both functions getShares and getAllShares to explicitly return a Result.
| * @throws \Exception | ||
| */ | ||
| protected function getFileInfo($mountpoint) { | ||
| $view = new View('/' . $this->uid . '/files/'); |
There was a problem hiding this comment.
Any better alternative? Maybe we can inject an IRootFolder and get the info from there? This might be a problem for testing.
There was a problem hiding this comment.
Not a problem for testing as this method was added exactly to make testing possible.
Moreover, RemoteOcsController has coverage 95.23% and is based on the code from a deleted apps/files_sharing/lib/API/Remote.php file that had 0% coverage.
Description
Shared with youtabRelated Issue
Motivation and Context
Shared with should include pending remote shares
How Has This Been Tested?
shared with youtab and accept/decline themScreenshots (if appropriate):
Types of changes
Checklist: