FirstLegoLeague / fllscoring

FLL Scoring app. See Demo:
http://firstlegoleague.github.io/fllscoring/
GNU General Public License v2.0
14 stars 19 forks source link

scores: Enable automatic refreshing (every 30s for now) of scores.json. #281

Closed poelstra closed 6 years ago

poelstra commented 6 years ago

Another part of #245: automatically refresh the scores and ranking screens.

Note: this PR only does a periodic refresh (hardcoded to 30s for now). It does not yet refresh on e.g. an MHub message, which can be done separately.

It also doesn't yet remove the Refresh button: I think it may occassionally still be useful to manually press it if needed, but let's discuss that in a separate issue.

Advice to merge this PR after #278, to prevent people being annoyed when a refresh happens when they're editing a score.

Also fixes #115.

rikkertkoppes commented 6 years ago

To reduce the code complexity a bit, I'd suggest to extract the 3 methods to something generic and external to the module. Basically is is a fancy $interval service that keeps track of subscriber count. The only parameters needed by this service is what to do every interval:

                if (self._updating <= 0) {
                    self.load();
                }

And the interval itself. All the rest is internal to the service

poelstra commented 6 years ago

@rikkertkoppes Addressed both of your remarks.

However, the tests for ng-scores now fail. Apparently, because ng-scores can't find the Poller factory, but this only happens in the tests. I've been pulling my hair out for 2 hours on this now, and can't figure out why.

I would really appreciate if you can take a look at why this may be happening, given that you initially set that stuff up, and are more knowledgeable on the angular side anyway...

poelstra commented 6 years ago

BTW: the conflicts are trivial to resolve: they are only marked as conflict because the lines happened to be adjacent. So keeping changes from both sides will do the trick.

poelstra commented 6 years ago

Tests issue should be fixed by #293, will merge master into this (or rebase) after that PR is merged.

coveralls commented 6 years ago

Coverage Status

Coverage increased (+0.2%) to 87.74% when pulling 97e09a7deeb4ab7d52324dd0888762d912227b07 on periodic_scores_refresh into f7f3b46fe01a58a0aa129b49879e09c28f650889 on master.

poelstra commented 6 years ago

Appears that another dependency was missing ('services' should depend on 'factories'), which fixes the tests (even without #293). Strange that it worked in the frontend without that dependency, though...

Merged master in to resolve the merge conflicts (basically accepting the changes from both sides).

coveralls commented 6 years ago

Coverage Status

Coverage increased (+0.2%) to 87.873% when pulling eb4582bd62a0a38a6aefa262f62b7fb947c5ebc1 on periodic_scores_refresh into a243e4ac5bf74625d626a4624d85b36d290f5e76 on master.