569161 - New Migration Assistant does not honor Advanced IMAP Synchronization settings

Status: VERIFIED FIXED

(Thunderbird :: Migration, defect)

Description

[rsx11m]

While the IMAP page introduced in bug 545563 is a great improvement over the 3.0 migration page, I figured that the choice "Per Account Synchronization" doesn't work as expected. Steps to reproduce:

Initial Setup:
 1. Take a 2.0.0.x profile which has no specific settings for account or folder
    synchronization (in my case, a single IMAP account = server2, with a couple
    of folders).
 2. Open it with 3.1 RC1 (in this case, on Windows XP).
 3. Get to the IMAP page and select "Per Account Synchronization".
 4. Click on the account in question, "Keep messages..." is checked.
 5. Select "Advanced", all folders are checked.
 6. Click Ok to Advanced, then Account Settings.

Verification:
 7. Click again on "Advanced", choices are lost, everything is checked.
 8. Uncheck again, continue with Migration Assistant.

Sanity check:
 9. Check account and folder settings, all are synchronized.
10. Go into Account Settings now and uncheck folders.
11. Now the setting "sticks" and synchronization for the selected folders is
    disabled as desired.

I'm nominating this for blocking 3.1 as it's a first-use experience issue.

Comment 1

[WADA:World Anti-bad-Duping Agency]

> the choice "Per Account Synchronization" doesn't work as expected.

It's not surprizing as for per folder offline-use setting, because Migration Assistant developers changed nothing at next module after patch for bug 562589.
> http://mxr.mozilla.org/comm-central/source/mail/base/content/msgMail3PaneWindow.js#791
Read bug 562589 comment #25 and bug 562589 comment #31, please.
It's the reason why I asked question of bug 541209 comment #10.

Comment 2

[rsx11m]

WADA, this is not about what I've suggested in bug 541209 comment #29, i.e., initializing the 3rd choice with the settings of the current 2.0 profile. Indeed, without switching the order there this wouldn't work.

The issue here is more significant: Even if the user selects the third choice offered and customizes his or her synchronization settings back to the desired values, those are lost as soon as the Account Settings dialog is closed. Apparently, the selected values are not communicated back to or overwritten by the Migration Assistant. The featureConfigurator() has been called already at this time, thus whatever UpgradeProfileAndBeUglyAboutIt() did before that should not be relevant for the bug here, unless it's somehow called again.

Comment 3

[rsx11m]

Following observations looking into the autosync part of featureConfigurator():
> http://mxr.mozilla.org/comm-central/source/mail/base/content/featureConfigurators/autosync.js

1. Function onLoad() prepares window.openDialog() for am-offline.xul,
2. updateStatus() calls updateSyncSettings(), which then
3. iterates over all servers to set syncSettings[server.key], *but*
4. setSyncStatus() sets syncSettings[server.key] for all folders.

Thus, the underlying problem appears to be:
 - updateSyncSettings() not iterating over the folders, and
 - setSyncStatus() overwriting any user choice with the account default.

Comment 4

[rsx11m]

Theory confirmed, unchecking "Keep messages for this account on this computer" in step #4 and forgoing the Advanced settings now leaves all folders as *not* selected for offline synchronization.

Comment 5

[WADA:World Anti-bad-Duping Agency]

(In reply to comment #2)
> The featureConfigurator() has been called already at this time, 
> thus whatever UpgradeProfileAndBeUglyAboutIt() did before that
> should not be relevant for the bug here, unless it's somehow called again.

Have you read bug 562589 comment #23 bug 562589 comment #25?
Blake Winton says that call of featureConfigurator() won't kick Migration Assistant immediately/synchronously.

It's probably wrong at least for per foder offline-use setting. Per foder offline-use setting is probably saved in .msf by UpgradeProfileAndBeUglyAboutIt(). Next part should be executed by Migration Assinstant or after Migration Assitant. It's mandatory to fullfil request of bug 541209.           
> 819         for each (let folder in fixIterator(allFolders, Components.interfaces.nsIMsgFolder))
> 820           folder.setFlag(Components.interfaces.nsMsgFolderFlags.Offline);

(In reply to comment #4)
> Theory confirmed, unchecking "Keep messages for this account on this computer"
> in step #4 and forgoing the Advanced settings now leaves all folders as *not*
> selected for offline synchronization.

It's also not surprizing, because it's current behaviour of the UI. See 562589 comment #1, please. It's almost same as "manual unchecking of Keep messages ..." just after migration. To achieve "not change per offline use setting upon migration", above change of UpgradeProfileAndBeUglyAboutIt() is required.

FYI.
Another way to prohibit auto-sync of an account:
  mail.server.serverX.autosync_offline_stores=false by Migration Assistant
  See bug 537943 comment #17 and bug 524684, please.
As mail.server.serverX.offline_download=true/false is tighed to check/unceck of "[ ? ] Keep messages of this account ...", someone may change it. But, as no one touches serverX.autosync_offline_stores, it should be kept, if Migration Assistant sets it.
However, it produces mismatch between current UI of "Keep mssages of this account...". Design change like bug 524684 and bug 537943 comment #17 is required. So, this approach unfortunately can't be chosen currently.

Comment 6

[rsx11m]

Please stop changing my flags, this is all platform-independent code.

> (comment #5) Have you read bug 562589 comment #23 bug 562589 comment #25?

Yes.

> Blake Winton says that call of featureConfigurator() won't kick Migration
> Assistant immediately/synchronously.

Since either the Migration Assistant or - for the Advanced settings - am-offline.js will iterate over all folders depending on the option chosen, initializing them explicitly still appears redundant to me. If in fact there
are issues with asynchronous modification of those, even more a reason to concentrate any folder-flag modification to a single function.

> offline-use setting is probably saved in .msf by

They are, though I couldn't identify the respective flag in the Mork code.

> It's also not surprizing, because it's current behaviour of the UI. See 562589
> comment #1, please. It's almost same as "manual unchecking of Keep messages
> ..." just after migration. To achieve "not change per offline use setting upon
> migration", above change of UpgradeProfileAndBeUglyAboutIt() is required.

As said, am-offline.js iterates over all folders when that option is changed, thus the main point of 4th item in comment #3 would be to forgo this step if the 3rd option is chosen as the Account Settings already took care of it.

> Another way to prohibit auto-sync of an account:

I'm not looking for a solution to prohibit auto-sync, those hacks are known. This specific bug report is about user choices being offered in the migration UI and then not considered for the settings. Thus, a user who has spent some time configuring his or her settings per account and folder during the 2.0/3.1 migration will eventually find that this time was wasted and the configuration has to be done again. Thus, either the folder-specific settings can be allowed to persist as select, or a way has to be found to disable the Advanced button if it's not possible due to some asynchronous nature of the functions.

Comment 7

[rsx11m]

Attachment: Proposed patch

This fixes it for me and should at least serve as a proof of concept.

With this patch, selecting the "Synchronize" and "Do not Synchronize" options behave like before and switch the off-line on or off as desired. The third case "Per Account Synchronization" now allows the Synchronization & Storage code to modify the folders, and won't override them due to the if(!="some") statement before iterating over the folders. Thus, any customization made is now retained also for the folder properties when continuing.

Comment 8

[WADA:World Anti-bad-Duping Agency]

I think line 801 to 821 of UpgradeProfileAndBeUglyAboutIt is better to be removed at same time.

The removal merely does next change.
(Before removal, with Bug 562589)
If serverX.offline_download=false is set prior to first use of Tb 3, offline-use setting is not touched upon migraion.
(After removal, with your patch)
If Migration Assistant is intentionally disabled prior to first use of Tb 3, offline-use setting is not touched upon migraion.

This meets requirement of Bug 562589. As all IMAP folder's offline-use setting was changed to off even though bug opener of Bug 562589 disabled Migration Assistant, he opened bug 562589. We needed a way to bypass "offline-use=off of all IMAP folders by migration" with Tb 3.0, so patch of Bug 562589 was created.
Because user now can select action on IMAP folder's offline use setting if Migration Assistant is enabled(default), the removal won't produce any problem. Tb 3.1 or later merely looses feature to force all users to use Tb 3 with offline-use=off of all IMAP folders always regardless of user's want on auto-sync, which Tb 3.0.0 to Tb 3.0.4 unfotunately had.
And, if removed, UpgradeProfileAndBeUglyAboutIt() can be renamed UpgradeProfileWithExcellentMigrationAssistant().

> http://mxr.mozilla.org/comm-central/source/mail/base/content/msgMail3PaneWindow.js#801
> 801       // Mark all imap folders as offline at the very first run of TB v3
> 802       // We use the threadpane ui version to determine TB profile version

> 806       for each (let server in fixIterator(servers, Components.interfaces.nsIMsgIncomingServer))
> 807       {
> 808         // If it's not an imap server, or if we haven't set the
> 809         // offlineDownload flag on the server, then don't set the offline
> 810         // flags on all the sub-folders.
> 811         // Note: We need to use instanceof to get the offlineDownload flag.
              (i)  Bug 562589 checked serverX.offline_download.
                   And if false, skips (ii).
              (ii) At here, offline-use=off of all IMAP folder is set.    
> 821       }

> 823       // Open a dialog explaining the major changes from version 2.
> 824       if (gPrefBranch.getBoolPref("mail.ui.show.migration.on.upgrade"))
> 825         // But let the main window finish opening first.
> 826         setTimeout(openFeatureConfigurator, 0, [true,]);

Comment 9

[rsx11m]

(In reply to comment #8)
> I think line 801 to 821 of UpgradeProfileAndBeUglyAboutIt is better to be
> removed at same time.

In general I agree, but that's not this bug - see bug 541209 comment #32, more would be needed to get the desired default setting for the Migration Assistant.

Comment 10

[Mark Banner (:standard8)]

We're respinning RC 1 semi-due to this bug, and some others, Blake will fill in the details soon.

Comment 11

[rsx11m]

Thanks for the update. Blake incorporated this into attachment 449299 [details] [diff] [review]
for bug 541209, thus I'm expecting the bug to be fixed if that patch goes in.

Comment 12

[Blake Winton (:bwinton) (:☕️)]

Attachment: A first cut at the patch.

(I accidentally attached this to bug 541209 earlier.)

A first cut at the patch.

Okay, here's a patch (against comm-central) that I think fixes this.

Play around with it, and let me know what you think.

(Also, I forget whether I was going to ask asuth or bienvenu for the review on
this one, so if I got it wrong, please feel free to re-assign the request,
Andrew.)

Thanks,
Blake.

Comment 13

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 449393 [details] [diff] [review]
A first cut at the patch.

This looks like what we discussed.  I have included a number of nits to in order to improve your who-gets-the-review decision-making process in the future >;)

http://reviews.visophyte.org/r/449393/

on file: mail/base/content/featureConfigurators/autosync.js line 68
>       // this.updateSyncSettings() was here.

This comment is not very useful.  An explanation of why the call migrated
south would be useful.  Since the call has been conditionalized, a comment on
why it is conditional (at the site) would likely be useful too.


on file: mail/base/content/featureConfigurators/autosync.js line 113
>       // XXXXXXX

Normally I would expect X's to be accompanied by some description of future
work or hint at what is ugly about what is going on.  What's the problem here?


on file: mail/base/content/featureConfigurators/autosync.js line 124
>       sync.data("automatic", true);

A comment as to what this does would be useful.  It appears to me that you are
tracking whether the user has modified the value or whether it has been
automatically set.  Setting "user-modified" (in the inverse) seems somewhat
more self-explanatory, but I don't really care; that might just need a comment
less.


on file: mail/base/content/featureConfigurators/autosync.js line 131
>   /**
>    * Set the sync preferences based on the radio buttons.
>    *
>    * @param aSyncStatus the requested sync status.
>    */
>   setSyncStatus: function as_setSyncStatus(aSyncStatus, aAutomatic) {

Update the docs for your new parameter.


on file: mail/base/content/featureConfigurators/autosync.js line 141
>       let dataElem = $("#autosync-choice");

Data storage on a "p" tag in the DOM seems weird when you set other stuff on
"this" later on.  Is there some bizarre life-cycle stuff going on that demands
this?


on file: mail/base/content/featureConfigurators/autosync.js line 193
>   updateSyncSettings: function as_updateSyncSettings() {

update sounds like a mutating verb with potentially destructure side-effects,
whereas updateSyncSettings and its new friend updateFolderSyncSettings would
appear to do things more resembling "check", "derive", "infer", "save", or
"preserve".


on file: mail/base/content/featureConfigurators/autosync.js line 216
>       for each (let folder in fixIterator(allFolders, Ci.nsIMsgFolder))
>         subSettings["#" + folder.folderURL] = folder.getFlag(Ci.nsMsgFolderFlags.Offline);

hygiene demands squiggly braces.

Comment 14

[Blake Winton (:bwinton) (:☕️)]

(In reply to comment #13)
> (From update of attachment 449393 [details] [diff] [review])
> This looks like what we discussed.  I have included a number of nits to in
> order to improve your who-gets-the-review decision-making process in the future
> >;)

I love nits.  It makes me feel like I've still got stuff to learn.  From now on, you get all my review requests!  ;)

> http://reviews.visophyte.org/r/449393/
> on file: mail/base/content/featureConfigurators/autosync.js line 68
> >       // this.updateSyncSettings() was here.
> This comment is not very useful.  An explanation of why the call migrated
> south would be useful.  Since the call has been conditionalized, a comment on
> why it is conditional (at the site) would likely be useful too.

The call migrated south so that we could tell whether we were ending up on allSync, someSync, or noneSync.  I've added a comment in the code explaining why it's conditional.

> on file: mail/base/content/featureConfigurators/autosync.js line 113
> >       // XXXXXXX
> Normally I would expect X's to be accompanied by some description of future
> work or hint at what is ugly about what is going on.  What's the problem here?

It was so that I knew where I was moving the code to.  Removed.

> on file: mail/base/content/featureConfigurators/autosync.js line 124
> >       sync.data("automatic", true);
> A comment as to what this does would be useful.  It appears to me that you are
> tracking whether the user has modified the value or whether it has been
> automatically set.  Setting "user-modified" (in the inverse) seems somewhat
> more self-explanatory, but I don't really care; that might just need a comment
> less.

Setting user-modified is harder, so I'ld rather not change it, but I've added a comment which should help clear things up.

> on file: mail/base/content/featureConfigurators/autosync.js line 131
> >   /**
> >    * Set the sync preferences based on the radio buttons.
> >    *
> >    * @param aSyncStatus the requested sync status.
> >    */
> >   setSyncStatus: function as_setSyncStatus(aSyncStatus, aAutomatic) {
> Update the docs for your new parameter.

Done.

> on file: mail/base/content/featureConfigurators/autosync.js line 141
> >       let dataElem = $("#autosync-choice");
> Data storage on a "p" tag in the DOM seems weird when you set other stuff on
> "this" later on.  Is there some bizarre life-cycle stuff going on that demands
> this?

So, yeah, I guess I should store them all on the p.
It doesn't make sense to store the previous chosen option in another option, since I only ever want one of them.
I can take the shortcut of storing the automaticity on the option itself, since I'll be calling the change method on it on the very next line, but I suppose that does make it a little more complicated, so I'll store that bit of info on the "p" as well.

> on file: mail/base/content/featureConfigurators/autosync.js line 193
> >   updateSyncSettings: function as_updateSyncSettings() {
> update sounds like a mutating verb with potentially destructure side-effects,
> whereas updateSyncSettings and its new friend updateFolderSyncSettings would
> appear to do things more resembling "check", "derive", "infer", "save", or
> "preserve".

Changed to saveSyncSettings/saveFolderSyncSettings.

> on file: mail/base/content/featureConfigurators/autosync.js line 216
> >       for each (let folder in fixIterator(allFolders, Ci.nsIMsgFolder))
> >         subSettings["#" + folder.folderURL] = folder.getFlag(Ci.nsMsgFolderFlags.Offline);
> hygiene demands squiggly braces.

Okay, what do you think about:
      for each (let folder in fixIterator(allFolders, Ci.nsIMsgFolder)) {
        subSettings["#" + folder.folderURL] =
            folder.getFlag(Ci.nsMsgFolderFlags.Offline);
      }

Later,
Blake.

Comment 16

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 449393 [details] [diff] [review]
A first cut at the patch.

This looks like what we talked about so I'll stamp it good.

Comment 17

[Blake Winton (:bwinton) (:☕️)]

Pushed to 1.9.2 as http://hg.mozilla.org/releases/comm-1.9.2/rev/bc354ee5d0f3
and 3.1rc2 relbranch as http://hg.mozilla.org/releases/comm-1.9.2/rev/8092632c4f66
and trunk as http://hg.mozilla.org/comm-central/rev/0bae15c56d3e

Steps for a Litmus test sent to Ludo.  (I don't know whether he added it or not, so I'm setting it to "in-litmus?")

Thanks,
Blake.

Comment 18

[rsx11m]

Verified fixed in 3.1 RC2 build1: Testing with 2.0 profile without any folders set to synchronize, as well with another 2.0 profile where some folders were set to be synchronized. Defaults to "Do not synchronize" in the first case and "Per Account" in the second case. Changes in the Advanced menu are correctly recognized, checkbox there affects all folders as intended.

As a nice side effect, bug 537943 appears to be fixed as well as far as the disable logic is concerned.