feat(sieve): implement redirecting action in filter UI#13095
Conversation
hazveg
force-pushed
the
feat/filter-forward-action
branch
from
50a9524 to
286e1bf
Compare
This patch implements an action for redirecting via the Sieve 'redirect' directive. Signed-off-by: hazveg <aaron.jaeger06@gmail.com>
|
Very nice. Thank you! We will check it out soon |
|
@coderabbitai review |
✅ Action performedReview finished.
|
📝 WalkthroughWalkthroughAdds a ChangesForward Mail Filter Action
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 22b34a44-9e72-4848-9bc2-e03653412729
📒 Files selected for processing (6)
lib/Service/MailFilter/FilterBuilder.phpsrc/components/mailFilter/Action.vuesrc/components/mailFilter/ActionForward.vuesrc/models/mailFilter.tstests/data/mail-filter/builder9.jsontests/data/mail-filter/builder9.sieve
| { | ||
| id: MailFilterActions.Forward, | ||
| label: this.t('mail', 'Forward to'), | ||
| }, |
There was a problem hiding this comment.
🗄️ 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-filterRepository: 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 expandedRepository: nextcloud/mail
Length of output: 320
🏁 Script executed:
# Read the MailFilterActionForward class definition more fully
sed -n '89,100p' src/models/mailFilter.tsRepository: 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
doneRepository: 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 jsonRepository: 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"
}| <!-- | ||
| - SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors | ||
| - SPDX-License-Identifier: AGPL-3.0-or-later | ||
| --> |
There was a problem hiding this comment.
📐 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.
| <!-- | |
| - 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
| export class MailFilterActionForward implements MailFilterAction { | ||
| public id: number | ||
| public type: string | ||
| constructor() { | ||
| this.id = randomId() | ||
| this.type = 'forward' | ||
| } |
There was a problem hiding this comment.
🗄️ 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.
| 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 = '' | |
| } | |
| } |
| "actions": [ | ||
| { | ||
| "type": "forward", | ||
| "flag": "alice@example.org" | ||
| } | ||
| ], |
There was a problem hiding this comment.
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"
}
],3 participants
This patch implements an action for redirecting via the Sieve 'redirect' directive.
I've attempted to include a corresponding test case, but was unfortunately unable to figure out how the tests are run - thus being unable to test my test. Sorry about that.
Summary by CodeRabbit
New Features
Tests