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

RESOLVED FIXED in Thunderbird 65.0

Status

enhancement
--
major
RESOLVED FIXED
11 months ago
7 months ago

People

(Reporter: bugzilla, Assigned: aceman)

Tracking

Thunderbird 65.0
Dependency tree / graph

Thunderbird Tracking Flags

(thunderbird_esr60+ verified, thunderbird64 fixed, thunderbird65 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

Reporter

Description

11 months ago
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.

Updated

11 months ago
Severity: critical → normal

Comment 1

11 months ago
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.
Flags: needinfo?(acelists)

Updated

10 months ago
See Also: → maildirblockers
Assignee

Comment 2

7 months ago
Posted patch 1481915.patch (obsolete) — Splinter Review
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.
Assignee: nobody → acelists
Status: NEW → ASSIGNED
Flags: needinfo?(acelists)
Attachment #9025964 - Flags: review?(mkmelin+mozilla)
Assignee

Comment 3

7 months ago
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.
Blocks: 856087
Severity: normal → major
Product: Thunderbird → MailNews Core
Target Milestone: --- → Thunderbird 65.0
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
Attachment #9025964 - Flags: review?(mkmelin+mozilla) → review+
Assignee

Comment 5

7 months ago
Thanks.
Keywords: checkin-needed
Assignee

Comment 6

7 months ago
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.
Attachment #9025964 - Flags: approval-comm-esr60?
Attachment #9025964 - Flags: approval-comm-beta?

Updated

7 months ago
Attachment #9025964 - Flags: approval-comm-beta? → approval-comm-beta+

Comment 7

7 months ago
Pushed by mozilla@jorgk.com:
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
Status: ASSIGNED → RESOLVED
Closed: 7 months ago
Keywords: checkin-needed
Resolution: --- → FIXED

Comment 9

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

Doing an early uplift here so we can try this ourselves.
Attachment #9025964 - Flags: approval-comm-esr60? → approval-comm-esr60+

Comment 10

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

The patch doesn't apply, but there is a larger problem, see next comment.
Attachment #9025964 - Flags: approval-comm-esr60+

Comment 11

7 months ago
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.
Status: RESOLVED → REOPENED
Flags: needinfo?(acelists)
Resolution: FIXED → ---

Comment 12

7 months ago
Changed my mind. This should do for ESR 60.
Attachment #9026352 - Flags: approval-comm-esr60+
Assignee

Comment 14

7 months ago
Comment on attachment 9026352 [details] [diff] [review]
93768af94f66a9d6e66e07042f9ed333772acabb

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

Thanks.
Attachment #9026352 - Flags: review+
Assignee

Comment 15

7 months ago
(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.
Flags: needinfo?(acelists)

Comment 16

7 months ago
Seems to work on TB 60 ESR.
Assignee

Comment 17

7 months ago
Posted patch 1481915-2.patch (obsolete) — Splinter Review
For trunk only.
Attachment #9026563 - Flags: review?(jorgk)

Comment 18

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

Thanks.
Attachment #9026563 - Flags: review?(jorgk) → review+

Comment 19

7 months ago
Posted patch e8f1be3803a6Splinter Review
This is the net change which matches the ESR version.
Attachment #9025964 - Attachment is obsolete: true
Attachment #9026563 - Attachment is obsolete: true
Attachment #9026625 - Flags: review+

Comment 20

7 months ago
Pushed by mozilla@jorgk.com:
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
Status: REOPENED → RESOLVED
Closed: 7 months ago7 months ago
Resolution: --- → FIXED
Assignee

Comment 21

7 months ago
Comment on attachment 9026625 [details] [diff] [review]
e8f1be3803a6

Yes, thanks.
Attachment #9026625 - Flags: feedback+
You need to log in before you can comment on or make changes to this bug.