Skip to content

feat(sieve): implement redirecting action in filter UI #13095

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
hazveg wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat(sieve): implement redirecting action in filter UI #13095

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
4 changes: 4 additions & 0 deletions lib/Service/MailFilter/FilterBuilder.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,10 @@ public function buildSieveScript(array $filters, string $untouchedScript): strin
if ($action['type'] === 'stop') {
$actions[] = 'stop;';
}
if ($action['type'] === 'forward') {
$recipient = SieveUtils::escapeString($action['recipient']);
$actions[] = sprintf('redirect "%s";', $recipient);
}
}

if (count($tests) > 1) {
Expand Down
8 changes: 8 additions & 0 deletions src/components/mailFilter/Action.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ import DeleteIcon from 'vue-material-design-icons/TrashCanOutline.vue'
import ActionAddflag from './ActionAddflag.vue'
import ActionAddSystemFlag from './ActionAddSystemFlag.vue'
import ActionFileinto from './ActionFileinto.vue'
import ActionForward from './ActionForward.vue'
import ActionStop from './ActionStop.vue'
import { MailFilterActions } from '../../models/mailFilter.ts'

Expand All @@ -51,6 +52,7 @@ export default {
ActionFileinto,
ActionAddflag,
ActionStop,
ActionForward,
DeleteIcon,
},

Expand Down Expand Up @@ -85,6 +87,10 @@ export default {
id: MailFilterActions.Stop,
label: this.t('mail', 'Stop'),
},
{
id: MailFilterActions.Forward,
label: this.t('mail', 'Forward to'),
},
Comment on lines +90 to +93

@coderabbitai coderabbitai Bot Jun 23, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🗄️ Data Integrity & Integration | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== Forward action objects in JSON fixtures =="
fd -e json . tests src | while read -r file; do
  jq -c --arg file "$file" '
    .. | objects
    | select(.type? == "forward")
    | {file: $file, hasRecipient: has("recipient"), hasFlag: has("flag"), object: .}
  ' "$file" 2>/dev/null || true
done

echo
echo "== Forward key usage in code paths =="
rg -n -C2 'forward|recipient|flag' src/components/mailFilter src/models lib/Service/MailFilter tests/data/mail-filter

Repository: nextcloud/mail

Length of output: 24829

🏁 Script executed:

# Get the MailFilterActionForward class definition from models
ast-grep outline src/models/mailFilter.ts --match MailFilterActionForward --view expanded

Repository: nextcloud/mail

Length of output: 320

🏁 Script executed:

# Read the MailFilterActionForward class definition more fully
sed -n '89,100p' src/models/mailFilter.ts

Repository: nextcloud/mail

Length of output: 395

🏁 Script executed:

# Verify there are no other forward actions in fixtures with the same issue
fd -e json tests src | while read -r file; do
  jq -c --arg file "$file" '
    .. | objects
    | select(.type? == "forward")
    | {file: $file, keys: keys}
  ' "$file" 2>/dev/null || true
done

Repository: nextcloud/mail

Length of output: 152

🏁 Script executed:

# Search for all forward action objects in JSON files
rg -A2 '"type":\s*"forward"' tests/data/mail-filter --type json

Repository: nextcloud/mail

Length of output: 323

Fix fixture data integrity: forward action should use recipient key, not flag.

The test fixture at tests/data/mail-filter/builder9.json contains a forward action with "flag": "alice@example.org", but the backend FilterBuilder consumes action['recipient'] (lib/Service/MailFilter/FilterBuilder.php:94) and the frontend ActionForward component emits { recipient: value }.

Additionally, the MailFilterActionForward model class (src/models/mailFilter.ts) lacks a recipient property definition.

Fixture inconsistency 
{
  "type": "forward",
  "flag": "alice@example.org"
}

Should be:

{
  "type": "forward",
  "recipient": "alice@example.org"
}

],
}
},
Expand All @@ -103,6 +109,8 @@ export default {
return ActionStop
} else if (this.action.type === MailFilterActions.AddSystemFlag) {
return ActionAddSystemFlag
} else if (this.action.type === MailFilterActions.Forward) {
return ActionForward
}
return null
},
Expand Down
53 changes: 53 additions & 0 deletions src/components/mailFilter/ActionForward.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
<!--
- SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors
- SPDX-License-Identifier: AGPL-3.0-or-later
-->
Comment on lines +1 to +4

@coderabbitai coderabbitai Bot Jun 23, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Use the repository-required SPDX header block format in this new Vue file.

The new file uses an HTML comment SPDX banner, but the guideline requires the C-style SPDX block format.

💡 Suggested fix 
-<!--
-  - SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors
-  - SPDX-License-Identifier: AGPL-3.0-or-later
--->
+/* SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors
+ * SPDX-License-Identifier: AGPL-3.0-or-later
+ */

As per coding guidelines "**/*.{php,js,ts,tsx,vue}: Every file must include an SPDX license header... Header format: /* SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors * SPDX-License-Identifier: AGPL-3.0-or-later */".

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
<!--
- SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors
- SPDX-License-Identifier: AGPL-3.0-or-later
-->
/* SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors
* SPDX-License-Identifier: AGPL-3.0-or-later
*/

Source: Coding guidelines

<template>
<NcTextField
:required="true"
:model-value="recipient"
:label-outside="true"
:placeholder="t('mail', 'Enter recipient')"
@update:value="onInput" />
</template>

<script>
import { NcTextField } from '@nextcloud/vue'

export default {
name: 'ActionForward',
components: {
NcTextField,
},

props: {
action: {
type: Object,
required: true,
},

account: {
type: Object,
required: true,
},
},

computed: {
recipient() {
return this.action.recipient ?? ''
},
},

methods: {
onInput(value) {
this.$emit('update-action', { recipient: value })
},
},
}
</script>

<style lang="scss" scoped>
.input-field {
display: inline-block; /* for flex expand */
}
</style>
10 changes: 10 additions & 0 deletions src/models/mailFilter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ export enum MailFilterConditionOperator {

export enum MailFilterActions {
AddSystemFlag = 'addsystemflag',
Forward = 'forward',
Stop = 'stop',
}

Expand Down Expand Up @@ -85,6 +86,15 @@ export class MailFilterActionStop implements MailFilterAction {
}
}

export class MailFilterActionForward implements MailFilterAction {
public id: number
public type: string
constructor() {
this.id = randomId()
this.type = 'forward'
}
Comment on lines +89 to +95

@coderabbitai coderabbitai Bot Jun 23, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Initialize recipient in the forward action model to preserve the frontend↔backend contract.

MailFilterActionForward currently creates { id, type } only, but the Sieve builder reads action['recipient'] directly for type === 'forward'. A newly-added forward action saved before input can therefore serialize without recipient and break redirect generation.

💡 Suggested fix 
 export class MailFilterActionForward implements MailFilterAction {
 	public id: number
 	public type: string
+	public recipient: string
 	constructor() {
 		this.id = randomId()
 		this.type = 'forward'
+		this.recipient = ''
 	}
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
export class MailFilterActionForward implements MailFilterAction {
public id: number
public type: string
constructor() {
this.id = randomId()
this.type = 'forward'
}
export class MailFilterActionForward implements MailFilterAction {
public id: number
public type: string
public recipient: string
constructor() {
this.id = randomId()
this.type = 'forward'
this.recipient = ''
}
}

}

export class MailFilter {
public id: number
public name: string
Expand Down
23 changes: 23 additions & 0 deletions tests/data/mail-filter/builder9.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
[
{
"name": "Test 9",
"enable": true,
"operator": "allof",
"tests": [
{
"operator": "is",
"values": [
"bob@example.org"
],
"field": "to"
}
],
"actions": [
{
"type": "forward",
"flag": "alice@example.org"
}
],
Comment on lines +15 to +20

@coderabbitai coderabbitai Bot Jun 23, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

Critical: Field name mismatch between test fixture and implementation.

Line 18 uses "flag" as the field name, but FilterBuilder.php line 94 expects "recipient":

$recipient = SieveUtils::escapeString($action['recipient'])

This will cause the forward action to fail at runtime because the expected field does not exist. The expected Sieve output in builder9.sieve line 3 confirms the correct field name should be "recipient".

[functional_correctness, data_integrity_and_integration]

🐛 Proposed fix for test fixture field name 
 		"actions": [
 			{
 				"type": "forward",
-				"flag": "alice@example.org"
+				"recipient": "alice@example.org"
 			}
 		],

"priority": 10
}
]
8 changes: 8 additions & 0 deletions tests/data/mail-filter/builder9.sieve
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
# Hello, this is a test
### Nextcloud Mail: Filters ### DON'T EDIT ###
# FILTER: [{"name":"Test 9","enable":true,"operator":"allof","tests":[{"operator":"is","values":["bob@example.org"],"field":"to"}],"actions":[{"type":"forward","recipient":"alice@example.org"}],"priority":10}]
# Test 9
if address :is :all "To" ["bob@example.org"] {
redirect "alice@example.org";
}
### Nextcloud Mail: Filters ### DON'T EDIT ###
Loading