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

feat: move messages to junk folder #8581

Merged
merged 1 commit into from
Aug 7, 2023
Merged

Conversation

kesselb
Copy link
Contributor

@kesselb kesselb commented Jun 27, 2023

For #4101

To Do

  • Decide: Merge JunkSettings into AccountDefaultSettings
  • Decide: Rename "Mark as Spam" to "Move to Spam" with checkbox "Move message to Junk" enabled
  • Inline documentation about the action/getter behavior
  • Testing

Test cases

Message List Message View
Screenshot from 2023-07-07 18-45-40 Screenshot from 2023-07-07 18-46-01
  1. Add a new account. See a different icon for an existing folder/mailbox with the special role "junk".
  2. Open account settings. A section "Junk settings" exist. The checkbox "Move message to Junk" is off by default.
  3. Open action menu from the message list. Click "Mark as Spam". The message is flagged as spam, but not moved.
  4. Open action menu from the message view. Click "Mark as Spam". The message is flagged as spam, but not moved.
  5. Open account settings. Toggle checkbox "Move message to Junk".
  6. Open action menu from the message list. Click "Mark as Spam". The message is flagged as spam and moved.
  7. Open action menu from the message view. Click "Mark as Spam". The message is flagged as spam and moved.
  8. Open junk mailbox and repeat 6 and 7.

@ChristophWurst ChristophWurst changed the title Enh move spam to spam folder feat: move messages to junk folder Jun 27, 2023
$mailbox->getName(),
$event->getUid(),
$account,
'INBOX',
Copy link
Member

Choose a reason for hiding this comment

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

can we assume the name to be always exactly like that? what about capitalization? personal namspace?

Copy link
Member

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

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

Decide: Rename "Mark as Spam" to "Move to Spam" with checkbox "Move message to Junk" enabled

It should stay a simple "Mark as spam" action. It is expected from (the majority of) other clients that "Mark as spam" will also move it to spam/junk.

@kesselb
Copy link
Contributor Author

kesselb commented Aug 7, 2023

image

I will add the junk mailbox to the default folders and use a different section for the move junk toggle.
It looks similar to Richard's trash retention feature then.

@kesselb kesselb force-pushed the enh-move-spam-to-spam-folder branch 2 times, most recently from 1438330 to cc52e74 Compare August 7, 2023 10:10
Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

Works

@ChristophWurst
Copy link
Member

OCA\Mail\Tests\Integration\Db\MailAccountTest::testMailAccountConstruct
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
     'quotaPercentage' => null
     'trashRetentionDays' => 60
     'name' => 'Peter Parker'
+    'junkMailboxId' => null
+    'moveJunk' => false
 )

Signed-off-by: Daniel Kesselberg <[email protected]>
@kesselb kesselb force-pushed the enh-move-spam-to-spam-folder branch from d8fb535 to 45c5ab9 Compare August 7, 2023 10:28
@kesselb kesselb merged commit 0a11a71 into main Aug 7, 2023
28 of 29 checks passed
@kesselb kesselb deleted the enh-move-spam-to-spam-folder branch August 7, 2023 10:38
@kesselb kesselb added this to the v3.4.0 milestone Aug 7, 2023
@jancborchardt
Copy link
Member

Follow-up as talked with @ChristophWurst and mentioned above:

It is expected from (the majority of) other clients that "Mark as spam" will also move it to spam/junk.

Meaning the toggle to need to enable this functionality can be removed as it should be enabled by default.

@jancborchardt
Copy link
Member

#8738

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants