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

perf: reduce number of avatar requests #10486

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions lib/Db/Message.php
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
use Horde_Mail_Rfc822_Identification;
use JsonSerializable;
use OCA\Mail\AddressList;
use OCA\Mail\Service\Avatar\Avatar;
use OCP\AppFramework\Db\Entity;
use ReturnTypeWillChange;
use function in_array;
Expand Down Expand Up @@ -133,6 +134,8 @@
/** @var Tag[] */
private $tags = [];

/** @var Avatar|null */
private $avatar;
public function __construct() {
$this->from = new AddressList([]);
$this->to = new AddressList([]);
Expand Down Expand Up @@ -283,6 +286,20 @@
);
}
}
/**
* @param Avatar|null $avatar
* @return void
*/
public function setAvatar(?Avatar $avatar): void {
$this->avatar = $avatar;

Check warning on line 294 in lib/Db/Message.php

View check run for this annotation

Codecov / codecov/patch

lib/Db/Message.php#L293-L294

Added lines #L293 - L294 were not covered by tests
}

/**
* @return Avatar
*/
public function getAvatar(): ?Avatar {
return $this->avatar;

Check warning on line 301 in lib/Db/Message.php

View check run for this annotation

Codecov / codecov/patch

lib/Db/Message.php#L300-L301

Added lines #L300 - L301 were not covered by tests
}

#[ReturnTypeWillChange]
public function jsonSerialize() {
Expand Down Expand Up @@ -327,6 +344,7 @@
'previewText' => $this->getPreviewText(),
'encrypted' => ($this->isEncrypted() === true),
'mentionsMe' => $this->getMentionsMe(),
'avatar' => $this->avatar ? $this->avatar->jsonSerialize() : null,

Check warning on line 347 in lib/Db/Message.php

View check run for this annotation

Codecov / codecov/patch

lib/Db/Message.php#L347

Added line #L347 was not covered by tests
];
}
}
26 changes: 24 additions & 2 deletions lib/IMAP/PreviewEnhancer.php
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
use OCA\Mail\Db\Message;
use OCA\Mail\Db\MessageMapper as DbMapper;
use OCA\Mail\IMAP\MessageMapper as ImapMapper;
use OCA\Mail\Service\AvatarService;
use Psr\Log\LoggerInterface;
use function array_key_exists;
use function array_map;
Expand All @@ -34,22 +35,32 @@
/** @var LoggerInterface */
private $logger;

/** @var AvatarService */
private $avatarService;

/** @var string|null */
private $userId;

public function __construct(IMAPClientFactory $clientFactory,
ImapMapper $imapMapper,
DbMapper $dbMapper,
LoggerInterface $logger) {
LoggerInterface $logger,
AvatarService $avatarService,
string $userId) {
$this->clientFactory = $clientFactory;
$this->imapMapper = $imapMapper;
$this->mapper = $dbMapper;
$this->logger = $logger;
$this->avatarService = $avatarService;
$this->userId = $userId;

Check warning on line 55 in lib/IMAP/PreviewEnhancer.php

View check run for this annotation

Codecov / codecov/patch

lib/IMAP/PreviewEnhancer.php#L54-L55

Added lines #L54 - L55 were not covered by tests
hamza221 marked this conversation as resolved.
Show resolved Hide resolved
}

/**
* @param Message[] $messages
*
* @return Message[]
*/
public function process(Account $account, Mailbox $mailbox, array $messages): array {
public function process(Account $account, Mailbox $mailbox, array $messages, string $mode = 'sync'): array {

Check warning on line 63 in lib/IMAP/PreviewEnhancer.php

View check run for this annotation

Codecov / codecov/patch

lib/IMAP/PreviewEnhancer.php#L63

Added line #L63 was not covered by tests
hamza221 marked this conversation as resolved.
Show resolved Hide resolved
$needAnalyze = array_reduce($messages, static function (array $carry, Message $message) {
if ($message->getStructureAnalyzed()) {
// Nothing to do
hamza221 marked this conversation as resolved.
Show resolved Hide resolved
Expand All @@ -59,6 +70,17 @@
return array_merge($carry, [$message->getUid()]);
}, []);

// If we are in the API call, we need to fetch the avatar for the sender
if ($mode === 'apiCall') {
foreach ($messages as $message) {
$from = $message->getFrom()->first() ;
hamza221 marked this conversation as resolved.
Show resolved Hide resolved
if ($message->getAvatar() === null && $from !== null && $from->getEmail() !== null && $this->userId !== null) {
$avatar = $this->avatarService->getAvatar($from->getEmail(), $this->userId);
$message->setAvatar($avatar);

Check warning on line 79 in lib/IMAP/PreviewEnhancer.php

View check run for this annotation

Codecov / codecov/patch

lib/IMAP/PreviewEnhancer.php#L74-L79

Added lines #L74 - L79 were not covered by tests
}
}
}

if ($needAnalyze === []) {
// Nothing to enhance
return $messages;
Expand Down
3 changes: 2 additions & 1 deletion lib/Service/Search/MailSearch.php
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,8 @@ public function findMessages(Account $account,
$this->messageMapper->findByIds($account->getUserId(),
$this->getIdsLocally($account, $mailbox, $query, $sortOrder, $limit),
$sortOrder,
)
),
'apiCall'
);
}

Expand Down
20 changes: 12 additions & 8 deletions src/components/Avatar.vue
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@

<script>
import NcAvatar from '@nextcloud/vue/dist/Components/NcAvatar.js'
import { fetchAvatarUrlMemoized } from '../service/AvatarService.js'
import { generateUrl } from '@nextcloud/router'
import logger from '../logger.js'

export default {
Expand All @@ -30,6 +30,10 @@ export default {
type: String,
required: true,
},
avatar: {
type: Object,
default: null,
},
email: {
type: String,
required: true,
Expand All @@ -55,14 +59,14 @@ export default {
},
},
async mounted() {
if (this.email !== '') {
try {
this.avatarUrl = await fetchAvatarUrlMemoized(this.email)
} catch {
logger.debug('Could not fetch avatar', { email: this.email })
}
if (this.avatar) {
this.avatarUrl = this.avatar.isExternal
? generateUrl('/apps/mail/api/avatars/image/{email}', {
email: this.email,
})
: this.avatar.url
}

logger.debug('Could not fetch avatar', { email: this.email })
this.loading = false
},
}
Expand Down
2 changes: 1 addition & 1 deletion src/components/Envelope.vue
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@
:class="{ 'important-one-line': oneLineLayout, 'icon-important': !oneLineLayout }"
:data-starred="isImportant ? 'true' : 'false'"
@click.prevent="hasWriteAcl ? onToggleImportant() : false"
v-html="importantSvg" />

Check warning on line 39 in src/components/Envelope.vue

View workflow job for this annotation

GitHub Actions / NPM lint

'v-html' directive can lead to XSS attack
<JunkIcon v-if="data.flags.$junk"
:size="18"
class="app-content-list-item-star junk-icon-style"
Expand All @@ -52,7 +52,7 @@
<CheckIcon :size="40" class="check-icon" :class="{ 'app-content-list-item-avatar-selected': selected }" />
</template>
<template v-else>
<Avatar :display-name="addresses" :email="avatarEmail" />
<Avatar :display-name="addresses" :email="avatarEmail" :avatar="data.avatar" />
</template>
</div>
</template>
Expand Down Expand Up @@ -475,7 +475,7 @@
this.onWindowResize()

window.addEventListener('resize', this.onWindowResize)
},

Check warning on line 478 in src/components/Envelope.vue

View workflow job for this annotation

GitHub Actions / NPM lint

The "computed" property should be above the "mounted" property on line 473
computed: {
...mapGetters([
'isSnoozeDisabled',
Expand Down
2 changes: 1 addition & 1 deletion src/components/OutboxMessageListItem.vue
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
:details="details"
@click="openModal">
<template #icon>
<Avatar :display-name="avatarDisplayName" :email="avatarEmail" />
<Avatar :display-name="avatarDisplayName" :email="avatarEmail" :avatar="message.avatar" />
</template>
<template #subname>
{{ subjectForSubtitle }}
Expand Down
1 change: 1 addition & 0 deletions src/components/ThreadEnvelope.vue
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
:display-name="envelope.from[0].label"
:disable-tooltip="true"
:size="40"
:avatar="envelope.avatar"
class="envelope__header__avatar-avatar" />
<div v-if="isImportant"
class="app-content-list-item-star icon-important"
Expand Down
Loading