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.

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

Merge events by user and file #12

Merged
merged 1 commit into from
Mar 7, 2017
Merged

Merge events by user and file #12

merged 1 commit into from
Mar 7, 2017

Conversation

nickvergessen
Copy link
Member

@jGleitz would be nice if you are willing to test this.
Just download the file from https://raw.githubusercontent.com/nextcloud/files_downloadactivity/1afc446b61cc24418dd476db8e95ddf9899882a5/lib/Activity/Provider.php
and replace your apps/files_downloadactivity/lib/Activity/Provider.php file with this new version.

Sample

activity_-nextcloud-_2017-03-07_17 13 53

Fix #11

Signed-off-by: Joas Schilling <coding@schilljs.com>
@jGleitz
Copy link

jGleitz commented Mar 7, 2017

Wow, this was really fast!

The PR generally works as I would expect.

I however noticed that it never merges more than 5 file into one activity log. If a user dowloaded n files, I get ceil(n / 5) entries in the activity view.

I don’t have an activity where a file was downloaded by more than 5 users before something different happened, so I can’t say if the same behaviour applies for users.

@jGleitz
Copy link

jGleitz commented Mar 7, 2017

I just discovered that the same applies for any other activity in the activity log. In older versions of own-/Nextcloud, more than five files where truncated to “and others”. This doesn’t seem to happen anymore, more that five files simply get split into more entries.

So I guess the behaviour isn’t a bug of this PR.

See nextcloud/activity#122

@nickvergessen
Copy link
Member Author

nickvergessen commented Mar 7, 2017

Yeah, it's a code design decision in the servers code:
https://github.com/nextcloud/server/blob/da9468522b9e846d10b6e91ad10fa0c1b3b99546/lib/private/Activity/EventMerger.php#L225-L226

I dropped the "others"-tooltip, because otherwise we would have needed another API endpoint for the desktop and mobile clients, since they can't use tooltips like we do.

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

Successfully merging this pull request may close these issues.

None yet

3 participants