-
-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Allow getting the unread comment count for an entire folder at once #4146
Conversation
@icewind1991, thanks for your PR! By analyzing the history of the files in this pull request, we identified @nickvergessen, @blizzz and @LukasReschke to be potential reviewers. |
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.
Can we haz tests....
@rullzer you haz your test |
lib/private/Comments/Manager.php
Outdated
->innerJoin('c', 'filecache', 'f', $qb->expr()->andX( | ||
$qb->expr()->eq('c.object_type', $qb->createNamedParameter('files')), | ||
$qb->expr()->eq('f.fileid', $qb->createFunction( | ||
'cast(' . $qb->getColumnName('c.object_id') . ' as ' . $castAs . ')' |
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.
Please use $query->expr()->castColumn()
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.
Done.
@icewind1991 tests are failing... |
Signed-off-by: Robin Appelman <robin@icewind.nl>
Signed-off-by: Robin Appelman <robin@icewind.nl>
Signed-off-by: Robin Appelman <robin@icewind.nl>
Signed-off-by: Robin Appelman <robin@icewind.nl>
Signed-off-by: Robin Appelman <robin@icewind.nl>
Signed-off-by: Robin Appelman <robin@icewind.nl>
8f22a3f
to
11c1e5d
Compare
@rullzer all green |
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.
Fine by me then!
$parentPath = '/'; | ||
} | ||
// if we already cached the folder this file is in we know there are no shares for this file | ||
if (array_search($parentPath, $this->cachedFolders) === false) { |
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.
It's faster to use the $parentPath as key and checking it's presence. OTOH, there won't be much in this cache, will it?
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.
@icewind1991 Ping
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.
I prefer to use a regular array and array_search here, performance shouldn't matter since this entire branch is an edge case anyway
* @return array [$fileId => $unreadCount] | ||
* @since 12.0.0 | ||
*/ | ||
public function getNumberOfUnreadCommentsForFolder($folderId, IUser $user); |
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.
Release notes, if we had them, would be a good place to mention this addition :)
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.
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.
Still works in the WebUI as well 👍
Instead of doing 2 queries per file in a folder, we do a single query to get all unread comment counts.
comparison with 100 files