add unified queue for match running#52
Conversation
amcsz
force-pushed
the
bigchanges
branch
2 times, most recently
from
48c5f98 to
f45653f
Compare
amcsz
force-pushed
the
bigchanges
branch
6 times, most recently
from
b265330 to
158ef1f
Compare
amcsz
force-pushed
the
bigchanges
branch
3 times, most recently
from
5637179 to
0cd4092
Compare
Add Docker Compose integration (tjcsl#51) [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci
[pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci
[pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci
[pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci
[pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci
[pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci
[pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci
[pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci
[pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci fix: polish queue table [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci
JasonGrace2282
left a comment
JasonGrace2282
left a comment
There was a problem hiding this comment.
I like the changes in this PR!
I've gone through and given feedback on how we can improve the code and get this merged - please take a look.
I do wish this PR had been split apart into multiple PRs based on the change. A nice split would have been:
- Fixing dev environment (easy to merge)
- Deleting unnecessary static files (easy to merge)
- Unifying the game queue
- Actual Elo implementation
This would have made the PR significantly easier to review - please never make a single PR this big ever again!
(See How to Make Your Code Reviewer Fall in Love with You)
That being said, it's far too late to split up the PR at this point in time, so don't try to do it with this PR :)
| uv run manage.py shell -c " | ||
| from django.contrib.auth import get_user_model | ||
| User = get_user_model() | ||
| if not User.objects.filter(username='admin').exists(): | ||
| User.objects.create_superuser( | ||
| username='admin', | ||
| email='admin@admin.com', | ||
| password='123' | ||
| ) | ||
| " |
There was a problem hiding this comment.
If you're going to do this, you should probably admin.set_password("tjcsl"), document it, and add a password login when settings.DEBUG is True
There was a problem hiding this comment.
For the admin.set_password("tjcsl") part, wouldn't it be just the same as calling create_superuser? Also, why would you need to only add it on settings.DEBUG = True when it's always going to be true since its a testing environment?
There was a problem hiding this comment.
Please squash all the migrations into one big migration file
| <div id="player-score-box" class="centered col-7"> | ||
| <p id="player-black" class="player-score"></p> | ||
| <p style="visibility: hidden">WWW</p> | ||
| <p id="player-white" class="player-score"></p> |
There was a problem hiding this comment.
The css seems a bit strange here:
Maybe it would be better to have a half/half split for names, and use some other indicator for the turn?
| # Celery | ||
| CELERY_RESULT_BACKEND = "django-db" | ||
| CELERY_BROKER_URL = "redis://localhost:6379/1" | ||
| CELERY_BROKER_URL = "redis://othello_redis:6379/1" |
There was a problem hiding this comment.
Didn't we change all of this stuff in the secret.sample.docker.py? I would rather not modify it in this PR, and delete it from the main settings file in a follow-up PR
| "created_at": formats.date_format( | ||
| timezone.localtime(match.created_at), | ||
| "DATETIME_FORMAT", | ||
| ), |
There was a problem hiding this comment.
The server should probably return it in UTC/isoformat and it should be converted to localtime client-side.
| $("#stepForward").click(function (){ | ||
| gameIndex = Math.min(++gameIndex, gameReplay.game.length-1); | ||
| drawBoardAtState(gameIndex); |
There was a problem hiding this comment.
I feel like we talked about this (and you possibly even showed it working), but it is intuitive to try arrow keys instead of clicking on buttons at the bottom of the screen. Can we do that in addition to the buttons?
| <a class="nav-link" href="{% url 'games:help' %}">Help</a> | ||
| </li> | ||
| <li class="nav-item"> | ||
| <a class="nav-link" href="{% url 'games:queue' %}">Queue</a> |
There was a problem hiding this comment.
Queue seems a bit of a weird name - maybe something like "Matches" is better?
| <li class="nav-item d-flex align-items-center"> | ||
| <form method="post" action="{% url 'auth:logout' %}" style="display: inline;"> | ||
| {% csrf_token %} | ||
| <button type="submit" class="nav-link nav-button"> |
There was a problem hiding this comment.
can we make this like slightly red or some other indicator (other than the font size difference?)
for more information, see https://pre-commit.ci
3 participants
still a wip