add notes from our review

lib/decrees.js

@@ -27,6 +27,7 @@ DISABLE_INTEGRATED_EVICTION
 // BROADCAST
 SET_LAST_BROADCAST_HASH
 SET_SURVEY_URL
+SET_MAINTENANCE
 
 NOT IMPLEMENTED:
 
@@ -129,10 +130,10 @@ var args_isString = function (args) {
     return Array.isArray(args) && typeof(args[0]) === "string";
 };
 var args_isMaintenance = function (args) {
-    return Array.isArray(args) && args[0] && args[0].end && args[0].start;
+    return Array.isArray(args) && args[0] && args[0].end && args[0].start; // XXX we could validate that these are numbers && !isNaN
 };
 
-var makeBroadcastSetter = function (attr) {
+var makeBroadcastSetter = function (attr) { // XXX could pass extra validation here?
     return function (Env, args) {
         if (!args_isString(args) && !args_isMaintenance(args)) {
             throw new Error('INVALID_ARGS');
@@ -149,7 +150,7 @@ var makeBroadcastSetter = function (attr) {
 commands.SET_LAST_BROADCAST_HASH = makeBroadcastSetter('lastBroadcastHash');
 
 // CryptPad_AsyncStore.rpc.send('ADMIN', [ 'ADMIN_DECREE', ['SET_SURVEY_URL', [url]]], console.log)
-commands.SET_SURVEY_URL = makeBroadcastSetter('surveyURL');
+commands.SET_SURVEY_URL = makeBroadcastSetter('surveyURL'); // XXX anticipate language-specific surveys
 
 // CryptPad_AsyncStore.rpc.send('ADMIN', [ 'ADMIN_DECREE', ['SET_MAINTENANCE', [{start: +Date, end: +Date}]]], console.log)
 // CryptPad_AsyncStore.rpc.send('ADMIN', [ 'ADMIN_DECREE', ['SET_MAINTENANCE', [""]]], console.log)

server.js

@@ -113,8 +113,7 @@ var setHeaders = (function () {
 
             // Don't set CSP headers on /api/config because they aren't necessary and they cause problems
             // when duplicated by NGINX in production environments
-            // XXX /api/broadcast too?
-            if (/^\/api\/config/.test(req.url)) { return; }
+            if (/^\/api\/(broadcast|config)/.test(req.url)) { return; }
             // targeted CSP, generic policies, maybe custom headers
             const h = [
                     /^\/common\/onlyoffice\/.*\/index\.html.*/,
@@ -273,7 +272,7 @@ var serveConfig = (function () {
     };
 }());
 
-var serveBroadcast = (function () {
+var serveBroadcast = (function () { // XXX deduplicate
     var cacheString = function () {
         return (Env.FRESH_KEY? '-' + Env.FRESH_KEY: '') + (Env.DEV_MODE? '-' + (+new Date()): '');
     };
@@ -284,7 +283,7 @@ var serveBroadcast = (function () {
             maintenance = undefined;
         }
         return [
-            'define(function(){',
+            'define(function(){', // XXX maybe this could just be JSON
             'return ' + JSON.stringify({
                 lastBroadcastHash: Env.lastBroadcastHash,
                 surveyURL: Env.surveyURL,

www/admin/app-admin.less

@@ -14,6 +14,10 @@
 
     display: flex;
     flex-flow: column;
+    a {
+        color: @cryptpad_color_link;
+        text-decoration: underline;
+    }
 
     .cp-admin-setlimit-form, .cp-admin-broadcast-form {
         label {

www/admin/inner.js

@@ -970,6 +970,7 @@ define([
     var getApi = function (cb) {
         return function () {
             require(['/api/broadcast?'+ (+new Date())], function (Broadcast) {
+                // XXX require.s.contexts._ can be used to erase old loaded objects
                 cb(Broadcast);
             });
         };
@@ -1219,7 +1220,7 @@ define([
                 }
                 if (error) {
                     console.error('One of the selected languages has no data');
-                    return false;
+                    return false; // XXX better error handling?
                 }
                 return {
                     defaultLanguage: defaultLanguage,
@@ -1229,7 +1230,7 @@ define([
 
             var send = function (data) {
                 $button.prop('disabled', 'disabled');
-                data.time = +new Date();
+                data.time = +new Date(); // XXX not used anymore?
                 common.mailbox.sendTo('BROADCAST_CUSTOM', data, {}, function (err /*, data */) { // XXX unused argument
                     if (err) {
                         $button.prop('disabled', '');
@@ -1249,7 +1250,7 @@ define([
                 send(data);
             });
 
-            UI.confirmButton(removeButton, {
+            UI.confirmButton(removeButton, { // XXX table jank
                 classes: 'btn-danger',
             }, function () {
                 if (!activeUid) { return; }
@@ -1311,7 +1312,8 @@ define([
             var end = h('input');
             var $start = $(start);
             var $end = $(end);
-            var endPickr = Flatpickr(end, {
+            // XXX new Date().toLocaleString('fr-fr', {month: 'long'}).replace(/./, c => c.toUpperCase())
+            var endPickr = Flatpickr(end, { // XXX translations?
                 enableTime: true,
                 minDate: new Date()
             });
@@ -1411,7 +1413,7 @@ define([
                     common.openUnsafeURL(Broadcast.surveyURL);
                 });
                 active = h('div.cp-broadcast-active', [
-                    h('p', a),
+                    h('p', a), // XXX spacing around this element is really cramped
                     removeButton
                 ]);
             }

www/common/application_config_internal.js

@@ -162,7 +162,7 @@ define(function() {
     // making it much faster to open new tabs.
     config.disableWorkers = false;
 
-    //config.surveyURL = "";
+    //config.surveyURL = ""; // XXX remove this?
 
     // Teams are always loaded during the initial loading screen (for the first tab only if
     // SharedWorkers are available). Allowing users to be members of multiple teams can

www/common/notifications.js

@@ -466,7 +466,7 @@ define([
 
     return {
         add: function(common, data) {
-            console.log(data);
+            console.log(data); // XXX noise?
             var type = data.content.msg.type;
 
             if (handlers[type]) {

www/common/outer/async-store.js

@@ -3156,7 +3156,7 @@ define([
             });
         };
 
-        Store.newVersionReload = function () {
+        Store.newVersionReload = function () { // XXX not used anymore?
             broadcast([], "NETWORK_RECONNECT");
         };
         Store.disconnect = function () {

www/common/outer/mailbox.js

@@ -341,7 +341,7 @@ proxy.mailboxes = {
                 };
                 var notify = box.ready;
                 Handlers.add(ctx, box, message, function (dismissed, toDismiss, setAsLKH) {
-                    if (setAsLKH) {
+                    if (setAsLKH) { // XXX confirm whether this if is used?
                         // Update LKH
                         box.data.lastKnownHash = hash;
                         box.data.viewed = [];

www/common/toolbar.js

@@ -1032,7 +1032,7 @@ MessengerUI, Messages) {
     Messages.broadcast_maintenance = "A maintenance is planned between <b>{0}</b> and <b>{1}</b>"; // XXX
     var createMaintenance = function (toolbar, config) {
         var $notif = toolbar.$top.find('.'+MAINTENANCE_CLS);
-        var button = h('button.cp-maintenance-wrench.fa.fa-wrench');
+        var button = h('button.cp-maintenance-wrench.fa.fa-wrench'); // XXX might need some color contrast
         $notif.append(button);
 
 

www/support/ui.js

@@ -445,7 +445,7 @@ define([
         };
         ctx.FM = common.createFileManager(fmConfig);
 
-        ui.send = function (id, type, data, dest) {
+        ui.send = function (id, type, data, dest) { // XXX confirm that this is actually used
             return send(ctx, id, type, data, dest);
         };
         ui.sendForm = function (id, form, dest) {