Closed Bug 1598332 Opened 8 months ago Closed 7 months ago

WX API for marking a mail as spam or ham

Categories

(Thunderbird :: Add-Ons: Extensions API, enhancement)

enhancement
Not set
normal

Tracking

(thunderbird_esr68 fixed)

RESOLVED FIXED
Thunderbird 73.0
Tracking Status
thunderbird_esr68 --- fixed

People

(Reporter: hermar05, Assigned: hermar05)

References

Details

Attachments

(2 files, 6 obsolete files)

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/78.0.3904.108 Safari/537.36

Steps to reproduce:

I tried marking a mail as spam

Actual results:

nothing since it cant be done as of now

Expected results:

a function which allows the webextension to mark a mail as spam

Attached file ext_messages_schema.json (obsolete) —
Attached file ext-messages.js (obsolete) —

Fallen was guiding me to extend the possible options in the update function...
But I dont know how to build a patch out of it...

Attached file test_ext_messages.js (obsolete) —

I dont know if thats the way its wanted but I tried adding some test cases...
modified rows: 117, 128-130, 150, 178-180

Assignee: nobody → hermar05
Status: UNCONFIRMED → ASSIGNED
Component: Untriaged → Add-Ons: Extensions API
Ever confirmed: true
Version: 72 → unspecified

Marvin, can you submit it these as a patch?

Attached patch the changes made as a patch (obsolete) — Splinter Review
Attached patch diff file made after the commit (obsolete) — Splinter Review
Attachment #9110574 - Attachment is obsolete: true
Attachment #9110575 - Attachment is obsolete: true
Attachment #9110622 - Attachment is obsolete: true
Attachment #9111557 - Attachment is obsolete: true
Comment on attachment 9111568 [details] [diff] [review]
diff file made after the commit

Review of attachment 9111568 [details] [diff] [review]:
-----------------------------------------------------------------

This is good! It raises some questions though, that I don't know the answer to:

* If we mark a message as junk, should we automatically mark it as read like the UI does? (depending on the user's prefs)
* What about moving it to the junk folder automatically? (depending on the user's prefs)
* Same questions, but in reverse, if we mark a message as not junk.
* On my IMAP server, if I mark a message as both not junk and not read, only one applies. We need to figure out what's going on with that.

::: mail/components/extensions/parent/ext-messages.js
@@ +273,5 @@
>              msgHdr.markFlagged(newProperties.flagged);
>            }
> +          if (newProperties.junk !== null) {
> +            let messages = Cc["@mozilla.org/array;1"].createInstance(Ci.nsIMutableArray);
> +            let score = newProperties.junk ? Ci.nsIJunkMailPlugin.IS_SPAM_SCORE : Ci.nsIJunkMailPlugin.IS_HAM_SCORE;

These two lines are too long. Run these commands to set up the linter/formatter and run it on this file.

mach eslint --setup
mach eslint --fix mail/components/extensions/parent/ext-messages.js

::: mail/components/extensions/test/xpcshell/test_ext_messages.js
@@ +175,5 @@
>    ok(message.isRead);
>    extension.sendMessage();
>  
> +  await extension.awaitMessage("junk");
> +  ok(message.isJunk);

This line should be "equal(message.getStringProperty("junkscore"), 100);"

Further down, where we check the flags have been cleared, the same check should take place, but with 0 instead of 100.
Attachment #9111568 - Flags: feedback+

Magnus, your thoughts on comment 8's questions?

Flags: needinfo?(mkmelin+mozilla)

(In reply to Geoff Lankow (:darktrojan) from comment #8)

This line should be "equal(message.getStringProperty("junkscore"), 100);"

On reflection, getProperty would probably be more appropriate!

Flags: needinfo?(mkmelin+mozilla)
Summary: Marking a mail as spam out of an extension → WX API for marking a mail as spam or ham

Good question... in general APIs should not cause such side effects, but do exactly what you say and nothing more. That does raise the question of how to get the side effects for the cases when you do want them. Maybe that needs to be a separate API like processMessage()?

maybe I can give some perspektive to the raised questions from my point of view:

  • If we mark a message as junk, should we automatically mark it as read like the UI does? (depending on the user's prefs)
    No, I think that because it (most likely) willl be set by an extension the user should take notice of that mail because the extension might be wrong

  • What about moving it to the junk folder automatically? (depending on the user's prefs)
    No, same point as above and because I just look roughly through the spam folder and then press "delete all"

  • Same questions, but in reverse, if we mark a message as not junk.
    If we mark it as not spam to mark it as unread too? only thing that comes to mind would be to get the attention of the user... but if said mail is still in the spam folder it wouldnt make much difference...

  • On my IMAP server, if I mark a message as both not junk and not read, only one applies. We need to figure out what's going on with that.
    agreed

Attached patch patch.diff (obsolete) — Splinter Review
Attachment #9111568 - Attachment is obsolete: true

Patches combined, white-space fixed. I think this is good to go.

Attachment #9112503 - Attachment is obsolete: true
Attachment #9113068 - Flags: review+

Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/e1257b911bd1
Extend messages.update API function to mark messages as junk or not junk. r=darktrojan DONTBUILD

Status: ASSIGNED → RESOLVED
Closed: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 73.0

worked fine for a couple of days but now the spam symbol just lights up for a second then vanishes again...

Flags: needinfo?(geoff)
Depends on: 1602628

Bug 1602628 should fix this.

Flags: needinfo?(geoff)

(In reply to Geoff Lankow (:darktrojan) from comment #8)

  • If we mark a message as junk, should we automatically mark it as read like
    the UI does? (depending on the user's prefs)
  • What about moving it to the junk folder automatically? (depending on the
    user's prefs)
  • Same questions, but in reverse, if we mark a message as not junk.

Personally I think this shouldn't automatically happen if an extension does it. What we can do is expose the setting if junk emails should be marked read or moved to the junk folder, then let the extension handle it. This might be a few calls more, but allows for more use cases where the moving or marking read might not be intended.

Either that, or maybe some flag that controls if such automatic actions should happen.

Comment on attachment 9131420 [details] [diff] [review]
1598332-spam-or-ham-esr.diff

Approved for ESR
Attachment #9131420 - Flags: approval-comm-esr68? → approval-comm-esr68+
You need to log in before you can comment on or make changes to this bug.