1280058 - Warning for destructive retention settings for POP and IMAP accounts shouldn't be shown for Local Folders

Status: RESOLVED FIXED

(MailNews Core :: Account Manager, defect)

Description

[rsx11m]

+++ This bug was initially created as a clone of Bug #582170 +++

> Expected Results:  
> A warning should pop up indicating that messages are going to be deleted from
> the server, and that this is an action that cannot be undone.

This was implemented in bug 582170, but now also shows up when checking anything other than "Don't delete any message" in the Local Folders account settings. The wording "on the remote server" does not make any sense in this context. The same may also apply to news and/or RSS accounts, where you can't delete server-side (didn't test).

Proposal for this bug is to either pick a different message for the respective account types, or to omit the message for those entirely.

Comment 1

[rsx11m]

http://hg.mozilla.org/comm-central/annotate/b46028761bce/mailnews/base/prefs/content/am-offline.js#l339

Looks like all that's needed here is a check for gServerType to be either "imap" or "pop" before entering the code for presenting the warning. I'll give this a try.

Comment 2

[rsx11m]

Attachment: Simple solution

Yeah, this works. Please double-check that this is the right action to take for news and RSS accounts (it certainly is for movemail, given that no access to the server exists in the first place and the default action is to remove it from the daemon's drop location after import).

Comment 3

[rsx11m]

Paenglab, Ratty: I'd consider this mostly a UX issue (hence the choice of reviewers), i.e., should a warning box be shown for accounts other than IMAP and POP as well, and if yes, which text. I'd think we can do without for those cases, given that the context is clearly local there and thus unambiguous.

Comment 4

[:aceman]

Thanks for noticing this. What about Exchange accounts? Instead of hard-coding checks like this, can you maybe use some of the nsIMsgIncomingServer properties to determine if the server has some "user-removable message"? If not, maybe we could add it. Because a similar check is needed in bug 274452. It is not the same, but a similar idea.

Comment 5

[Richard Marti (:Paenglab)]

Comment on attachment 8762703 [details] [diff] [review]
Simple solution

I give a ui-r+ and let aceman decide if he wants an other solution to check if it also deletes messages on remote server.

Comment 6

[Philip Chee]

Comment on attachment 8762703 [details] [diff] [review]
Simple solution

Looks reasonable.

Comment 7

[rsx11m]

Thanks for the quick reviews.

(In reply to :aceman from comment #4)
> Instead of hard-coding checks like this, can you maybe use some of the
> nsIMsgIncomingServer properties to determine if the server has some
> "user-removable message"?

Well, there are plenty of hardcoded "imap" and "pop3" in both am-offline.js and am-offline.xul, so this would apply to a lot more instances and require some rewrite of that code to add property flags like HAS_LOCAL_DATA, HAS_REMOTE_DATA, CAN_DELETE_REMOTE, etc. I see your point though, an extension adding a new server type won't be able to define that server type for showing the warning box.

As a compromise, how about introducing an attribute confirmfor="imap,pop3" to the retention.keepMsg radiogroup, thus extensions could modify that attribute and add a new type for showing the warning?

Comment 8

[rsx11m]

Attachment: Add server-type attribute (v2)

This implements the "confirmfor" attribute with a server-type list suggested in comment #7. I can rename that to "warnfor" if that sounds better.

Carrying forward the ui-r+ from attachment 8762703 [details] [diff] [review] as it's functionally the same.

Comment 9

[:aceman]

(In reply to rsx11m from comment #7)
> Well, there are plenty of hardcoded "imap" and "pop3" in both am-offline.js
> and am-offline.xul, so this would apply to a lot more instances and require
> some rewrite of that code to add property flags like HAS_LOCAL_DATA,
> HAS_REMOTE_DATA, CAN_DELETE_REMOTE, etc.

Yes, there are many of these, but we would like to migrate away from these hardcodings.
Addons or even the new JsAccounts could need to plug into these checks. There were some proposals for this (can't find the bugs now) on how to solve it (whether new attributes on nsIMsgIncomingServer or just one function that would take a string identifier of a property and return a value). There was even a GSOC project proposal for this. But nothing materialized yet, so I have at least started bug 1238271 to improve the situation for now.

> As a compromise, how about introducing an attribute confirmfor="imap,pop3"
> to the retention.keepMsg radiogroup, thus extensions could modify that
> attribute and add a new type for showing the warning?

Yes, probably better than nothing for now.

Comment 10

[:aceman]

Comment on attachment 8762781 [details] [diff] [review]
Add server-type attribute (v2)

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

::: mailnews/base/prefs/content/am-offline.js
@@ +337,5 @@
>  function warnServerRemove(aRadio)
>  {
> +  let confirmFor = aRadio.getAttribute("confirmfor");
> +
> +  if (confirmFor && confirmFor.split(',').indexOf(gServerType) >= 0 && aRadio.value != 1) {

I think .includes() works on an array.

Comment 11

[:aceman]

CCing rkent so he can use this in the Exchange addon.

Comment 12

[rsx11m]

Attachment: Patch as checked in (v3)

(In reply to :aceman from comment #10)
> I think .includes() works on an array.

It does, pushed with that change as http://hg.mozilla.org/comm-central/rev/abd1ef9761da

Comment 13

[rsx11m]

(In reply to :aceman from comment #9)
> Addons or even the new JsAccounts could need to plug into these checks.
> There were some proposals for this (can't find the bugs now) on how to solve
> it (whether new attributes on nsIMsgIncomingServer or just one function that
> would take a string identifier of a property and return a value).

Agreed, but working out an isolated solution here probably wouldn't be helpful either to consider all current and future use cases of such a mechanism. That should be done in a respectively scoped bug.

> I have at least started bug 1238271 to improve the situation for now.

Oops, I've bitrotted your attachment 8737582 [details] [diff] [review], but should be trivial to fix.

Comment 14

[:aceman]

(In reply to rsx11m from comment #13)
> Agreed, but working out an isolated solution here probably wouldn't be
> helpful either to consider all current and future use cases of such a
> mechanism. That should be done in a respectively scoped bug.

Sure, I didn't ask for that :) I just asked if you have at least looked at the existing attributes on incomingServer if one does not match the usecase here :)

> > I have at least started bug 1238271 to improve the situation for now.
> 
> Oops, I've bitrotted your attachment 8737582 [details] [diff] [review], but
> should be trivial to fix.
No problem :)

That one still isn't the full right solution but at least should reduce the amount of hardcoding server types.

Comment 15

[rsx11m]

(In reply to :aceman from comment #14)
> Sure, I didn't ask for that :) I just asked if you have at least looked at the
> existing attributes on incomingServer if one does not match the usecase here :)

attribute ACString type; looks tempting. ;-)

There is also ACString localStoreType but may be ambiguous.

I don't like using such heuristics to determine properties and capabilities of a server and how it's locally managed, some set of explicit flags seems to be a much safer way to go.

Comment 16

[:aceman]

I'm sure gServerType that you used is taken from 'attribute ACString type;' so that is no better:) localStoreType is interesting, but we would need to again hardcode "mailbox" (but that may cover Local folders) and "imap" (and some unknown types from addons again).

Maybe the attribute 'canFileMessagesOnServer'. Would need to check which servers set it and what it means and how it is used.

Comment 17

[rsx11m]

(In reply to :aceman from comment #16)
> I'm sure gServerType that you used is taken from 'attribute ACString type;'

I know, I was kidding. ;-)

https://dxr.mozilla.org/comm-central/source/mailnews/local/src/nsPop3IncomingServer.cpp#451 says:
> // override this so we can say that deferred accounts can't have messages
> // filed to them, which will remove them as targets of all the move/copy
> // menu items.
for POP3 accounts. Thus, that attribute is apparently more in the local sense of "server" rather than indicating remote capabilities. In a similar way there could be a set of boolean or a single integer readonly attribute listing the capabilities and properties associated with that type, undefined or zeroed by default and set in the respective ns<type>IncomingServer.cpp implementations.

But anyway, that's for a different bug.

Comment 18

[rsx11m]

Comment on attachment 8762868 [details] [diff] [review]
Patch as checked in (v3)

Do you want this on the branches? It should be trivial enough as such, but I'm not sure if the addition of an attribute (and its implied semantics) already constitutes a change in interface.

[Approval Request Comment]
Regression caused by (bug #): bug 582170
User impact if declined: User may see a warning box in cases where it's not applicable.
Testing completed (on c-c, etc.): tested on a 45.x builds
Risk to taking this patch (and alternatives if risky): low

Comment 19

[Jorg K (CEST = GMT+2)]

Comment on attachment 8762868 [details] [diff] [review]
Patch as checked in (v3)

Interface change is a change to an IDL file.

Comment 20

[rsx11m]

Ok, thanks.

http://hg.mozilla.org/releases/comm-aurora/rev/12e810cf04e3
http://hg.mozilla.org/releases/comm-beta/rev/a63d523ef2ea

Comment 21

[Kent James (:rkent)]

Comment on attachment 8762868 [details] [diff] [review]
Patch as checked in (v3)

http://hg.mozilla.org/releases/comm-esr45/rev/0750b50fd584