Make data limits a permanent feature #698

alalamav · 2020-06-17T01:10:59Z

shadowbox

Moves the access key data limits endpoints to /server/access-key-data-limits from experimental/access-key-data-limits.
Redirects the experimental endpoint to the new endpoint with a 301 for backwards compatibility.
Increases the server's minor version to v1.4.0.

Outline Manager

Moves the data limits UI out of the experiments section. Retains the experiments UI for the future.
Configures the data limits API endpoint based on the server version.
Removes the data limits availability date and associated surveys. Retains the survey code for the future.

mpmcroy · 2020-06-17T16:24:55Z

src/server_manager/web_app/ui_components/outline-server-view.js

@@ -546,7 +546,7 @@ export class ServerView extends DirMixin(PolymerElement) {
          </div>
        </div>
        <div name="settings">
-          <outline-server-settings id="serverSettings" server-id="[[serverId]]" server-hostname="[[serverHostname]]" server-name="[[serverName]]" server-version="[[serverVersion]]" is-hostname-editable="[[isHostnameEditable]]" server-management-api-url="[[serverManagementApiUrl]]" server-port-for-new-access-keys="[[serverPortForNewAccessKeys]]" is-access-key-port-editable="[[isAccessKeyPortEditable]]" access-key-data-limit="{{accessKeyDataLimit}}" is-access-key-data-limit-enabled="{{isAccessKeyDataLimitEnabled}}" data-limits-availability-date="[[dataLimitsAvailabilityDate]]" supports-access-key-data-limit="[[supportsAccessKeyDataLimit]]" server-creation-date="[[serverCreationDate]]" server-monthly-cost="[[monthlyCost]]" server-monthly-transfer-limit="[[_formatBytesTransferred(monthlyOutboundTransferBytes)]]" is-server-managed="[[isServerManaged]]" server-location="[[serverLocation]]" metrics-enabled="[[metricsEnabled]]" localize="[[localize]]">
+          <outline-server-settings id="serverSettings" server-id="[[serverId]]" server-hostname="[[serverHostname]]" server-name="[[serverName]]" server-version="[[serverVersion]]" is-hostname-editable="[[isHostnameEditable]]" server-management-api-url="[[serverManagementApiUrl]]" server-port-for-new-access-keys="[[serverPortForNewAccessKeys]]" is-access-key-port-editable="[[isAccessKeyPortEditable]]" access-key-data-limit="{{accessKeyDataLimit}}" is-access-key-data-limit-enabled="{{isAccessKeyDataLimitEnabled}}" supports-access-key-data-limit="[[supportsAccessKeyDataLimit]]" server-creation-date="[[serverCreationDate]]" server-monthly-cost="[[monthlyCost]]" server-monthly-transfer-limit="[[_formatBytesTransferred(monthlyOutboundTransferBytes)]]" is-server-managed="[[isServerManaged]]" server-location="[[serverLocation]]" metrics-enabled="[[metricsEnabled]]" localize="[[localize]]">


Nit: should we reformat this across across several lines? It's difficult for me to read on my laptop screen.

We lost the formatting during the polymer upgrade. We should look into a way of automating formatting for these mixed (html + css + js) files.

mpmcroy · 2020-06-17T16:24:59Z

src/shadowbox/server/manager_service.ts

+  apiServer.put(
+      `${apiPrefix}/server/access-key-data-limit`, service.setAccessKeyDataLimit.bind(service));
+  apiServer.del(
+      `${apiPrefix}/server/access-key-data-limit`, service.removeAccessKeyDataLimit.bind(service));


I wonder if it's good to model features as a resource like we do with access keys (/server/features/). That would be similar to /experimental/ url part we have.

Ideally, I think it would be nice to have something broken down by component like:

/server/access-key/keys/:id

/server/access-key/features/:id

/server/access-key/custom-methods

But that might not be possible to shoehorn to our current scheme.

https://google.aip.dev/

In a way we already do this: the access key data limit is a property of the server (resource). I like the idea of formalizing the hierarchy.

fortuna

Minor important change to the redirect code.

fortuna · 2020-06-18T14:50:04Z

src/shadowbox/server/manager_service.ts

+}
+
+// Returns a request handler that redirects a bound request path to `url` with HTTP status `code`.
+function redirect(url: string, code = 301): restify.RequestHandlerType {


Please use 308 instead to preserve the HTTP method.

fortuna · 2020-06-18T14:51:45Z

src/shadowbox/server/manager_service.ts

+}
+
+// Returns a request handler that redirects a bound request path to `url` with HTTP status `code`.
+function redirect(url: string, code = 301): restify.RequestHandlerType {


Since you never pass a different code, maybe remove it?

alalamav · 2020-07-07T23:38:27Z

We now display a disclaimer for feature metrics data collection for users with metrics reporting enabled who haven't opened up the app since before the experimental data limits feature was released (video).

cc @cjhenck

fortuna

Looks good. I have some minor readability tweaks.

fortuna · 2020-07-20T18:40:54Z

src/server_manager/web_app/ui_components/outline-server-view.js

@@ -649,6 +650,7 @@ export class ServerView extends DirMixin(PolymerElement) {
      this.isAccessKeyDataLimitEnabled = false;
      /** Whether the server supports data limits. */
      this.supportsAccessKeyDataLimit = false;
+      this.shouldShowFeatureMetricsDisclaimer = false;


You can be shorter: this.showFeatureMetricsDisclaimer

fortuna · 2020-07-20T18:41:46Z

src/server_manager/web_app/ui_components/outline-server-settings.js

@@ -262,6 +272,7 @@ Polymer({
    accessKeyDataLimit: {type: Object, value: null},  // type: app.DisplayDataAmount
    supportsAccessKeyDataLimit:
        {type: Boolean, value: false},  // Whether the server supports data limits.
+    shouldShowFeatureMetricsDisclaimer: {type: Boolean, value: false},


You can be shorter: showFeatureMetricsDisclaimer

fortuna · 2020-07-20T19:12:56Z

src/server_manager/web_app/app.ts

+//   - opt-in metrics are enabled
+//   - user has not previously seen the data limits help bubble
+//   - user has not previously seen this disclaimer
+function shouldShowFeatureMetricsDisclaimer(server: server.Server): boolean {


Let's have a hasConsentForFeatureMetrics or something along those lines. I think it's easier to reason about than having to understand the flows.

It should return dataLimits-feature-collection-consent || dataLimitsHelpBubble-dismissed. Notice it's independent of the server.

Then you can have

view.showFeatureMetricsDisclaimer = server.getMetricsEnabled() && !server.getAccessKeyDataLimit() && !this.hasConsentForFeatureMetrics()

fortuna · 2020-07-20T19:14:06Z

src/server_manager/web_app/app.ts

@@ -1024,6 +1037,9 @@ export class App {
      this.appRoot.showNotification(this.appRoot.localize('saved'));
      serverView.accessKeyDataLimit = dataLimitToDisplayDataAmount(limit);
      this.refreshTransferStats(this.selectedServer, serverView);
+      // Don't display the feature collection disclaimer anymore.
+      serverView.shouldShowFeatureMetricsDisclaimer = false;
+      window.localStorage.setItem('dataLimits-feature-collection-disclaimer', 'true');


Let's rename -disclaimer to -consent. Consent is really what we want to track.

fortuna · 2020-07-20T21:02:22Z

src/server_manager/web_app/app.ts

@@ -88,6 +85,13 @@ async function computeDefaultAccessKeyDataLimit(
  }
 }

+// Returns whether the user has seen a disclaimer for the updated feature metrics data collection
+// policy.
+function hasFeatureMetricsConsent(): boolean {


I'm sorry, I misunderstood on first read. So it's not really consent, because we can't collect the metric unless the metrics sharing is enabled. It's really about notification.
So maybe call it something like notifiedOfFeatureMetrics()

fortuna · 2020-07-20T21:02:50Z

src/server_manager/web_app/app.ts

-      this.surveys.presentDataLimitsEnabledSurvey();
+      // Don't display the feature collection disclaimer anymore.
+      serverView.showFeatureMetricsDisclaimer = false;
+      window.localStorage.setItem('dataLimits-feature-collection-consent', 'true');


Then this would be dataLimits-feature-collection-notification or something similar.

alalamav added 5 commits June 16, 2020 19:35

Move data limits endpoints to /server; redirect experimental endpoints

a7fd50c

Increase shadowbox version

6ad2e2c

Move data limits UI out of experiments section

64caffc

Configure data limits path based on shadowbox version

654547e

Remove data limits availability date and associated surveys

d379d31

alalamav requested review from fortuna, cjhenck and smithmonique June 17, 2020 01:10

alalamav self-assigned this Jun 17, 2020

googlebot added the cla: yes label Jun 17, 2020

mpmcroy reviewed Jun 17, 2020

View reviewed changes

fortuna approved these changes Jun 18, 2020

View reviewed changes

Redirect with 308

cb67732

fortuna approved these changes Jun 18, 2020

View reviewed changes

alalamav added 4 commits July 7, 2020 14:30

Merge branch 'master' into alalama-permanent-data-limits

40b529b

Lint

f1c5e8d

Update permanent data limits version to v1.4.0

b89ca61

Display feature metrics disclaimer when enabling data limits

482152e

Fix typo

66da29b

alalamav requested a review from fortuna July 7, 2020 23:41

alalamav added 2 commits July 8, 2020 12:59

Display disclaimer if user has not seen the data limits help bubble

1397e50

Improve disclaimer language

f84e99c

fortuna reviewed Jul 20, 2020

View reviewed changes

Address review comments

765268e

alalamav requested a review from fortuna July 20, 2020 20:10

fortuna approved these changes Jul 20, 2020

View reviewed changes

Feature metrics: s/consent/notification

ae906a3

fortuna approved these changes Jul 20, 2020

View reviewed changes

alalamav merged commit b63606e into master Jul 20, 2020

alalamav deleted the alalama-permanent-data-limits branch July 20, 2020 21:32

notpushkin mentioned this pull request Mar 18, 2022

can we get traffic stat display in client? Jigsaw-Code/outline-apps#674

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make data limits a permanent feature #698

Make data limits a permanent feature #698

alalamav commented Jun 17, 2020 •

edited

Loading

mpmcroy Jun 17, 2020

alalamav Jun 17, 2020

mpmcroy Jun 17, 2020

alalamav Jun 17, 2020

fortuna left a comment

fortuna Jun 18, 2020

alalamav Jun 18, 2020

fortuna Jun 18, 2020

alalamav Jun 18, 2020

alalamav commented Jul 7, 2020 •

edited

Loading

fortuna left a comment

fortuna Jul 20, 2020

alalamav Jul 20, 2020

fortuna Jul 20, 2020

alalamav Jul 20, 2020

fortuna Jul 20, 2020

alalamav Jul 20, 2020

fortuna Jul 20, 2020

alalamav Jul 20, 2020

fortuna Jul 20, 2020

alalamav Jul 20, 2020

fortuna Jul 20, 2020

alalamav Jul 20, 2020

Make data limits a permanent feature #698

Make data limits a permanent feature #698

Conversation

alalamav commented Jun 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fortuna left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alalamav commented Jul 7, 2020 • edited Loading

fortuna left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alalamav commented Jun 17, 2020 •

edited

Loading

alalamav commented Jul 7, 2020 •

edited

Loading