Release beta v2 0.3 #802

adamlerer · 2022-07-01T15:51:21Z

Release v0.3 of the new beta UI.

IMPORTANT notes:

Unlike previous beta versions, this one makes db modifications, meaning we will have to upgrade to 1.70. master is still on 1.68 though, so what do we do? We can either upgrade to 1.70 in prod, or switch the 1.69 and 1.70 changes and switch to 1.69 ...
Should this be merged into master or staging?

Feature list:

turn off "Early Access" guard for Beta UI
allow games to specify a shorter phase length for R/B phases and grace periods, vs M phases
Fix handling of civil disorders, pre-game phase, and vote processing in beta UI
Add blinking for unsaved unit orders
Add support for beta UI within the webdiplomacy docker configuration
fix longstanding bug in old UI in rulebook press, where moderator would broadcast a private message to everyone if they sent a message in a R/B phase

@lightvector

@lightvector comments

Add phase names between messages

Don't broadcast a private message to everyone just because you are mod

report variantID for active games

* html sanitize fix * fix lint errors Co-authored-by: utiq <cesargutierrez@fb.com>

Don't give an exception if vote response is empty string (ie no votes)

WDIP-32: Turn off early access guard for beta

WDIP-21: Fix handling of Civil Disorder in beta UI

* docker-compose configuration * put back temp change * put back proxy in package.json * port back to 8895 * Revert "port back to 8895" This reverts commit a793f6a. * Revert "put back proxy in package.json" This reverts commit 091637c. * add scripts to run development env out of the box * fix entrypoint * fix entrypoint * fix permissions * add config copy * update variable * update default values Co-authored-by: utiq <cesargutierrez@fb.com>

WDIP-17: Different phase duration for retreats/builds/missed turns

…macy" This reverts commit 327f288, reversing changes made to f43b00e.

Db version

Revert process votes

Fix memcached issue

Show the map during pre-game

revert some stuff

…-beta-from-game-search-results WDIP-39: Remove "Play Beta" from game search results

Process votes

adamlerer · 2022-07-01T17:23:56Z

@kestasjk I tried resolving the merge conflicts here, but at least on my private instance, I found that the current master branch isn't working properly (gamemaster doesn't start bot games, cancel doesn't work, etc.) so I was wondering if your changes from a few hours ago might have broken something.

Are changes on master auto-synced to webdiplomacy.net ? Because things are working fine there.

kestasjk · 2022-07-03T02:53:27Z

Hi Adam,
Hmm that's odd..

I changed how votes are handled since it wasn't working reliably; PHP logic detected when a vote passed, and the logic was at a point where I didn't understand the intent. Also because it was in PHP you couldn't search for votes that need processing, you had to detect it when the vote is submitted or game is viewed by a player and then set a flag in memcache for the gamemaster to handle.
I replaced it with a query to search for votes to process that ignores votes from bots entirely. This is a change to the previous logic which did factor in votes from bots, but I couldn't make sense of what it was trying to do where it would sort of allow votes depending on whether the bot had gained or lots supply centers in the last 2-4 turns, so I figured (since it wasn't working for many anyway) I would simplify it right down and see if any issues arose.

Possibly unrelated but FYI: We were also still getting deadlocks / lock timeouts appear in the error logs that I wanted to resolve. When looking into the API I realized it was locking games for update, the most strict and intrusive lock type, to do read-only operations including just checking whether a user was in a game. With hindsight I'm impressed (with the hardware) that the site never became totally inoperable.
Part of the reason is knowing the new board does do a lot of background server requests, and I want to add an instant-play feature so anyone can go to a URL, no account needed, start a bot game and leave when they like, which might increase server load.

Yep webdiplomacy.net syncs with the master branch automatically, and it is in sync at the moment. Is it 1 human games that aren't processing or multi-human games, and are full human games processing? I didn't change how the gamemaster does game processing and it is working so I'm not sure what's going on there. I can see you've got some changes talking about database changes / vote processing etc so maybe there's a merge conflict of some sort. I want to try and finish up a couple of features today but I might be able to look at this merge. I do also want to get staging up to pace with the master branch so you can merge with staging again.

I'll try and bring the database/code version up to 1.71 today, and do a test of a clean install in docker, so you can merge in the database changes neatly into 1.72 via a install/1.71-1.72/update.sql script.

adamlerer · 2022-07-03T03:18:47Z

Thanks @kestasjk . Yeah, the vote logic was hard for me to understand as well, I sort of treated it as a black box. Once you update the code I'll probably start from scratch on how to initiate vote processing from the setVote API call (let me know if you have a suggestion).

Re locking: I didn't write this code, not surprised that this happened because the getAssociatedGame function was originally written for a limited purpose and got cargo-culted around for various purposes. Lucky it didn't cause any problems.

What's not working on my end is the processing (starting) of all-bot games. I haven't experimented with anything else.

I'll take another pass once you make your changes.

TimothyJones

Hi there! I partially reviewed the react code. Nothing show stopping, but I left a number of optional comments about style improvements and risk reduction. If this is useful, I'm happy to review the rest of it.

TimothyJones · 2022-07-03T12:03:52Z

beta-src/src/components/controllers/WDMainController.tsx

@@ -101,6 +103,14 @@ const WDMainController: React.FC = function ({ children }): React.ReactElement {
            // *not* yet seen (i.e. presumably they saw this phase when they were entering
            // orders for it, before that phase ended and other powers' orders appeared).
            dispatch(gameApiSliceActions.setViewedPhaseToLatestPhaseViewed());
+            if (status.status === "Left") {


Unrelated to this change, but I think it's better to declare the function outside the JSX inside a useCallback, otherwise there may be rendering overhead. In the current form, a new clickhandler function is created every time this component renders.

True, although I believe this component renders rarely (only on phase updae).

TimothyJones · 2022-07-03T12:09:54Z

beta-src/src/components/controllers/WDMainController.tsx

-    noPhase || (displayedPhaseKey && overviewKey !== displayedPhaseKey);
+    noPhase ||
+    status.status === "Left" ||
+    (displayedPhaseKey && overviewKey !== displayedPhaseKey);
  if (displayedPhaseKey === null && overview.phase) {
    setDisplayedPhaseKey(overviewKey);


I might be missing something with this Controller pattern, but I believe this call (and the other dispatch calls) have side effects, which means they would properly be placed inside a useEffect or an event handler. I don't think it's right to call them directly in the render function.

Personally, I would put this whole check inside a selector - if you can compute it from the main state, you don't need to store it in separate state. Storing things you can compute is risky because you might forget to update them, or you might get out of sync with the main state in other ways.

@lightvector wrote this code, but I believe the reason that this is stateful is that on a phase change, we want to remember the last phase that was being viewed, and let the user manually scroll forward to the current phase. Therefore, it's not a stateless function of the current state.

TimothyJones · 2022-07-03T12:13:28Z

beta-src/src/components/controllers/WDUnitController.tsx

  unitWidth,
  unitHeight,
 }): React.ReactElement {
  const iconWidth =
-    type === "Fleet" ? FLEET_RAW_ICON_WIDTH : ARMY_RAW_ICON_WIDTH;
+    unit.unit.type === "Fleet" ? FLEET_RAW_ICON_WIDTH : ARMY_RAW_ICON_WIDTH;


Is there a way to write this without needing to access it with unit.unit? I feel like there are two different meanings of unit here, and it's not obvious what the difference is during a skim read.

As an aside, knowing how to calculate the width and height doesn't feel like the responsibility of a controller - it might read better as a function that returns { iconWidth, iconHeight }

TimothyJones · 2022-07-03T12:14:26Z

beta-src/src/components/controllers/WDUnitController.tsx

-      {type === "Fleet" && (
-        <WDFleetIcon iconState={iconState} country={meta.country} />
+      {unit.unit.type === "Fleet" && (
+        <WDFleetIcon iconState={unitState} country={unit.country} />


I like this unitState rename. Good catch.

TimothyJones · 2022-07-03T12:17:09Z

beta-src/src/components/map/components/WDArrowContainer.tsx

@@ -14,6 +14,7 @@ import ArrowColor from "../../../enums/ArrowColor";
 import drawArrowFunctional, {
  getTargetXYWH,
  getArrowX1Y1X2Y2,
+  makeSVGDrawAsUnsavedAnimateElement,


This name doesn't feel like it's as clear as it could be. Can it be more about what it is for, not how it does it?

TimothyJones · 2022-07-03T12:43:12Z

beta-src/src/components/ui/WDPhaseUI.tsx

@@ -14,13 +14,22 @@ import {
 import WDCountdownPill from "./WDCountdownPill";
 import WDPillScroller from "./WDPillScroller";

+const getCurPhaseMinutes = function (phaseMinutes, phaseMinutesRB, phase) {


Same comment as earlier about get - I think these are selectors or renderers, and would be better without the get prefix.

TimothyJones · 2022-07-03T12:44:29Z

beta-src/src/components/ui/WDPress.tsx

              multiline
              maxRows={4}
-              onChange={(text) => setUserMsg(text.target.value)}
+              onChange={(text) => {


Same comment about not declaring these inline

TimothyJones · 2022-07-03T12:45:57Z

beta-src/src/state/game/game-api-slice.ts

@@ -128,6 +128,7 @@ export const setVoteStatus = createAsyncThunk(
    vote: string;
    voteOn: string;
  }) => {
+    console.log("Submitting vote");


I don't think this is meant to be in the PR (and probably should be a lint error)

TimothyJones · 2022-07-03T12:47:47Z

beta-src/src/state/game/game-api-slice.ts

@@ -143,14 +144,25 @@ export const markMessagesSeen = createAsyncThunk(
    gameID: string;
    seenCountryID: string;
  }) => {
-    const { data } = await getGameApiRequest(
+    const { data } = await postGameApiRequest(
      ApiRoute.MESSAGES_SEEN,
      queryParams,
    );
    return data as string;


This as would be better put inside the *GameApiRequest functions, I think.

Also, since it's just a string that we're looking for, we could check that it's actually a string and throw an error if it isn't, too.

I don't know why this as string is here - the linter isn't complaining even when I remove it.

TimothyJones · 2022-07-03T12:51:42Z

beta-src/src/state/game/game-api-slice.ts

      ApiRoute.MESSAGES_SEEN,
      queryParams,
    );
    return data as string;
  },
 );

+export const setBackFromLeft = createAsyncThunk(


I personally would not name these set as they can be confused with state setters. For consistency with the other function you could call it markBackFromLeft.

I like update for a prefix for server updates, and fetch for server gets.

In this case I would name the function markReturnedToGame

Also, probably it should be a put not a post, because (unless I'm missing something) this particular request is idempotent.

kestasjk · 2022-07-03T12:56:11Z

Righto didn't get the anonymous game suspicion stuff done, but I've wrapped everything up and started a new release 1.70, and I've tested out a fresh install from the docker composer to check that it's all working. Next dev cycle probably won't be for a few weeks so that should let you make your changes without any moving parts.

adamlerer · 2022-07-05T17:54:00Z

Okay, I've merged in the latest master, things seem to be working correctly now.

@kestasjk there are still a couple things that aren't working quite properly for me right now:

The UI behaves weirdly when you cancel a game, in either the old or new UI. To repro: create a bot game, then press cancel, in either UI. (The cancellation works, but the UI misbehaves because the game has disappeared).
When I run docker-compose up, memcached doesn't work. The reason is that the memcached server is exposed as memcached:11211, while the PHP code looks for 127.0.0.1:11211. Solutions I've found are either to change the PHP code to $MC->addServer("memcached", 11211); or docker exec into the PHP machine and set up memcached there.

@TimothyJones I addressed some of your comments, but some of them would be better suited to a follow-up PR. It would certainly be great if you had time to improve the beta code systematically, there's definitely a lot of things that could be cleaned up and made more idiomatic. But I don't think fixing them locally in just a single place is worth it, and I don't want to do too much more in this PR since it's already quite large.

…age-load WDIP-42: Open popover on page load

adamlerer and others added 30 commits June 14, 2022 10:38

WIP

2ab2711

Don't broadcast a private message to everyone just because you are mod

8becdbd

report variantID for active games

03d894b

Add phase names between messages

59893cc

@lightvector comments

Merge pull request #106 from adamlerer/chat-phase-names

e4350c0

Add phase names between messages

Merge pull request #107 from adamlerer/moderator-message-bug

f2d4407

Don't broadcast a private message to everyone just because you are mod

Merge pull request #109 from adamlerer/active-games-variant

db58b8f

report variantID for active games

WDIP-23: html sanitize fix (#112)

910050e

* html sanitize fix * fix lint errors Co-authored-by: utiq <cesargutierrez@fb.com>

Unsaved flicker

62318df

Sharper blink

f5d0bb2

finish separate phase length for R/B

ad595ea

Reduce options

23134b3

Turn off early access guard for beta

8becfdf

Don't give an exception if vote response is empty string (ie no votes)

5eab340

WIP

04bb771

Merge pull request #118 from adamlerer/vote-no

9b74e1a

Don't give an exception if vote response is empty string (ie no votes)

Fix handling of Civil Disorder in beta UI

0de8317

blink

e79b599

Merge pull request #117 from adamlerer/beta-early-access

18e3725

WDIP-32: Turn off early access guard for beta

remove logs

addb73a

Merge pull request #119 from adamlerer/civil-disorder

f43b00e

WDIP-21: Fix handling of Civil Disorder in beta UI

Merge branch 'process-votes' of github.com:adamlerer/webDiplomacy

327f288

Merge branch 'main' of github.com:adamlerer/webDiplomacy

998120a

tailwind configuration

c01f941

improve game create page

a250088

Merge pull request #108 from adamlerer/phase-minutes-rb

4366c84

WDIP-17: Different phase duration for retreats/builds/missed turns

Revert "Merge branch 'process-votes' of github.com:adamlerer/webDiplo…

f744c2d

…macy" This reverts commit 327f288, reversing changes made to f43b00e.

keep the good part

634e839

solve merge conflicts WDIP-22 into main

55609ae

adamlerer and others added 17 commits June 29, 2022 10:20

Update global db version for RB phase times

f8f1d1b

Merge pull request #128 from adamlerer/db-version

d1e48b2

Db version

Merge pull request #124 from adamlerer/revert-process-votes

49b731f

Revert process votes

fix memcached issue

1d035ef

Merge pull request #129 from adamlerer/fix-memchached-issue

49cb33e

Fix memcached issue

update fullInstall.sql

1d896e4

Show the map during pre-game

bff159e

remove log

2615137

revert some stuff

204a07b

remove join beta button

89907d5

Merge pull request #130 from adamlerer/pre-game-map

6dd7ce8

Show the map during pre-game

Merge pull request #132 from adamlerer/misc-backend2

7d80369

revert some stuff

WIP

056c329

fix cancelling games in old UI

ec0beed

Merge pull request #133 from adamlerer/chore/utiq/wdip-39-remove-play…

9f23456

…-beta-from-game-search-results WDIP-39: Remove "Play Beta" from game search results

make it work

4bcf0ec

Merge pull request #136 from adamlerer/process-votes

357a0e0

Process votes

TimothyJones reviewed Jul 3, 2022

View reviewed changes

TimothyJones mentioned this pull request Jul 3, 2022

Fix format of phase finished message #805

Closed

adamlerer added 2 commits July 5, 2022 10:22

Merge remote-tracking branch 'kestasjk/master'

b52fd79

Suggestions from @TimothyJones

e30b356

utiq and others added 2 commits July 7, 2022 18:43

Open popover on page load

0496e05

Merge pull request #139 from adamlerer/utiq/wdip-42-open-popover-on-p…

b6b09b2

…age-load WDIP-42: Open popover on page load

kestasjk merged commit 733311c into kestasjk:master Jul 8, 2022

adamlerer mentioned this pull request Jul 8, 2022

Build /beta #809

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Release beta v2 0.3 #802

Release beta v2 0.3 #802

adamlerer commented Jul 1, 2022 •

edited

Loading

adamlerer commented Jul 1, 2022

kestasjk commented Jul 3, 2022

adamlerer commented Jul 3, 2022 •

edited

Loading

TimothyJones left a comment

TimothyJones Jul 3, 2022

adamlerer Jul 5, 2022

TimothyJones Jul 3, 2022

adamlerer Jul 5, 2022

TimothyJones Jul 3, 2022

TimothyJones Jul 3, 2022

TimothyJones Jul 3, 2022

TimothyJones Jul 3, 2022

TimothyJones Jul 3, 2022

TimothyJones Jul 3, 2022

TimothyJones Jul 3, 2022

adamlerer Jul 5, 2022

TimothyJones Jul 3, 2022

kestasjk commented Jul 3, 2022

adamlerer commented Jul 5, 2022 •

edited

Loading

Release beta v2 0.3 #802

Release beta v2 0.3 #802

Conversation

adamlerer commented Jul 1, 2022 • edited Loading

adamlerer commented Jul 1, 2022

kestasjk commented Jul 3, 2022

adamlerer commented Jul 3, 2022 • edited Loading

TimothyJones 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

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

kestasjk commented Jul 3, 2022

adamlerer commented Jul 5, 2022 • edited Loading

adamlerer commented Jul 1, 2022 •

edited

Loading

adamlerer commented Jul 3, 2022 •

edited

Loading

adamlerer commented Jul 5, 2022 •

edited

Loading