-
Notifications
You must be signed in to change notification settings - Fork 68
feat: Add sharing activity for teams #1754
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
Conversation
b5c753a to
0efac00
Compare
Activity
|
||||||||||||||||||||||||||||||||||||||||
| Project |
Activity
|
| Branch Review |
feat/activity-for-team-share
|
| Run status |
|
| Run duration | 01m 16s |
| Commit |
|
| Committer | Ferdinand Thiessen |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
|
|
2
|
|
|
0
|
|
|
0
|
|
|
8
|
|
|
0
|
| View all changes introduced in this branch ↗︎ | |
Tests for review
sidebar.cy.ts • 1 failed test • Run E2E
| Test | Artifacts | |
|---|---|---|
| Check activity listing in the sidebar > Has creation activity |
Test Replay
Screenshots
|
|
settings.cy.ts • 1 failed test • Run E2E
| Test | Artifacts | |
|---|---|---|
| Check that user's settings survive a reload > Form survive a reload |
Test Replay
Screenshots
|
|
miaulalala
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
b69f982 to
2295c86
Compare
|
TIL: |
2295c86 to
0e06c4f
Compare
0e06c4f to
af98881
Compare
This adds support for Teams (previously "Circles"), the user notification already work, but the notification for the owner and sharer need support in the `files_sharing` app. Signed-off-by: Ferdinand Thiessen <[email protected]>
af98881 to
af19269
Compare
|
/backport to stable31 |
| $users = array_map(fn (IUser $user) => $user->getUID(), $users); | ||
| $shouldFlush = $this->startActivityTransaction(); | ||
| while (!empty($users)) { | ||
| $this->addNotificationsForGroupUsers($users, $actionUser, $share->getNodeId(), $share->getNodeType(), $share->getTarget(), (int)$share->getId()); | ||
| $userIds = array_map(fn (IUser $user) => $user->getUID(), $users); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$users contains string[] after line 931, then you try to convert to string again.
Results in:
Argument #1 ($user) must be of type OCP\IUser, string given in file 'server/server/apps/activity/lib/FilesHooks.php' line 935
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh seems like a rebase issue or similar, line 931 should be removed...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(or remove 934 to keep the activity transaction as short as possible)
This adds support for Teams (previously "Circles"), the user notification already work, but the notification for the owner and sharer need support in the
files_sharingapp.So if this is merged we can add the required providers in
files_sharing.But one thing to mention: For groups we paginate the members, I did not find a nice way to do this with teams.
So maybe @ArtificialOwl can have a look at the member query if there is something to improve performance wise (e.g. pagination?).