Conversation
WalkthroughThis pull request refactors the empty trash operation to pass full Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@lib/features/mailbox_dashboard/presentation/controller/mailbox_dashboard_controller.dart`:
- Around line 1833-1869: Inside emptyTrashFolderAction, add a final permission
gate that checks trashFolder.myRights?.mayRemoveItems == true before attempting
clearMailbox or calling _emptyTrashFolderInteractor.execute; if the check fails,
short-circuit by calling consumeState with a Left(EmptyTrashFolderFailure(...))
containing an appropriate "not allowed" permission error and return. Place this
check after resolving trashFolder and before the existing
CapabilityIdentifier.jmapMailboxClear branch so both clearMailbox and
_emptyTrashFolderInteractor paths honor mayRemoveItems.
In `@model/lib/extensions/presentation_mailbox_extension.dart`:
- Around line 43-54: isTrashTeamMailbox currently returns true for any
descendant named "trash" because it only checks isChildOfTeamMailboxes (presence
of any parent) and the folder name; update isTrashTeamMailbox to only match the
first-level team trash mailbox by verifying the folder's immediate parent is a
team mailbox root (not any ancestor) and the folder's name equals
PresentationMailbox.trashRole (case-insensitive). Locate isTrashTeamMailbox and
replace the broad parent check (isChildOfTeamMailboxes/hasParentId) with a check
that the direct parent is a team mailbox root (e.g., compare parent type/id to
the team mailbox identifier or use an isImmediateChildOfTeamMailbox helper) so
isTrash consumers only get true for the top-level team Trash.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 78c26d54-6faa-4fcb-8ac1-447539b2c57f
📒 Files selected for processing (5)
lib/features/base/mixin/mailbox_action_handler_mixin.dartlib/features/mailbox/presentation/mailbox_controller.dartlib/features/mailbox/presentation/mixin/mailbox_widget_mixin.dartlib/features/mailbox_dashboard/presentation/controller/mailbox_dashboard_controller.dartmodel/lib/extensions/presentation_mailbox_extension.dart
| void emptyTrashFolderAction({ | ||
| Function? onCancelSelectionEmail, | ||
| MailboxId? trashFolderId, | ||
| int totalEmails = 0, | ||
| PresentationMailbox? trashMailbox, | ||
| }) { | ||
| onCancelSelectionEmail?.call(); | ||
|
|
||
| final trashMailboxId = trashFolderId ?? mapDefaultMailboxIdByRole[PresentationMailbox.roleTrash]; | ||
| final trashFolder = trashMailbox | ||
| ?? (selectedMailbox.value?.isTrash == true ? selectedMailbox.value : null) | ||
| ?? mapMailboxById[mapDefaultMailboxIdByRole[PresentationMailbox.roleTrash]]; | ||
| final accountId = this.accountId.value; | ||
| final session = sessionCurrent; | ||
|
|
||
| if (accountId == null || sessionCurrent == null) { | ||
| if (accountId == null || session == null) { | ||
| consumeState(Stream.value(Left(EmptyTrashFolderFailure(NotFoundSessionException())))); | ||
| return; | ||
| } | ||
|
|
||
| if (trashMailboxId == null) { | ||
| if (trashFolder == null) { | ||
| consumeState(Stream.value(Left(EmptyTrashFolderFailure(NotFoundMailboxException())))); | ||
| return; | ||
| } | ||
|
|
||
| if (CapabilityIdentifier.jmapMailboxClear.isSupported(sessionCurrent!, accountId)) { | ||
| if (CapabilityIdentifier.jmapMailboxClear.isSupported(session, accountId) && | ||
| trashFolder.isTrashTeamMailbox != true) { | ||
| clearMailbox( | ||
| sessionCurrent!, | ||
| session, | ||
| accountId, | ||
| trashMailboxId, | ||
| trashFolder.id, | ||
| PresentationMailbox.roleTrash, | ||
| ); | ||
| } else { | ||
| final totalEmailsInTrash = totalEmails == 0 | ||
| ? mapMailboxById[trashMailboxId]?.countTotalEmails ?? 0 | ||
| : totalEmails; | ||
|
|
||
| consumeState(_emptyTrashFolderInteractor.execute( | ||
| sessionCurrent!, | ||
| session, | ||
| accountId, | ||
| trashMailboxId, | ||
| totalEmailsInTrash, | ||
| trashFolder.id, | ||
| trashFolder.countTotalEmails, | ||
| progressStateController, |
There was a problem hiding this comment.
Enforce mayRemoveItems inside emptyTrashFolderAction.
Lines 1839-1869 resolve and execute against any trash mailbox, but never verify trashFolder.myRights?.mayRemoveItems == true. lib/features/mailbox/presentation/mixin/mailbox_widget_mixin.dart still adds MailboxActions.emptyTrash for default trash mailboxes without that guard, so some callers can reach this path and only fail after the server rejects the request. Add the permission check here as the final gate.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@lib/features/mailbox_dashboard/presentation/controller/mailbox_dashboard_controller.dart`
around lines 1833 - 1869, Inside emptyTrashFolderAction, add a final permission
gate that checks trashFolder.myRights?.mayRemoveItems == true before attempting
clearMailbox or calling _emptyTrashFolderInteractor.execute; if the check fails,
short-circuit by calling consumeState with a Left(EmptyTrashFolderFailure(...))
containing an appropriate "not allowed" permission error and return. Place this
check after resolving trashFolder and before the existing
CapabilityIdentifier.jmapMailboxClear branch so both clearMailbox and
_emptyTrashFolderInteractor paths honor mayRemoveItems.
| bool get isTrash { | ||
| if (isPersonal) { | ||
| return role == PresentationMailbox.roleTrash; | ||
| } else { | ||
| return isTrashTeamMailbox; | ||
| } | ||
| } | ||
|
|
||
| bool get isTrashTeamMailbox { | ||
| return isChildOfTeamMailboxes && | ||
| name?.name.toLowerCase() == PresentationMailbox.trashRole.toLowerCase(); | ||
| } |
There was a problem hiding this comment.
isTrashTeamMailbox matches any descendant named trash.
Line 52 only checks hasParentId() via isChildOfTeamMailboxes, so folders like Team A/Projects/Trash will also return true. Line 43 then propagates that broader match to every isTrash consumer, which can expose empty-trash and permanent-delete behavior on ordinary folders. Tighten this to the actual first-level team trash mailbox.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@model/lib/extensions/presentation_mailbox_extension.dart` around lines 43 -
54, isTrashTeamMailbox currently returns true for any descendant named "trash"
because it only checks isChildOfTeamMailboxes (presence of any parent) and the
folder name; update isTrashTeamMailbox to only match the first-level team trash
mailbox by verifying the folder's immediate parent is a team mailbox root (not
any ancestor) and the folder's name equals PresentationMailbox.trashRole
(case-insensitive). Locate isTrashTeamMailbox and replace the broad parent check
(isChildOfTeamMailboxes/hasParentId) with a check that the direct parent is a
team mailbox root (e.g., compare parent type/id to the team mailbox identifier
or use an isImmediateChildOfTeamMailbox helper) so isTrash consumers only get
true for the top-level team Trash.
|
This PR has been deployed to https://linagora.github.io/tmail-flutter/4416. |
1 participant
Issue
#4392
User story 3
Resolved
demo-delete-trash.mov
Summary by CodeRabbit
Release Notes
Bug Fixes
New Features