Skip to content

perf(quick-actions): eliminate duplicate api calls for each envelope #11945

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

perf(quick-actions): eliminate duplicate api calls for each envelope #11945

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
5 changes: 0 additions & 5 deletions appinfo/routes.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -510,11 +510,6 @@
'url' => '/api/textBlocks/{id}/shares',
'verb' => 'GET',
],
[
'name' => 'actionStep#findAllStepsForAction',
'url' => '/api/action-step/{actionId}/steps',
'verb' => 'GET'
],
],
'resources' => [
'accounts' => ['url' => '/api/accounts'],
Expand Down
15 changes: 0 additions & 15 deletions lib/Controller/ActionStepController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,21 +33,6 @@ public function __construct(
$this->uid = $userId;
}

/**
* @NoAdminRequired
*
* @return JsonResponse
*/
#[TrapError]
public function findAllStepsForAction(int $actionId): JsonResponse {
if ($this->uid === null) {
return JsonResponse::error('User not found', Http::STATUS_UNAUTHORIZED);
}
$actionSteps = $this->quickActionsService->findAllActionSteps($actionId, $this->uid);

return JsonResponse::success($actionSteps);
}

/**
* @NoAdminRequired
* @param string $name
Expand Down
12 changes: 12 additions & 0 deletions lib/Db/Actions.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,18 +22,30 @@
class Actions extends Entity implements JsonSerializable {
protected $name;
protected $accountId;
protected $actionSteps = [];
protected $icon = '';

public function __construct() {
$this->addType('name', 'string');
$this->addType('accountId', 'integer');
}

public function setActionSteps(array $actionSteps): void {
$this->actionSteps = $actionSteps;
}

public function setIcon(string $icon): void {
$this->icon = $icon;
}

#[ReturnTypeWillChange]
public function jsonSerialize() {
return [
'id' => $this->getId(),
'name' => $this->getName(),
'accountId' => $this->getAccountId(),
'actionSteps' => $this->actionSteps,
'icon' => $this->icon,

];
}
Expand Down
17 changes: 7 additions & 10 deletions lib/Service/QuickActionsService.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,13 +35,18 @@ public function __construct(
private ActionStepMapper $actionStepMapper,
) {
}

/**
* @param string $userId
* @return Actions[]
*/
public function findAll(string $userId): array {
return $this->actionsMapper->findAll($userId);
$actions = $this->actionsMapper->findAll($userId);
foreach ($actions as $action) {
$actionSteps = $this->actionStepMapper->findAllStepsForOneAction($action->getId(), $userId);
$action->setActionSteps($actionSteps);
$action->setIcon($actionSteps[0]->getName());
}
return $actions;
}

/**
Expand Down Expand Up @@ -74,14 +79,6 @@ public function delete(int $actionId, string $userId): void {
$this->actionsMapper->delete($action);
}

/**
* @param string $userId
* @return ActionStep[]
*/
public function findAllActionSteps(int $actionId, string $userId): array {
return $this->actionStepMapper->findAllStepsForOneAction($actionId, $userId);
}

/**
* @throws DoesNotExistException
*/
Expand Down
63 changes: 25 additions & 38 deletions src/components/Envelope.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -479,7 +479,6 @@ import NoTrashMailboxConfiguredError
from '../errors/NoTrashMailboxConfiguredError.js'
import logger from '../logger.js'
import { buildRecipients as buildReplyRecipients } from '../ReplyBuilder.js'
import { findAllStepsForAction } from '../service/QuickActionsService.js'
import { FOLLOW_UP_TAG_LABEL } from '../store/constants.js'
import useMainStore from '../store/mainStore.js'
import { mailboxHasRights } from '../util/acl.js'
Expand Down Expand Up @@ -589,7 +588,6 @@ export default {
customSnoozeDateTime: new Date(moment().add(2, 'hours').minute(0).second(0).valueOf()),
overwriteOneLineMobile: false,
hoveringAvatar: false,
filteredQuickActions: [],
quickActionLoading: false,
}
},
Expand Down Expand Up @@ -846,20 +844,36 @@ export default {
},
].filter((option) => option.timestamp !== null)
},
},

watch: {
storeActions() {
this.filterAndEnrichQuickActions()
filteredQuickActions() {
const filteredQuickActions = []
const quickActions = this.mainStore.getQuickActions().filter((action) => action.accountId === this.data.accountId)
for (const action of quickActions) {
const check = action.actionSteps.every((step) => {
if (['markAsSpam', 'applyTag', 'markAsImportant', 'markAsFavorite'].includes(step.name) && !this.hasWriteAcl) {
return false
}
if (['markAsRead', 'markAsUnread'].includes(step.name) && !this.hasSeenAcl) {
return false
}
if (['moveThread', 'deleteThread'].includes(step.name) && !this.hasDeleteAcl) {
return false
}
return true
})
if (check) {
filteredQuickActions.push({
...action,
})
}
}
return filteredQuickActions
},
},

async mounted() {
mounted() {
this.onWindowResize()
window.addEventListener('resize', this.onWindowResize)
if (this.filteredQuickActions.length === 0) {
await this.filterAndEnrichQuickActions()
}
},

methods: {
Expand All @@ -874,38 +888,11 @@ export default {
return shortRelativeDatetime(new Date(this.data.dateInt * 1000))
},

async filterAndEnrichQuickActions() {
this.filteredQuickActions = []
const quickActions = this.mainStore.getQuickActions().filter((action) => action.accountId === this.data.accountId)
for (const action of quickActions) {
const steps = await findAllStepsForAction(action.id)
const check = steps.every((step) => {
if (['markAsSpam', 'applyTag', 'markAsImportant', 'markAsFavorite'].includes(step.type) && !this.hasWriteAcl) {
return false
}
if (['markAsRead', 'markAsUnread'].includes(step.type) && !this.hasSeenAcl) {
return false
}
if (['moveThread', 'deleteThread'].includes(step.type) && !this.hasDeleteAcl) {
return false
}
return true
})
if (check) {
this.filteredQuickActions.push({
...action,
steps,
icon: steps[0]?.name,
})
}
}
},

async executeQuickAction(action) {
this.closeQuickActionsMenu()
this.quickActionLoading = true
try {
for (const step of action.steps) {
for (const step of action.actionSteps) {
switch (step.name) {
case 'markAsSpam':
if (this.layoutMessageViewThreaded) {
Expand Down
24 changes: 17 additions & 7 deletions src/components/quickActions/Settings.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ import TagIcon from 'vue-material-design-icons/TagOutline.vue'
import IconDelete from 'vue-material-design-icons/TrashCanOutline.vue'
import Action from './Action.vue'
import logger from '../../logger.js'
import { createActionStep, deleteActionStep, findAllStepsForAction, updateActionStep } from '../../service/QuickActionsService.js'
import { createActionStep, deleteActionStep, updateActionStep } from '../../service/QuickActionsService.js'
import useMainStore from '../../store/mainStore.js'

export default {
Expand Down Expand Up @@ -228,8 +228,8 @@ export default {
this.localAction = { id: null, name: '' }
this.actions = []
} else {
this.localAction = { ...action }
this.actions = await findAllStepsForAction(action.id)
this.localAction = { id: action.id, name: action.name, accountId: action.accountId }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there no generic way to clone an object in vue? the manual destruct/construct is fragile. when we change properties of the object we have to keep this line in sync.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need specific properties from the Object

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you elaborate? what I see above we map three properties to identical properties.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I meant the object contains both quickActions and action steps to avoid multiple requests to the servers
so it looks like {id, name, accountId, actionsSteps : [array of steps] }
I split them back here so I don't break the CRUD logic for both
If you look at the next line you will see that I'm also storing the steps in a local variable.

this.actions = action.actionSteps
this.highestOrder = Math.max(...this.actions.map((a) => a.order), 0)
this.editMode = true
}
Expand Down Expand Up @@ -260,7 +260,7 @@ export default {
for (const [index, action] of this.actions.entries()) {
if (action?.id !== null && action?.id !== undefined) {
try {
await updateActionStep(action.id, action.name, action.order, action?.tagId, action?.mailboxId)
this.actions[index] = await updateActionStep(action.id, action.name, action.order, action?.tagId, action?.mailboxId)
} catch (error) {
logger.error('Could not update quick action step', {
error,
Expand All @@ -273,10 +273,12 @@ export default {
this.actions[index] = createdStep
}
}
this.localAction = quickAction
}
showSuccess(t('mail', 'Quick action updated'))
} else {
let quickAction
const createdSteps = []
try {
quickAction = await this.mainStore.createQuickAction(this.localAction.name, this.account.id)
} catch (error) {
Expand All @@ -288,17 +290,23 @@ export default {
}
try {
for (const action of this.actions) {
await createActionStep(action.name, action.order, quickAction.id, action?.tagId, action?.mailboxId)
const createdStep = await createActionStep(action.name, action.order, quickAction.id, action?.tagId, action?.mailboxId)
if (createdStep) {
createdSteps.push(createdStep)
}
}
this.actions = createdSteps
} catch (error) {
logger.error('Could not add step to quick action', {
error,
})
showError(t('mail', 'Failed to add steps to quick action'))
this.closeEditModal()
}
this.localAction = quickAction
showSuccess(t('mail', 'Quick action created'))
}
this.mainStore.patchActionStepsLocally(this.localAction.id, this.actions)
this.closeEditModal()
},

Expand Down Expand Up @@ -338,9 +346,13 @@ export default {
},

async deleteAction(item) {
this.actions = this.actions.filter((action) => action.order !== item.order).map((action, index) => ({ ...action, order: index + 1 }))
this.highestOrder = Math.max(...this.actions.map((a) => a.order), 0)
if (item.id) {
try {
await deleteActionStep(item.id)
const actions = this.actions.filter((action) => action.id)
this.mainStore.patchActionStepsLocally(this.localAction.id, actions)
} catch (error) {
logger.error('Could not delete action step', {
error,
Expand All @@ -349,8 +361,6 @@ export default {
return
}
}
this.actions = this.actions.filter((action) => action.order !== item.order).map((action, index) => ({ ...action, order: index + 1 }))
this.highestOrder = Math.max(...this.actions.map((a) => a.order), 0)
},
},
}
Expand Down
8 changes: 0 additions & 8 deletions src/service/QuickActionsService.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,14 +40,6 @@ export async function deleteQuickAction(id) {
})
}

export async function findAllStepsForAction(actionId) {
const url = generateUrl('/apps/mail/api/action-step/{id}/steps', { id: actionId })
return handleHttpAuthErrors(async () => {
const response = await axios.get(url)
return response.data.data
})
}

export async function createActionStep(name, order, actionId, tagId = null, mailboxId = null) {
const url = generateUrl('/apps/mail/api/action-step')
return handleHttpAuthErrors(async () => {
Expand Down
8 changes: 8 additions & 0 deletions src/store/mainStore/actions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2322,6 +2322,14 @@ export default function mainStoreActions() {
Vue.set(this.quickActions, index, quickAction)
}
},
patchActionStepsLocally(id, steps) {
const index = this.quickActions.findIndex((s) => s.id === id)
if (index !== -1) {
const updatedQuickAction = this.quickActions[index]
updatedQuickAction.actionSteps = steps
Vue.set(this.quickActions, index, updatedQuickAction)
}
},
deleteQuickActionLocally(id) {
const index = this.quickActions.findIndex((s) => s.id === id)
if (index !== -1) {
Expand Down
Loading