580672 - Implement quota UI

Status: VERIFIED FIXED

(Firefox :: Sync, defect)

Description

[Philipp von Weitershausen [:philikon]]

We need a client UI for the quota system (bug 577590). This will consist of

* a warning when the server sends the X-Weave-Quota header (which means we're approaching quota),

* an error message when the server refuses a write because we're over quota,

* an overview dialog that lets the user review how much space each collection occupies and subsequently disable individual engines (which will trigger immediate DELETEs).

Both the warning and the error message should point the user to that quota overview dialog.

Comment 1

[Igor Velkov]

Not forget that user can want to disable engine just for one client, keppeng it work for other.

Comment 2

[Philipp von Weitershausen [:philikon]]

(In reply to comment #1)
> Not forget that user can want to disable engine just for one client, keppeng it
> work for other.

That's an explicit non-goal. See bug 578671.

Comment 3

[Philipp von Weitershausen [:philikon]]

Attachment: Part 1 (v1): Implement quota parts of Sync API

Recognize quota warnings from server, implement API calls to retrieve quota information.

Comment 4

[Philipp von Weitershausen [:philikon]]

Attachment: Part 2 (v1): Quota notifications and dialog

UI: Implement quota notifications and View Quota dialog

Comment 5

[Philipp von Weitershausen [:philikon]]

To test this, I used Tarek's Python server. It required a small patch because the info/collection_usage API call was mispelt. Then enable quotas in development.ini and set quota level to a ridiculously low amount (<=1 MB will always trigger the warning for instance):

storage.use_quota = True
storage.quota_size = 1024

Comment 6

[Mike Connor [:mconnor]]

Comment on attachment 473066 [details] [diff] [review]
Part 1 (v1): Implement quota parts of Sync API


>diff --git a/services/sync/modules/resource.js b/services/sync/modules/resource.js

>+      if (success && headers["x-weave-quota-remaining"])
>+        Observers.notify("weave:service:quota:remaining",
>+                         parseInt(headers["x-weave-quota"], 10));

x-weave-quota-remaining here, I assume?

>diff --git a/services/sync/modules/service.js b/services/sync/modules/service.js
>--- a/services/sync/modules/service.js
>+++ b/services/sync/modules/service.js
>@@ -207,7 +207,7 @@
>       return;
> 
>     let storageAPI = this.clusterURL + Svc.Prefs.get("storageAPI") + "/";
>-    let userBase = storageAPI + this.username + "/";
>+    let userBase = this.userBaseURL = storageAPI + this.username + "/";
>     this._log.debug("Caching URLs under storage user base: " + userBase);

why still have userBase if we're adding this.userBaseURL?

>@@ -1571,8 +1571,11 @@
>     if (Utils.checkStatus(resp.status, null, [500, [502, 504]])) {
>       Status.enforceBackoff = true;
>       if (resp.status == 503 && resp.headers["retry-after"])
>-        Svc.Obs.notify("weave:service:backoff:interval", parseInt(resp.headers["retry-after"], 10));
>+        Svc.Obs.notify("weave:service:backoff:interval",
>+                       parseInt(resp.headers["retry-after"], 10));
>     }
>+    if (resp.status == 400 && resp == "14")
>+      Status.sync = OVER_QUOTA;

we should get relevant consts into constants.js, no magic numbers!

> // Load Weave on the first time this file is loaded
>diff --git a/services/sync/tests/unit/test_resource.js b/services/sync/tests/unit/test_resource.js
>--- a/services/sync/tests/unit/test_resource.js
>+++ b/services/sync/tests/unit/test_resource.js
>@@ -99,6 +99,19 @@
>   response.bodyOutputStream.write(body, body.length);  
> }
> 
>+function server_quota_notice(request, response) {
>+  let body = "You're approaching quota.";
>+  response.setHeader("X-Weave-Quota-Remaining", '1048576', false);
>+  response.setStatusLine(request.httpVersion, 200, "OK");
>+  response.bodyOutputStream.write(body, body.length);  
>+}
>+
>+function server_quota_error(request, response) {
>+  let body = "14";
>+  response.setHeader("X-Weave-Quota-Remaining", '-1024', false);
>+  response.setStatusLine(request.httpVersion, 400, "OK");
>+  response.bodyOutputStream.write(body, body.length);  
>+}
> 
> function server_headers(metadata, response) {
>   let ignore_headers = ["host", "user-agent", "accept", "accept-language",
>@@ -131,17 +144,19 @@
>   logger = Log4Moz.repository.getLogger('Test');
>   Log4Moz.repository.rootLogger.addAppender(new Log4Moz.DumpAppender());
> 
>-  let server = new nsHttpServer();
>-  server.registerPathHandler("/open", server_open);
>-  server.registerPathHandler("/protected", server_protected);
>-  server.registerPathHandler("/404", server_404);
>-  server.registerPathHandler("/upload", server_upload);
>-  server.registerPathHandler("/delete", server_delete);
>-  server.registerPathHandler("/json", server_json);
>-  server.registerPathHandler("/timestamp", server_timestamp);
>-  server.registerPathHandler("/headers", server_headers);
>-  server.registerPathHandler("/backoff", server_backoff);
>-  server.start(8080);
>+  let server = httpd_setup({
>+    "/open": server_open,
>+    "/protected": server_protected,
>+    "/404": server_404,
>+    "/upload": server_upload,
>+    "/delete": server_delete,
>+    "/json": server_json,
>+    "/timestamp": server_timestamp,
>+    "/headers": server_headers,
>+    "/backoff": server_backoff,
>+    "/quota-notice": server_quota_notice,
>+    "/quota-error": server_quota_error
>+  });
> 
>   Utils.prefs.setIntPref("network.numRetries", 1); // speed up test
> 
>@@ -181,7 +196,7 @@
>   let res2 = new Resource("http://localhost:8080/protected");
>   content = res2.get();
>   do_check_true(did401);
>-  do_check_eq(content, "This path exists and is protected - failed")
>+  do_check_eq(content, "This path exists and is protected - failed");
>   do_check_eq(content.status, 401);
>   do_check_false(content.success);
> 
>@@ -335,6 +350,25 @@
>   content = res10.get();
>   do_check_eq(backoffInterval, 600);
> 
>+
>+  _("X-Weave-Quota header notifies observer on successful requests.");

now I'm really confused about which header is which.

>+  let quotaValue;
>+  function onQuota(subject, data) {
>+    quotaValue = subject;
>+  }
>+  Observers.add("weave:service:quota:remaining", onQuota);



>diff --git a/services/sync/tests/unit/test_service_quota.js b/services/sync/tests/unit/test_service_quota.js
>new file mode 100644
>--- /dev/null
>+++ b/services/sync/tests/unit/test_service_quota.js
>@@ -0,0 +1,50 @@
>+Cu.import("resource://services-sync/service.js");
>+Cu.import("resource://services-sync/util.js");
>+
>+function send(body) {
>+  return function(request, response) {
>+    response.setStatusLine(request.httpVersion, 200, "OK");
>+    response.bodyOutputStream.write(body, body.length);
>+  };
>+}
>+
>+function run_test() {
>+  let collection_usage = {steam:  65.11328,
>+                          petrol: 82.488281,
>+                          diesel: 2.25488281};
>+  let quota = [2169.65136, 8192];
>+
>+  do_test_pending();
>+  let server = httpd_setup({
>+    "/1.0/johndoe/info/collection_usage": send(JSON.stringify(collection_usage)),
>+    "/1.0/johndoe/info/quota":            send(JSON.stringify(quota)),
>+    "/1.0/janedoe/info/collection_usage": send("gargabe"),
>+    "/1.0/janedoe/info/quota":            send("more gargabe")
>+  });

garbage? :D

looks okay, with issues addressed.

Comment 7

[Philipp von Weitershausen [:philikon]]

Attachment: View Quota dialog

Comment 8

[Philipp von Weitershausen [:philikon]]

Attachment: View Quota dialog after unchecking an item

Comment 9

[Philipp von Weitershausen [:philikon]]

(In reply to comment #6)

Ugh, sorry for the half bit rotten patch. It should be X-Weave-Quota-Remaining everywhere.

> why still have userBase if we're adding this.userBaseURL?

No need, just shorter ;)

Comment 10

[Philipp von Weitershausen [:philikon]]

Attachment: Part 1 (v2): Implement quota parts of Sync API

Addressed review comments:
* Made "14" response code a constant
* Fix resource.js (X-Weave-Quota -> X-Weave-Quota-Remaining)
* Avoid definition of userBase
* Fix typo

Comment 11

[Philipp von Weitershausen [:philikon]]

Attachment: Part 1 (v3): Implement quota parts of Sync API

Also catch errors from the Clients engine. It's the first to sync and thus quite likely to run into quota issues.

Comment 12

[Philipp von Weitershausen [:philikon]]

Attachment: Part 2 (v2): Quota notifications and dialog

Reorder error checks to actually catch OVER_QUOTA errors.

Comment 13

[Axel Hecht [:Pike]]

Comment on attachment 473608 [details] [diff] [review]
Part 2 (v2): Quota notifications and dialog

+quota.usage.label       = You are using %S%% (%S) of your allowed %S.
and
+quota.freeup.label      = This will free up %S.

should get a localization note explaining the parameters that get inserted. Also, it'd be good for usage.label to use ordered variable references right away, i.e., %1$S and use those in the comment, too.

Comment 14

[Mike Connor [:mconnor]]

Right, I should have called those out, thanks Axel!

Comment 15

[Philipp von Weitershausen [:philikon]]

Attachment: Part 2 (v3): Quota notifications and dialog

Address Axel's comments: Explain parameters in strings and use explicit ordering for multiple parameters.

Comment 16

[Axel Hecht [:Pike]]

Comment on attachment 473835 [details] [diff] [review]
Part 2 (v3): Quota notifications and dialog

There are a few candidates here that I'll need to look at with my head on, more tomorrow.

Comment 17

[Philipp von Weitershausen [:philikon]]

Attachment: Part 2 (v3.1): Quota notifications and dialog

Correct typo in a comment.

Comment 18

[Philipp von Weitershausen [:philikon]]

Attachment: Part 1 (v3.1): Implement quota parts of Sync API

Test cosmetics

Comment 19

[Philipp von Weitershausen [:philikon]]

Attachment: Part 2 (v4): Quota notifications and dialog

Be more tolerant towards servers that implement the quota API but don't support quotas (as the prod servers do right now apparently...)

Comment 20

[Axel Hecht [:Pike]]

Comment on attachment 473915 [details] [diff] [review]
Part 2 (v4): Quota notifications and dialog

r- on the usage of DownloadUtils.convertByteUnits, that should be used as in other places in downloads, aka, both number and metric need to be independently placed. The localization note should reference the download manager sizes, too. Same for quota.usagePercentage.label and quota.freeup.label.

Also, the ", " for quota.removal.label needs to be localizable.

Comment 21

[Philipp von Weitershausen [:philikon]]

Attachment: Part 2 (v5): Quota notifications and dialog

Make the position of numeric values and units localizable and add localizer note that their definitions come from the download manager. Also make gap fillers localizable.

Comment 22

[Axel Hecht [:Pike]]

Comment on attachment 474166 [details] [diff] [review]
Part 2 (v5): Quota notifications and dialog

That's looking great, thanks.

Comment 23

[Philipp von Weitershausen [:philikon]]

Part 1: http://hg.mozilla.org/services/fx-sync/rev/886d90e528ff
Part 2: http://hg.mozilla.org/services/fx-sync/rev/d5b925f006ef

Comment 24

[Tracy Walker [:tracy]]

verified with recent nightly minefield builds

though currently  doesn't seem to working quite right

Comment 25

[Mike Connor [:mconnor]]

Yes, because quota support isn't deployed on the server yet.