682954 - Make ErrorHandler drive the UI

Status: NEW

(Firefox :: Sync, defect)

Comment 1

[Marina Samuel [:emtwo]]

Attachment: WIP - Error Handler drives the UI

Note: this patch is pretty rough. There is a lot of room for improvement and some things may not appear sane at the moment (aside from a severe lack of tests). However it gives a good idea of the direction I'd like to go.

Comment 2

[Philipp von Weitershausen [:philikon]]

Comment on attachment 558429 [details] [diff] [review]
WIP - Error Handler drives the UI

Very, very nice.

>+    switch (notificationDetails.errorCode) {
>+      case Weave.LOGIN_ERROR:
>+        buttonFunction = function() { gSyncUI.openPrefs(); return true; };
>+        break;
>+      case Weave.SYNC_UPDATE_NEEDED:
>+        buttonFunction = 
>+          function() { window.openUILinkIn("https://services.mozilla.com/update/", "tab"); return true; };
>+        break;
>+      case Weave.APPROACHING_QUOTA:
>+        buttonFunction = function() { gSyncUI.openQuotaDialog(); return true; };
>+        break;
>+      case Weave.SERVER_BACKOFF:
>+        buttonFunction = function() { gSyncUI.openServerStatus(); return true; };
>+        break;
>+      case Weave.SYNC_ERROR:
>+        buttonFunction = function() { gSyncUI.doSync(); return true; };
>+        break;
>+      case Weave.QUOTA_NOTICE:
>+        buttonFunction = function() { gSyncUI.openQuotaDialog(); return true; };
>+        break;
>+      case Weave.DELAYED_SYNC:
>+        buttonFunction = function() { gSyncUI.openServerStatus(); return true; };
>+        break;

This can be written more elegantly:

  let buttonDispatch = {};
  buttonDispatch[Weave.LOGIN_ERROR] = function () { gSyncUI.openPrefs(); return true; };
  buttonDispatch[Weave.APPROACHING_QUOTA] = function () { gSyncUI.openQuotaDialog(); return true; };
  ...

and then do:

  let buttonFunc = buttonDispatch[notificationDetails.errorCode];

You probably also want to make sure that we actually found a function for the given error code, so you could do something like this, because showing no button at all rather than one that doesn't work is probably better:

  if (typeof buttonFunc == "function") {
    buttons.push(...);
  }

>       case "weave:ui:login:error":
>-        this.onLoginError();
>+        // if we haven't set up the client, don't show errors
>+        if (this._needsSetup()) {
>+          return;
>+        }
>+        // fall through
>+      case "weave:ui:sync:error":
>+      case "weave:ui:sync:delayed":
>+      case "weave:ui:quota:remaining":
>+        this.showError(subject);
>         break;

Do we still need different notification topics here? Wouldn't one suffice? (The _needsSetup() check won't hurt for any of the other

>   checkServerError: function checkServerError(resp) {
>     switch (resp.status) {
>       case 400:
>         if (resp == RESPONSE_OVER_QUOTA) {
>           Status.sync = OVER_QUOTA;
>+        } else if (Weave.Service.isLoggedIn) {
>+          Status.sync = BAD_SERVER_REQUEST;
>+        } else {
>+          Status.login = BAD_SERVER_REQUEST;
>         }
>         break;
...
>+BAD_SERVER_REQUEST:                    "error.sync.reason.bad_server_request",
...
>+error.sync.reason.bad_server_request = HTTP Error 400 - Bad Server Request

This is not a very useful message to show users. An HTTP 400 status code is basically just an indication for the client to look for the actual error code in the HTTP body, which can be one of these: http://docs.services.mozilla.com/respcodes.html. None of them are meaningful to users apart from the over quota bit. All the other ones basically mean that either the client or the server messed up, so "a technical error occurred" is a perfectly valid description. ;) We should probably log the response body, though!

>diff --git a/services/sync/locales/en-US/sync.properties b/services/sync/locales/en-US/sync.properties
>--- a/services/sync/locales/en-US/sync.properties
>+++ b/services/sync/locales/en-US/sync.properties
>@@ -1,32 +1,33 @@
> # %1: the user name (Ed), %2: the app name (Firefox), %3: the operating system (Android)
> client.name2 = %1$S's %2$S on %3$S
> 
> # %S is the date and time at which the last sync successfully completed
> lastSync2.label = Last sync: %S
> 
> mobile.label = Mobile Bookmarks
> 
>-remote.pending.label = Remote tabs are being syncedâ¦
>+remote.pending.label = Remote tabs are being synced
...
>-error.login.prefs.label = Preferencesâ¦
>+error.login.prefs.label = Preferences

Please don't change these strings.

>+error.sync.backoff = The Sync server has encountered an error, but you don't need to do anything about it. We'll resume syncing your data in a bit.

How about "The Sync server is experiencing problems. Syncing will resume automatically." It's a bit shorter :) This will only show if you're explicitly clicking "Sync Now", right?

>+// UI Error Codes
>+LOGIN_ERROR:                           0,
>+SYNC_ERROR:                            1,
>+SYNC_UPDATE_NEEDED:                    2,
>+APPROACHING_QUOTA:                     3,
>+SERVER_BACKOFF:                        4,
>+QUOTA_NOTICE:                          5,
>+DELAYED_SYNC:                          6,

Can we use strings here? For a lot of these we already have constants, right? Like LOGIN_FAILED, SYNC-FAILED, etc.

>+  onQuotaNotice: function onQuotaNotice(subject, data) {
>+    let title = Utils.toStr("warning.sync.quota.label");
>+    let description = Utils.toStr("warning.sync.quota.description");
>+
>+    let notificationDetails = {
>+      notification: 
>+        new Weave.Notification(title, description, null,
>+                         Weave.Notifications.PRIORITY_WARNING),
>+      buttonStrings: 
>+        [Utils.toStr("error.sync.viewQuotaButton.label"),
>+        Utils.toStr("error.sync.viewQuotaButton.accesskey")],
>+
>+      errorCode: QUOTA_NOTICE
>+    };
>+
>+    Svc.Obs.notify("weave:ui:quota:remaining", notificationDetails);    
>+  },

Look at this as an example for all the other "onFoobar" methods, I think we probably want to fix Weave.Notification a bit. I think the translation shouldn't happen here but in there, and the button stuff should probably be contained in there, too. Essentially, you'd create a Notification object based on the error code and a bunch of strings and then would hand that off to the UI. I think that'd be a lot cleaner.

Skipping the tests for now... gotta run.