Referees 2018/scores sync #257

idanstark42 · 2017-08-09T14:06:08Z

Hi all,
So as you see I'm back with this branch, and that's because fixing it did took me allot shorter then creating a new one.
I tried creating the shared code between the client and the server, but as I saw, there isn't enough code to share, just pieces of code. If I was to finish it, what you would see is a massive pile of dependency injections for it to be generic. So, I decided not to go there, and to simply fix the problem that was with the last request: the client couldn't operate on his own.
So, After splitting the ng-scores into several services (and one factory) for SRP purposes, I added the $independence service, to give us the ability to save actions that couldn't be sent to the server, and redo them later on, in case the server goes back up.

…esh scores button

…tton

… the server

… and back-end

…an get to it.

…s to fix

Bugfixes

poelstra · 2017-08-27T11:36:35Z

src/js/services/ng-scores.js

-                        // New style storage, scores in a property,
-                        // and an explicit version identifier.
-                        version = res.version;
-                        scores = res.scores;


Why was the old version removed? It is useful to allow old data to be loaded for testing purposes.
Seems it should be fairly easy to keep this backwards compatibility, too?

Because version 3 uses id, not index. Working with index can easily cause errors, and I thought it would be best to not support it, by that forcing the users to use id oriented scores

Ah. I think it would be better to have the server do a one-time version-upgrade then, e.g. when it starts, such that it can add (random) ids to each entry.

poelstra

Next part of the review.
I didn't get to review the actual guts like the new scores/ranking/independence etc.

I also didn't run any of it yet.

It would be helpful for me if you somehow address all of my comments, even if you disagree. i.e. either 'fix' it, or leave a reply with why you don't want to fix it. I will consider everything that's unanswered as pending.

poelstra · 2017-08-27T11:38:57Z

server_modules/scores.js

@@ -15,6 +16,32 @@ function reduceToMap(key) {
    }
 }

+function changeScores(action) {
+    var path = fileSystem.getDataFilePath('scores.json');


Typically better to move all code of a function to inside the Promise chain, in case it throws. Otherwise, the caller needs to check for both promise rejections AND synchronous errors. Probably not worth the trouble in this case though (after you refactor the rest of this function to accomodate the feedback).

Will change that

poelstra · 2017-08-27T11:39:04Z

server_modules/scores.js

@@ -15,6 +16,32 @@ function reduceToMap(key) {
    }
 }

+function changeScores(action) {


Add doc comment.

That's not a doc comment. I would expect something like:

/** * 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 */

i.e. don't make the user (me) parse the code to understand how to use it, and don't make 'handwavy' comments like "using lockfile and a promise chain".

poelstra · 2017-08-27T11:41:15Z

server_modules/scores.js

+                return data;
+            })
+            .catch(function() {
+                return { version:3, scores: [] };


Very tricky: if someone e.g. manually editted the file, but made a small JSON mistake, the whole file will be thrown away. Make this an explicit check on e.g. file not found-style errors, and rethrow all other errors.

You're right. I added a condition here

poelstra · 2017-08-27T11:41:27Z

server_modules/scores.js

+
+            promise = fileSystem.readJsonFile(path)
+            .then(function(data) {
+                return data;


This step is a no-op. Remove.

poelstra · 2017-08-27T11:42:50Z

server_modules/scores.js

+            })
+            .then(action)
+            .then(function(scores) {
+                fileSystem.writeFile(path, JSON.stringify(scores));


writeFile returns a promise, which is ignored here, thus the unlock will continue without waiting for it.

You're right. I changed that

poelstra · 2017-08-27T13:46:49Z

src/js/views/scores.js

+            $scope.$watch(function() {
+                return $scores.scores;
+            }, function() {
+                $scope.scores = enrich($scores.scores);


This seems very tricky: changing the same variable that you're watching.
Although it may work, I strongly suggest putting the 'non-enriched' scores in e.g. $scores.rawScores (or $scores.nonEnrichedScores).

The mistake here is changing the original, which is fixed after the above comment

Oh, you're right, I didn't see the difference between $scope and $scores...

poelstra · 2017-08-27T13:56:43Z

src/js/views/scoresheet.js

-                return $fs.write("scoresheets/" + fn,data).then(function() {
-                    log('result saved');
+                return $scores.create(data).then(function() {
+                    log('result saved: ');


why the colon?

For the notice message

Don't see another log call being made there?

No, that's my bad

poelstra · 2017-08-27T13:57:23Z

src/js/views/scoresheet.js

+                        ` ${data.team.name} in ${data.stage.name} ${data.round}.`;
+                    $window.alert(message);
+                }).catch(function(err) {
+                    log('result saved: ');


Shouldn't this log the error?

Right, sorry. My bad

poelstra · 2017-08-27T14:09:33Z

src/js/views/scoresheet.js

-                    data.uniqueId
-                ].join('_')+'.json';
+                data.scoreEntry.score = $scope.score();
+                data.scoreEntry.calcFilename();


I'm not sure calcFilename() should be in scoreEntry: we're saving a scoresheet here, so theoretically, we'd also want to e.g. include the referee in the filename.

Also, the scoresheet is a whole different beast from a score entry (as you already noted yourself, because you did move e.g. stage, round, but not referee or signature).

The scoreEntry is not a 'subset' of the data of a scoresheet (thus not a simple property of it), but a really different thing. For example, it includes published status, whether it was editted, etc.
It is of course possible to create a new scoreEntry by filling some of its properties from a scoresheet.

Therefore, I'm not a big fan of changing the score property from a number to a scoreEntry object.
In fact, I would strongly suggest reverting this part of the changes here.

Then the server code must also be changed, by no longer picking the 'score' property out of the scoresheet object.
I consider this a good thing: the client should post the scoresheet, and can then separately post the score entry.
(Note that that is not as hard as it sounds, because if any of the two fail, both can be retried, even if the scoresheet post already worked.)

I don't think it's hard change, but I do disagree with it.
The score entry may have some status property that are used in later steps of the score's life cycle, but it is being born out of a scoresheet.
Because of that, I tought it would be most efficient and logical to start the progress of the score entry with the scoresheet. The code here, is of course temporary (it really doesn't look that good, does it?) because The idea that came behind this, is to make the $score factory, and object the starts with the score from the moment it's created (as a scoresheet) to moment it's being saved in the server, and in later editing. After all, it is the same score from the beginning.
Notice that it's not the server that extrudes them, but the $scores service, which will make it easier to change it later to a single full-process object

Scoresheets are really just one of the ways in which a score can be born. Another is to manually enter them in the Scores overview.

Another use-case can be found in #261. When a scoresheet with a team signature is 'editted', a new scoresheet must be created. However, both scoresheets will refer to the same score entry.

Also, any property of the score entry can be changed independent of the scoresheet, and when re-opening the scoresheet, that sheet must appear exactly as it was when the team signed it, even if it contained mistakes.

So although we can discuss about where to best 'convert' or 'sync' or instantiate one to the other, they are really different, and one is never just a subset of the other.

I don't look at it a a subset, as I said, this is just temporary. (Iv'e got a little time now so I might be able to change that to show you what I mean, but it'll take me about a week). I'm saying that the scoresheet and the score are two states of the same entity. One is the state of creation, and the other is in the state of data manipulation.
I agree that the score can be created independently. That doesn't destroy the case that they are both the same entity, just in different forms.

The way I see it:
The scoresheet is the mission points which the team singed off on.
The score is the editable version of that.
I can see why that would seem like two different things, since they can change indepenetly (which make me wonder - why can they change independently? If the team singed on something, it should be permanent. It even says so in the game rules every year. If something like that is edited, it should be only on team aggrement - like you said in #261 it should be applied to all data, and it should be logged that it changed), but even if they do, that doesn't mean they don't represent the same score, just in different phases of its cycle.

Anyway, I'll try to show you what I mean by creating a score object that follows a score entry from the moment it's an empty scoresheet (or an entry of some other kind) to the moment it's edited, with full logs to show the changes that were applied to it.

I thought about it, and I don't have the time to change it as I would want to, so I took it back to the way it was (Or the closest to it that I could)

poelstra · 2017-08-27T14:12:24Z

src/views/pages/ranking.html

@@ -69,12 +69,12 @@ <h1 ng-if="stage.rounds > 0">
                    </tr>
                </thead>
                <tbody>
-                    <tr ng-repeat="item in scoreboard[stage.id] | index | orderBy:stage.sort:stage.rev track by item.team.number">
+                    <tr ng-repeat="item in scoreboard[stage.id]">


sorting is now broken?

Yes. It's brought back in Itay Dafner's PR

Apparently it was broken in the 'small refactoring' PR, and I didn't see it due to caching issues.
I've pushed a fix on master. Just cherry-pick that one for now, and leave unrelated stuff like this out of the PR.

poelstra · 2017-08-27T14:24:35Z

BTW, please excuse the terse wording. Again, you've done great work!
Some comments may come over as harsh, but are definitely not intended that way, just written quickly :)

poelstra

Some more remarks.

poelstra · 2017-08-28T15:10:44Z

server_modules/scores.js

+                    if(err) rej(err);
+                });
+                return scores;
+            }).then(res).catch(rej);


You can, and you should.
Again, you have to think differently: the first lockfile-stuff should already do resolve/reject on the promise, then the next part can start, etc.

This is very tricky to get right though, so here's my attempt at it (warning: didn't run or compile this code myself, so please doublecheck!)

/** * Create lockfile for given path. * @param path Path to lockfile (e.g. "something.json.lock") * @return Promise that resolves when lockfile is created */ function lockFile(path) { return new Promise((res, rej) => { lockfile.lock(path, { retries: 5, retryWait: 100 }, (err) => { if (err) { return rej(err); } res(); }); }) } /** * Unlock lockfile for given path. * @param path Path to lockfile (e.g. "something.json.lock") * @return Promise that resolves when lockfile is unlocked. */ function unlockFile(path) { return new Promise((res, rej) => { lockfile.unlock(path, (err) => { if (err) { return rej(err); } res(); }); }) } /** * 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 scoresPath = fileSystem.getDataFilePath('scores.json'); var lockfilePath = scoresPath + '.lock'; var scores; return lockFile(lockfilePath) .then(() => fileSystem.readJsonFile(scoresPath)) .catch(function (err) { if (err.status === 404) { // File doesn't exist, return initial version return { version: 3, scores: [] }; } throw err; }) .then(action) .then((scores) => { scores = scores; return fileSystem.writeFile(scoresPath, JSON.stringify(scores)) }) .then( () => unlockFile(lockfilePath), (err) => { // always unlock, also in case of errors, but make sure // the original error is still returned return unlockFile(lockfilePath) .then( () => { throw err; }, () => { throw err; } ); } ) .then(() => scores); }

I did quite a lot of things differently:

lockfile is now created next to the file it's about, which makes it more robust when multiple processes (i.e. non-fllscoring) would use this mechanism.

If an error happens in the more interesting parts (the action and/or writeFile), that error is indeed returned, instead of being swallowed by the unlock

The file-not-found code checks for a more stable thing (message is typically a human-readable thing and should not be considered reliable)

Pulled the lockfile stuff to its own functions to promisify them

Added doc comments

That whole 'tail' with the rethrowing of errors isn't looking very nice, but it's a bit of a consequence of using native promises (that don't have handy helpers) and the fact that we're trying to 'bypass' the unlock, but still want to wait for its completion.

poelstra · 2017-08-28T15:19:05Z

src/js/services/ng-message.js

-                        if(Object.keys(data).length === 0)
-                            delete data.data;
+                        msg.from = headers["scoring-token"];;
+                        msg.fromMe = msg.from === token;


Rather ugly to mutate msg. It smells a bit like bad design. I would expect from and fromMe to only be used inside this file. Possibly the listeners should indicate whether they are interested in hearing their own stuff back.

Alternatively, you could just do that extra scores load ;)

I see what you're saying. It's more of a metadata header. Some listeners might want to receive both local and remote messages, but want to treat them differently. Also they might want to know who sent it. I wanted to expose that data to the listener to let him decide for itself what it would like to do with it. In our example, yes, it does decide no to listen to it. But I can think of different examples where that's not the case

poelstra · 2017-08-28T15:21:12Z

src/js/views/scores.js

+            $scope.$watch(function() {
+                return $scores.scores;
+            }, function() {
+                $scope.scores = enrich($scores.scores);


Oh, you're right, I didn't see the difference between $scope and $scores...

poelstra · 2017-08-28T15:22:55Z

src/js/views/scores.js

            $scope.$watch(function() {
                return $scores.scores;
            }, function() {
                $scope.scores = enrich($scores.scores);
            });

+            $scores.init().then(function() {
+                $scope.stages = $stages.stages;


We didn't init $stages...

$scores.init initializes $stages

Changed that

…into referees-2018/scores_sync

idanstark42 added 30 commits July 31, 2017 23:34

Added score CRUD routing

2b6275b

Using the scores API in the client

39d9422

Added locking mechanism on scores.json

698c517

Now that the page is saved in a cookie, there's no need for the refer…

d13338c

…esh scores button

Now that the pollSheets is automatic, there's no need for the poll bu…

bff8e14

…tton

Added angular-local-stroage depedency

42e1d5e

Added the angular-local-storage lib

bdaae48

Using angular-storage instead: it's more reliable

1ef02c4

Using local storage to save a queue of scoresheet ready to be sent to…

3865c07

… the server

Added message to explain about the scoresheet-queue

d5f16d2

CR fixes: removed String.format and fixed typos

54f76df

Added common library and the ability to access it from both front-end…

b1d0419

… and back-end

Moved common to the correct location - where both client and server c…

7c2cff7

…an get to it.

Added scores to commong area

b4cf42e

Changed to saving all actions

eb38315

Added saving other actions as well

9e4adf6

Finished big migration towards multi-service core. There are some bug…

5b99c53

…s to fix

Fixed some bugs: rankings now works

c54e452

Removed old common library - it became angular services

ef1d2dc

Little bugfix

cf44e67

Removed common library

f95e5be

Showing only existing rankings in the rankings view

ca9a558

working scoresheet insertion

4f99d87

Fixed some bugs - now the scores update accordingly

59c3b4f

Added listening funciton to the module

74e85cc

Cleaning after scores remodeling mess: now it works

e85c1da

It's better to splice an array!

4d36900

Added sync between clients

288f373

Merge branch 'master' into referees-2018/scores_sync

d9fdc9c

Removing server module for common - we don't need it anymore

7d81136

idanstark42 added 5 commits August 26, 2017 19:27

Fixed error message on scoresheet

f46345e

Merge pull request FirstLegoLeague#11 from idanstark42/bugfixes

b9e8b29

Bugfixes

Fixed error dealing in scores andin general

c4d8d26

Removed backward compatibility: working only with ids from now on

0bb4427

UT

e70f572

poelstra reviewed Aug 27, 2017

View reviewed changes

poelstra suggested changes Aug 27, 2017

View reviewed changes

idanstark42 added 3 commits August 27, 2017 21:27

CR fixes

d053b27

Added ranking filter

8307c42

Merge branch 'server_exceptions' into referees-2018/scores_sync

2cc771a

poelstra reviewed Aug 28, 2017

View reviewed changes

idanstark42 added 17 commits August 28, 2017 22:44

Merge branch 'master' of https://github.com/FirstLegoLeague/fllscoring …

3b5dc2b

…into referees-2018/scores_sync

CR changes

45c5b7b

Fixed some bugs with the lock module

e487f95

Removed unused statement

cd2503c

prettifing code

cd56007

Removed this mock: it's a part of the unit and should be tested as well

e72cde7

Using catch instead of second then argument

ff4592b

Added clearing the localStorage between runs

ba18987

Skipping pendingActions tests

5ac279f

Extracted score entry from scoresheet

ac25467

Added backward comaptibility in scores.json versions

3d128dc

Made it work

47cc7cd

Explicit call to .init

777d7ee

Running backward compatibility only on server start

b5d5c8a

Ignoring existing file error in locks

d6199bb

Adding id to scores that don't have it on creation

10752c3

Added deprecation warning to scores json

19aa792

idanstark42 mentioned this pull request Sep 5, 2017

Scores seamless sync #277

Open

Referees 2018/scores sync #257

Are you sure you want to change the base?

Referees 2018/scores sync #257

Conversation

idanstark42 commented Aug 9, 2017

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

poelstra 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

poelstra Aug 28, 2017 • 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

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

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

poelstra commented Aug 27, 2017

poelstra left a comment • 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

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

poelstra Aug 28, 2017 •

edited

Loading

poelstra left a comment •

edited

Loading