1481915 - The local folders account can be converted between mbox and maildir without setting the hidden pref in config editor to true

Status: RESOLVED FIXED

(MailNews Core :: Account Manager, enhancement)

Description

[Alex Ihrig [:Thunderbird_Mail_DE]]

Thunderbird 60 has the possibility to convert mbox to maildir and vice versa. The converter is still experimental and therefore needs to be unlocked with a hidden pref (mail.store_conversion_enabled).

The problem is that this option is not necessary for the local folders account. The prefs standard "false" seems to be ignored for the local folders account.

If people use the converter they have the risk of dataloss. That is especially in case of the local folders (it would be the same for POP accounts) a really big risk. If the mail in local folders are lost, users have no chance to (re-)synchronise them from a mail server. And from german support forums we know, that users use the local folders often to "backup" their mail.

Comment 1

[Wayne Mery (:wsmwk)]

Thanks for filing the bug report.
Aceman, is this a previously reported issue?

(In reply to Alex Ihrig from comment #0)
> Thunderbird 60 has the possibility to convert mbox to maildir and vice
> versa. The converter is still experimental and therefore needs to be
> unlocked with a hidden pref (mail.store_conversion_enabled).
> 
> The problem is that this option is not necessary for the local folders
> account. The prefs standard "false" seems to be ignored for the local
> folders account.

I can confirm this. However, I could swear when I first looked it, it wasn't offered.


> If people use the converter they have the risk of dataloss. 
But only if a previously undiscovered bug exists.

Comment 2

[:aceman]

Attachment: 1481915.patch

I can see this, it seems like an oversight from bug 856087. The same code controlling the menulist in am-server.js wasn't done in am-serverwithnoidentities.js.

Comment 3

[:aceman]

I can see this, it seems like an oversight from bug 856087. The same code controlling the menulist in am-server.js wasn't done in am-serverwithnoidentities.js.

Comment 4

[Magnus Melin [:mkmelin]]

Comment on attachment 9025964 [details] [diff] [review]
1481915.patch

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

Learn something new each day. I don't think I ever noticed we have something like this. r=mkmelin

Comment 5

[:aceman]

Thanks.

Comment 6

[:aceman]

Comment on attachment 9025964 [details] [diff] [review]
1481915.patch

[Approval Request Comment]
Regression caused by (bug #): 856087
User impact if declined: exposing an experimental potentially datalossy feature to stable release users.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/e8f1be3803a6
Do not allow converting Local Folders to maildir if the needed pref isn't set. r=mkmelin

Comment 8

[Jorg K (CEST = GMT+2)]

Beta (TB 64 beta 3):
https://hg.mozilla.org/releases/comm-beta/rev/93768af94f66a9d6e66e07042f9ed333772acabb

Comment 9

[Jorg K (CEST = GMT+2)]

Comment on attachment 9025964 [details] [diff] [review]
1481915.patch

Doing an early uplift here so we can try this ourselves.

Comment 10

[Jorg K (CEST = GMT+2)]

Comment on attachment 9025964 [details] [diff] [review]
1481915.patch

The patch doesn't apply, but there is a larger problem, see next comment.

Comment 11

[Jorg K (CEST = GMT+2)]

Looks like https://hg.mozilla.org/comm-central/rev/e8f1be3803a6 isn't right. We have:
-var gAccount;
+var gServer;
which undoes this here:
https://hg.mozilla.org/comm-central/rev/bff3cba93b7c#l5.11
but you forgot that gAccount is still used in the XUL file :-(
https://searchfox.org/comm-central/search?q=parent.setAccountLabel(gAccount.key&case=false&regexp=false&path=
I'm not sure about the other two XUL files, maybe they are wrong too now.

All that said, the only hunk that is needed in TB 60 would be th8is, right?
+  // Disable store type change if store has not been used yet.
+  storeTypeElement.setAttribute("disabled",
+    gServer.getBoolValue("canChangeStoreType") ?
+      "false" : !Services.prefs.getBoolPref("mail.store_conversion_enabled"));
I'm not backporting this now without confirmation.

Comment 12

[Jorg K (CEST = GMT+2)]

Attachment: 93768af94f66a9d6e66e07042f9ed333772acabb

Changed my mind. This should do for ESR 60.

Comment 13

[Jorg K (CEST = GMT+2)]

TB 60.4 ESR:
https://hg.mozilla.org/releases/comm-esr60/rev/4f5354ecf23fd73446760ff46f58dec8e01fe7f8

Comment 14

[:aceman]

Comment on attachment 9026352 [details] [diff] [review]
93768af94f66a9d6e66e07042f9ed333772acabb

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

Thanks.

Comment 15

[:aceman]

(In reply to Jorg K (GMT+1) from comment #11)

> https://hg.mozilla.org/comm-central/rev/bff3cba93b7c#l5.11
> but you forgot that gAccount is still used in the XUL file :-(

Yes, sorry, I need to fix this, by reverting the change.

Comment 16

[Jorg K (CEST = GMT+2)]

Seems to work on TB 60 ESR.

Comment 17

[:aceman]

Attachment: 1481915-2.patch

For trunk only.

Comment 18

[Jorg K (CEST = GMT+2)]

Comment on attachment 9026563 [details] [diff] [review]
1481915-2.patch

Thanks.

Comment 19

[Jorg K (CEST = GMT+2)]

Attachment: e8f1be3803a6

This is the net change which matches the ESR version.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/77d43522f459
Backed out changeset e8f1be3803a6 to make way for the correct solution. a=backout
https://hg.mozilla.org/comm-central/rev/b31bf692d3a1
Do not allow converting Local Folders to maildir if the needed pref isn't set (take 2). r=mkmelin,jorgk DONTBUILD

Comment 21

[:aceman]

Comment on attachment 9026625 [details] [diff] [review]
e8f1be3803a6

Yes, thanks.

Comment 22

[Jorg K (CEST = GMT+2)]

If we do a TB 64 beta 4, this could be forgotten easily, so here goes:
https://hg.mozilla.org/releases/comm-beta/rev/ec3758df141d1034281f31f9567ddf1d3a1acf70
https://hg.mozilla.org/releases/comm-beta/rev/e9bf46e6e0d0bd323129e8edbb598e30dbce1e11