Referees 2018/scores sync #257

poelstra · 2017-08-14T09:09:02Z

Slightly unrelated to this PR, but instead of (or in addition to) logging the start of a request, it would be helpful to log the completion of a request because that can include the reponse code.

How can I do that? Is there a way to call a function after the routing functions?

So I looked it up, and it is possible. You just need to add next() calls to all the routing functions. It will be a separate PR though

poelstra · 2017-08-27T12:39:48Z

Much better, but still tricky in that you return raw server errors. (minor thing for now)

Very well, since I'm swamp, I think we can leave that for later on

poelstra · 2017-08-27T11:46:30Z

What happens when body is invalid JSON?

An error is thrown, and sent to the client

poelstra · 2017-08-14T09:29:46Z

What happens if the id isn't found? (e.g. conficting edit actions)
Return a more appropriate error to the client than a crash?

-Original file line number
+Diff line change
@@ Expand Up / @@ -20,6 +20,7 @@ src/components/*/* @@
     !src/components/angular-touch/angular*.js
     !src/components/angular-touch/angular*.map
     !src/components/angular-bootstrap/*.js
+    !src/components/angular-storage/*.js
     !src/components/bootstrap-css/css
     !src/components/bootstrap-css/img
@@ Expand Down @@

-Original file line number
+Diff line change
@@ -1,11 +1,17 @@
     language: node_js
+    dist: trusty
+    sudo: true
     node_js:
-      - 0.10
+      - "stable"
     before_install:
       - "npm i -g bower karma-cli"
     before_script:
+      - export CHROME_BIN=/usr/bin/google-chrome
       - export DISPLAY=:99.0
       - sh -e /etc/init.d/xvfb start
+      - sudo apt-get install -y libappindicator1 fonts-liberation
+      - wget https://dl.google.com/linux/direct/google-chrome-stable_current_amd64.deb
+      - sudo dpkg -i google-chrome*.deb
     after_script:
       - ls ./coverage
       - 'npm install coveralls@2.10.0 && cat "./coverage/Firefox 31.0.0 (Linux)/lcov.info" | coveralls'

-Original file line number
+Diff line change
@@ Expand Up / @@ -17,6 +17,7 @@ @@
       "dependencies": {
         "angular": "~1.2.20",
         "angular-bootstrap": "~0.11.0",
+        "angular-local-storage": "~0.6.0",
         "angular-mocks": "~1.2.20",
         "angular-sanitize": "~1.3.3",
         "angular-touch": "~1.3.4",
@@ Expand Down @@

-Original file line number
+Diff line change
@@ Expand Up / @@ -17,6 +17,7 @@ module.exports = function(config) { @@
           'src/components/jquery/jquery.min.js',
           'src/components/angular/angular.min.js',
           'src/components/angular-mocks/angular-mocks.js',
+          'src/components/angular-storage/angular-storage.js',
           'src/components/q/q.js',
           'src/components/idbwrapper/idbstore.js',
           'spec/helpers/*.js',
@@ Expand Down @@

-Original file line number
+Diff line change
@@ Expand Up / @@ -37,5 +37,5 @@ routers.forEach(function(router) { @@
     app.listen(args.port, function() {
         console.log('Listening on port ', args.port);
-        console.log('open browser to http://localhost:{0}/'.format(args.port));
+        console.log(`open browser to http://localhost:${args.port}/`);
     });

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Referees 2018/scores sync #257

Uh oh!

Diff view

Diff view

There are no files selected for viewing

poelstra Aug 14, 2017

Uh oh!

idanstark42 Aug 14, 2017

Uh oh!

poelstra Aug 14, 2017

Uh oh!

idanstark42 Aug 28, 2017

Uh oh!

poelstra Aug 27, 2017

Uh oh!

idanstark42 Aug 27, 2017

Uh oh!

poelstra Aug 27, 2017

Uh oh!

idanstark42 Aug 27, 2017

Uh oh!

poelstra Aug 14, 2017

Uh oh!

Uh oh!

-Original file line number
+Diff line change
@@ Expand Up / @@ -5,7 +5,7 @@ @@
       "main": "index.js",
       "scripts": {
         "start": "node localserver",
-        "test": "./node_modules/.bin/karma start --single-run --browsers Firefox --reporters dots,coverage"
+        "test": "./node_modules/.bin/karma start --single-run --reporters dots,coverage"
       },
       "repository": {
         "type": "git",
@@ Expand All / @@ -30,6 +30,7 @@ @@
       },
       "dependencies": {
         "basic-auth-connect": "~1.0.0",
+        "lockfile": "~1.0.3",
         "uuid": "^3.1.0",
         "express": "^4.10.4",
         "express-session": "^1.5.0",
@@ Expand Down @@

-Original file line number
+Diff line change
@@ Expand Up / @@ -5,7 +5,7 @@ exports.route = function(app) { @@
         //get challenges over xhr, for hosted service
         app.get('/challenge/:year', function(req, res) {
-            var file = fileSystem.resolve('challenges/js/{0}.js'.format(req.params.year));
+            var file = fileSystem.resolve(`challenges/js/${req.params.year}.js`);
             res.header('Content-Type','text/plain');
             res.sendFile(file);
@@ Expand Down @@

-Original file line number
+Diff line change
@@ Expand Up / @@ -33,7 +33,6 @@ exports.readFile = function(file) { @@
                 if (exists) {
                     resolve(exists);
                 } else {
-                    log.error("file not found {0}".format(file));
                     reject({
                         status: 404,
                         message: 'file not found'
@@ Expand All / @@ -42,7 +41,6 @@ exports.readFile = function(file) { @@
             });
         }).then(function() {
             return Q.nfcall(fs.readFile, file, "utf-8").catch(function(e) {
-                log.error("error reading file {0}".format(file));
                 throw new Error({
                     status: 500,
                     message: 'error reading file'
@@ Expand Down Expand Up / @@ -77,24 +75,21 @@ exports.route = function(app) { @@
             var file = exports.getDataFilePath(req.params[0]);
             fs.stat(file, function(err, stat) {
                 if (err) {
-                    log.error("file not found {0}".format(file));
-                    res.status(404).send('file not found');
+                    utils.sendError(res, { status: 404, message: `file not found ${file}` })
                     return;
                 }
                 if (stat.isFile()) {
                     fs.readFile(file, function(err, data) {
                         if (err) {
-                            log.error("error reading file {0}".format(file));
-                            res.status(500).send('error reading file');
+                            utils.sendError(res, { status: 500, message: `error reading file ${file}` })
                             return;
                         }
                         res.send(data);
                     });
                 } else if (stat.isDirectory()) {
                     fs.readdir(file, function(err, filenames) {
                         if (err) {
-                            log.error("error reading dir {0}".format(file));
-                            res.status(500).send('error reading dir');
+                            utils.sendError(res, { status: 500, message: `error reading dir ${file}` })
                             return;
                         }
                         // FIXME: this doesn't work for filenames containing
@@ Expand All / @@ -103,15 +98,13 @@ exports.route = function(app) { @@
                             return name.indexOf("\n") >= 0;
                         });
                         if (hasNewline) {
-                            log.error("invalid filename(s) {0}".format(filenames.join(', ')));
-                            res.status(500).send('invalid filename(s)');
+                            utils.sendError(res, { status: 500, message: `invalid filename(s) ${filenames.join(', ')}` })
                             return;
                         }
                         res.send(filenames.join('\n'));
                     });
                 } else {
-                    log.error("error reading file {0}".format(file));
-                    res.status(500).send('error reading file');
+                    utils.sendError(res, { status: 500, message: `error reading file ${file}` })
                     return;
                 }
             });
@@ Expand All / @@ -123,8 +116,7 @@ exports.route = function(app) { @@
             exports.writeFile(file, req.body).then(function() {
                 res.status(200).end();
             }).catch(function(err) {
-                log.error("error writing file {0}".format(err));
-                res.status(500).send('error writing file');
+                utils.sendError(res, { status: 500, message: `error writing file ${file}` })
             });
         });
@@ Expand All / @@ -133,8 +125,7 @@ exports.route = function(app) { @@
             var file = exports.getDataFilePath(req.params[0]);
             fs.unlink(file, function(err) {
                 if (err) {
-                    log.error("error removing file {0}".format(err));
-                    res.status(500).send('error removing file');
+                    utils.sendError(res, { status: 500, message: `error removing file ${file}` })
                 }
                 res.status(200).end();
             });
@@ Expand Down @@

-Original file line number
+Diff line change
@@ -0,0 +1,33 @@
+    //This module wraps lockfile with promises.
+    var lockfile = require('lockfile');
+    module.exports = function(filename, options) {
+        this.filename = filename;
+        this.options = options || {};
+        this.lock = function() {
+            var self = this;
+            return new Promise(function(resolve, reject) {
+                    lockfile.lock('scores.json.lock', self.options, function(err) {
+                    if(err !err.startsWith('EEXIST')) {
+                        reject(err);
+                    }
+                    resolve();
+                });
+            });
+        };
+        this.unlock = function() {
+            return new Promise(function(resolve, reject) {
+                lockfile.unlock('scores.json.lock', function(err) {
+                    if(err && !err.startsWith('EEXIST')){
+                        reject(err);
+                    }
+                    resolve();
+                });
+            });
+        };
+    }

-Original file line number
+Diff line change
@@ Expand Up / @@ -12,7 +12,7 @@ exports.middleware = function(req, res, next) { @@
         req.log = exports.log;
-        req.log.debug('Starting {0} {1}'.format(req.method, req.originalUrl));
+        req.log.debug(`Starting ${req.method} ${req.originalUrl}`);
         next();
@@ Expand Down @@

-Original file line number
+Diff line change
@@ -1,6 +1,9 @@
+    var Lock = require('./lock');
     var utils = require('./utils');
     var fileSystem = require('./file_system');
+    var log = require('./log').log;
     var Q = require('q');
+    var id = require('uuid/v4');
     function filterPublished(score) {
         return score.published;
@@ Expand All / @@ -15,6 +18,45 @@ function reduceToMap(key) { @@
         }
     }
+    /**
+     * Atomically change scores file.
+     * Action callback is called with the current contents of the scores file, and is expected
+     * to return the new contents (or a Promise for it).
+     * A lock is acquired and held during the entire operation.
+     * @param action Callback that receives current scores.json object, must return new contents (or Promise for it)
+     * @return Promise for updated scores object
+     */
+    function changeScores(action) {
+        var path = fileSystem.getDataFilePath('scores.json');
+        var lock = new Lock('scores.json.lock', { retries: 5, retryWait: 100 });
+        console.log(lock.options);
+        return lock.lock()
+        .then(() => fileSystem.readJsonFile(path))
+        .catch((err) => { //Ignoring all file not found errors, and just returning empty scores.json
+            console.log("catching");
+            if(err.message === 'file not found') {
+                console.log("hells yeah!");
+                return { version:3, scores: [] };
+            } else {
+                console.log("ho no! " + err.message);
+                throw err;
+            }
+        })
+        .then(action)
+        .then(scores => {
+            return fileSystem.writeFile(path, JSON.stringify(scores))
+            .then(() => {
+                return lock.unlock();
+            }).catch((err) => {
+                return lock.unlock();
+            }).then(function() {
+                return scores;
+            });
+        });
+    }
     exports.route = function(app) {
         //get all, grouped by round
@@ Expand All / @@ -32,7 +74,7 @@ exports.route = function(app) { @@
                     return rounds;
                 },{});
                 res.json(published);
-            }).catch(utils.sendError(res)).done();
+            }).catch(err => utils.sendError(res, err)).done();
         });
         //get scores by round
@@ Expand All / @@ -44,7 +86,84 @@ exports.route = function(app) { @@
                     return score.published && score.round === round;
                 });
                 res.json(scoresForRound);
-            }).catch(utils.sendError(res)).done();
+            }).catch(err => utils.sendError(res, err)).done();
+        });
+        //save a new score
+        app.post('/scores/create',function(req,res) {
+            var body = JSON.parse(req.body);
+            var scoresheet = body.scoresheet;
+            var score = body.score;
+            fileSystem.writeFile(fileSystem.getDataFilePath("scoresheets/" + score.file), req.body)
+            .then(changeScores(function(result) {
+                if(typeof(score.id) === 'undefined') {
+                    score.id = id();
+                }
+                result.scores.push(score);
+                return result;
+            }))
+            .then(function(scores) {
+                res.json(scores).end();
+            }).catch(err => utils.sendError(res, err)).done();
+        });
+        //delete a score at an id
+        app.post('/scores/delete/:id',function(req,res) {
+            changeScores(function(result) {
+                var index = result.scores.findIndex((score) => score.id === req.params.id);
+                if(index === -1) {
+                    throw new Error(`Could not find score with id ${req.params.id}`);
+                }
+                result.scores.splice(index, 1);
+                return result;
+            }).then(function(scores) {
+                res.json(scores).end();
+            }).catch(err => utils.sendError(res, err)).done();
         });
+        //edit a score at an id
+        app.post('/scores/update/:id',function(req,res) {
+            var score = JSON.parse(req.body);
+            changeScores(function(result) {
+                var index = result.scores.findIndex((score) => score.id === req.params.id);
+                if(index === -1) {
+                    throw new Error(`Could not find score with id ${req.params.id}`);
+                }
+                result.scores[index] = score;
+                return result;
+            }).then(function(scores) {
+                res.json(scores).end();
+            }).catch(err => utils.sendError(res, err)).done();
+        });
     };
+    // For backward compatibility
+    changeScores(function(scores) {
+        if(typeof(scores.version) === 'undefined') {
+            log.warn('Deprecated scores version. Updating to version 3.')
+            scores.forEach(score => score.id = id())
+            return {
+                version: 3,
+                scores: scores
+            }
+        } else if(scores.version === 3) {
+            return scores;
+        } else if(scores.version === 2) {
+            log.warn('Deprecated scores version. Updating to version 3.');
+            scores.scores.forEach(score => score.id = id())
+            scores.version = 3;
+            return scores;
+        } else {
+            throw new Error('Unkown scores version');
+        }
+    });

-Original file line number
+Diff line change
@@ Expand Up / @@ -7,7 +7,7 @@ exports.route = function(app) { @@
         app.get('/teams',function(req,res) {
             fileSystem.readJsonFile(fileSystem.getDataFilePath('teams.json')).then(function(result) {
                 res.json(result);
-            }).catch(utils.sendError(res)).done();
+            }).catch(err => utils.sendError(res, err)).done();
         });
         app.get('/teams/:nr',function(req,res) {
@@ Expand All / @@ -16,7 +16,7 @@ exports.route = function(app) { @@
                     return team.number == req.params.nr;
                 })[0];
                 res.json(team);
-            }).catch(utils.sendError(res)).done();
+            }).catch(err => utils.sendError(res, err)).done();
         });
     };

-Original file line number
+Diff line change
@@ -1,16 +1,11 @@
-    exports.root = __dirname + '/../';
-    exports.sendError = function(res) {
-        return function(err) {
-            res.status(err.status).send(err.message);
-        }
-    }
-    if (!String.prototype.format) {
-        String.prototype.format = function() {
-            var args = arguments;
-            return this.replace(/{(\d+)}/g, function(match, number) {
-                return typeof args[number] !== 'undefined' ? args[number] : match;
-            });
-        };
-    }
+    var log = require('./log').log;
+    exports.root = __dirname + '/../';
+    exports.sendError = function(res, err) {
+        var status = err.status || 500;
+        var message = err.message || "Internal server error"
+        log.error(message);
+        res.status(status).send(message);
+    }

Referees 2018/scores sync #257

Are you sure you want to change the base?

Uh oh!

Referees 2018/scores sync #257

Uh oh!

Uh oh!

Diff view

Diff view

There are no files selected for viewing

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!