IQBall
/
Application-Web
6
0
Fork 0
Code Issues 49 Pull Requests Packages Projects 5 Releases Wiki Activity
Labels Milestones

Persist tactics #23

Merged
maxime.batista merged 6 commits from tactic/persistance into master 1 year ago
Conversation 0 Commits 6 Files Changed 23
maxime.batista commented 1 year ago

This pull request adds persistance to tactics.

you can now edit a tactic, place some players in the court, and refresh the page will bring back the previous court state.

This pull request adds persistance to tactics. you can now edit a tactic, place some players in the court, and refresh the page will bring back the previous court state.
maxime.batista added 3 commits 1 year ago
maxime.batista force-pushed tactic/persistance from 53ea75f48e to c4487c8365 1 year ago
maxime.batista force-pushed tactic/persistance from c4487c8365 to e4d908f2fb 1 year ago
maxime.batista requested review from clement.freville2 1 year ago
maxime.batista requested review from yanis.dahmane-bounoua 1 year ago
maxime.batista requested review from vivien.dufour 1 year ago
maxime.batista requested review from mael.daim 1 year ago
maxime.batista requested review from samuel.berion 1 year ago
clement.freville2 requested changes 1 year ago
front/Fetcher.ts Outdated
export function fetchAPI(
url: string,
payload: object,
clement.freville2 commented 1 year ago 
-    payload: object,
+    payload: unknown,

Anything is json-serializable.

```diff - payload: object, + payload: unknown, ``` Anything is json-serializable.
maxime.batista marked this conversation as resolved
front/views/Editor.tsx Outdated
import { CSSProperties, useRef, useState } from "react"
import React, { CSSProperties, useEffect, useRef, useState } from "react"
clement.freville2 commented 1 year ago 
- import React, { CSSProperties, useEffect, useRef, useState } from "react"
+ import { CSSProperties, useEffect, useRef, useState } from "react"

React should never be imported.

```diff - import React, { CSSProperties, useEffect, useRef, useState } from "react" + import { CSSProperties, useEffect, useRef, useState } from "react" ``` React should **never** be imported.
maxime.batista marked this conversation as resolved
front/views/Editor.tsx Outdated
didMount.current = true
return
}
setSaveState(SaveStates.Saving)
clement.freville2 commented 1 year ago

State setters shouldn't be called in the top level scope of an useEffect hook.
This causes avoidable re-renders because this state will always be updated after a first re-render when a dependency changes.
Then a second re-render will be triggered by this state update.
Instead, change the state directly when the players dependency changes: React will batch the state updates and only trigger a single re-render.

So... Let's join those two states together in a custom hook (alternatively, each setPlayers call should also call setSaveState):

function useSyncedState<S>(
  initialState: S,
  initialSaveState = SaveStates.Ok,
): [
  S,
  Dispatch<SetStateAction<S>>,
  SaveState,
  Dispatch<SetStateAction<SaveState>>,
] {
  const [state, setState] = useState<S>(initialState)
  const [saveState, setSaveState] = useState<SaveState>(initialSaveState)
  const syncState = useCallback((newState: SetStateAction<S>) => {
    setState((current) => {
      const state =
        typeof newState === "function"
          ? (newState as (prevState: S) => S)(current)
          : newState
      if (state !== current) {
        setSaveState(SaveStates.Saving)
      }
      return state
    })
  }, [])
  return [state, syncState, saveState, setSaveState]
}

Usage:

const [players, setPlayers, saveState, setSaveState] = useSyncedState<
  Player[]
>(content.players)

To go the extra mile, you may include the useEffect directly in this custom hook. This reduces the number of items this function returns:

function useSyncedState<S>(
  initialState: S,
  syncCallback: (newState: S) => Promise<boolean>,
  initialSaveState = SaveStates.Ok,
): [S, Dispatch<SetStateAction<S>>, SaveState] {
  // ...
  useEffect(() => {
    if (initialState === state) return
    // ...
  }, [syncCallback, state])
  return [state, syncState, saveState]
}
State setters shouldn't be called in the top level scope of an `useEffect` hook. This causes avoidable re-renders because this state will always be updated after a first re-render when a dependency changes. Then a second re-render will be triggered by this state update. Instead, change the state directly when the `players` dependency changes: React will batch the state updates and only trigger a single re-render. So... Let's join those two states together in a custom hook (alternatively, each `setPlayers` call should also call `setSaveState`): ```ts function useSyncedState<S>( initialState: S, initialSaveState = SaveStates.Ok, ): [ S, Dispatch<SetStateAction<S>>, SaveState, Dispatch<SetStateAction<SaveState>>, ] { const [state, setState] = useState<S>(initialState) const [saveState, setSaveState] = useState<SaveState>(initialSaveState) const syncState = useCallback((newState: SetStateAction<S>) => { setState((current) => { const state = typeof newState === "function" ? (newState as (prevState: S) => S)(current) : newState if (state !== current) { setSaveState(SaveStates.Saving) } return state }) }, []) return [state, syncState, saveState, setSaveState] } ``` Usage: ```ts const [players, setPlayers, saveState, setSaveState] = useSyncedState< Player[] >(content.players) ``` To go the extra mile, you may include the `useEffect` directly in this custom hook. This reduces the number of items this function returns: ```ts function useSyncedState<S>( initialState: S, syncCallback: (newState: S) => Promise<boolean>, initialSaveState = SaveStates.Ok, ): [S, Dispatch<SetStateAction<S>>, SaveState] { // ... useEffect(() => { if (initialState === state) return // ... }, [syncCallback, state]) return [state, syncState, saveState] } ```
maxime.batista marked this conversation as resolved
front/views/Editor.tsx Outdated
(p) =>
p.team === player.team &&
p.role === player.role,
)
clement.freville2 commented 1 year ago

Consider extracting this index search elsewhere (this code is very indented and repeated above).

Consider extracting this index search elsewhere (this code is very indented and repeated above).
maxime.batista marked this conversation as resolved
sql/setup-tables.sql Outdated
id_team integer,
id_user integer,
role char(1) CHECK (role IN ('Coach', 'Player')),
role char(1) CHECK (role IN ('Coach', 'Player')),
clement.freville2 commented 1 year ago 
-     role    char(1) CHECK (role IN ('Coach', 'Player')), 
+     role    TEXT    CHECK (role IN ('Coach', 'Player')),

Since we're here, the CHAR type is not valid if we see those checks. TEXT is the only SQLite type adapted here.

```diff - role char(1) CHECK (role IN ('Coach', 'Player')), + role TEXT CHECK (role IN ('Coach', 'Player')), ``` Since we're here, the `CHAR` type is not valid if we see those checks. `TEXT` is the only [SQLite type](https://www.sqlite.org/datatype3.html) adapted here.
maxime.batista marked this conversation as resolved
src/Api/Controller/APITacticController.php Outdated
*/
public function saveContent(int $id, Account $account): HttpResponse {
return Control::runChecked([
"content" => [],
clement.freville2 commented 1 year ago

No HTTP 413 Content too large?
Checking the vague structure of the JSON object also help API user to catch their mistakes.

No [`HTTP 413 Content too large`](https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/413)? Checking the vague structure of the JSON object also help API user to catch their mistakes.
maxime.batista commented 1 year ago
Poster

checks about tactics in backend will be done later as the structure will evolve too much and we'll need to keep it in sync between front end definition and backend checks

checks about tactics in backend will be done later as the structure will evolve too much and we'll need to keep it in sync between front end definition and backend checks
clement.freville2 commented 1 year ago

While your point is valid, I would argue that since players is nested in content, it misses a free check that it is an array. Anyway, it is perfectly fine as it is for now.

While your point is valid, I would argue that since `players` is nested in `content`, it misses a free check that it is an array. Anyway, it is perfectly fine as it is for now.
maxime.batista marked this conversation as resolved
src/Core/Gateway/TacticInfoGateway.php Outdated
);
}
public function updateContent(int $id, string $json): void {
clement.freville2 commented 1 year ago

This does not check neither if the id is valid nor if the user is the owner of this tactic.

For the first point, you reach the limits of your current Connection class.

public function updateContent(int $id, string $json): bool {
    $req = $this->pdo->prepare(
        "UPDATE Tactic SET content = :content WHERE id = :id",
    );
    $req->execute([
        "content" => $json,
        "id" => $id,
    ]);
    return $req->rowCount() === 1;
}

For the second point, it may happen elsewhere.

This does not check neither if the id is valid nor if the user is the owner of this tactic. For the first point, you reach the limits of your current `Connection` class. ```php public function updateContent(int $id, string $json): bool { $req = $this->pdo->prepare( "UPDATE Tactic SET content = :content WHERE id = :id", ); $req->execute([ "content" => $json, "id" => $id, ]); return $req->rowCount() === 1; } ``` For the second point, it may happen elsewhere.
maxime.batista marked this conversation as resolved
vivien.dufour changed title from Persist tactics to WIP: Persist tactics 1 year ago
vivien.dufour changed title from WIP: Persist tactics to WIP: Persist tactics 1 year ago
vivien.dufour changed title from WIP: Persist tactics to Persist tactics 1 year ago
maxime.batista requested review from clement.freville2 1 year ago
maxime.batista force-pushed tactic/persistance from ee977a0e7b to 84965a0fad 1 year ago
vivien.dufour approved these changes 1 year ago
maxime.batista force-pushed tactic/persistance from 84965a0fad to cb24dd53a9 1 year ago
clement.freville2 approved these changes 1 year ago
clement.freville2 left a comment

Some nitpicks, it is really qualitative otherwise!

Some nitpicks, it is really qualitative otherwise!
front/components/editor/CourtPlayer.tsx Outdated
const x = player.rightRatio
const y = player.bottomRatio
const [x, setX] = useState(player.rightRatio)
const [y, setY] = useState(player.bottomRatio)
clement.freville2 commented 1 year ago 
-     const [x, setX] = useState(player.rightRatio)
-     const [y, setY] = useState(player.bottomRatio)
+     const x = player.rightRatio
+     const y = player.bottomRatio

Keep one source of truth, it is either the state or the props.

```diff - const [x, setX] = useState(player.rightRatio) - const [y, setY] = useState(player.bottomRatio) + const x = player.rightRatio + const y = player.bottomRatio ``` Keep one source of truth, it is either the state or the props.
maxime.batista marked this conversation as resolved
front/components/editor/CourtPlayer.tsx
const { x, y } = calculateRatio(pieceBounds, parentBounds)
setX(x)
setY(y)
clement.freville2 commented 1 year ago 
-                 setX(x)
-                 setY(y)
```diff - setX(x) - setY(y) ```
maxime.batista marked this conversation as resolved
front/views/Editor.tsx Outdated
default_value={name}
on_validated={(new_name) => {
onNameChange(new_name).then((success) => {
success ? setStyle({}) : setStyle(ERROR_STYLE)
clement.freville2 commented 1 year ago 
-                                 success ? setStyle({}) : setStyle(ERROR_STYLE) 
+                                 setStyle(success ? {} : ERROR_STYLE)
```diff - success ? setStyle({}) : setStyle(ERROR_STYLE) + setStyle(success ? {} : ERROR_STYLE) ```
maxime.batista marked this conversation as resolved
front/views/Editor.tsx Outdated
}
return state
})
}, [])
clement.freville2 commented 1 year ago 
-     }, []) 
+     }, [saveStateCallback])

Just for exhaustiveness, does not change anything here since this hook is always called without any local capture and does not change.

```diff - }, []) + }, [saveStateCallback]) ``` Just for exhaustiveness, does not change anything here since this hook is always called without any local capture and does not change.
maxime.batista marked this conversation as resolved
src/Api/Controller/APITacticController.php Outdated
"content" => [],
], function (HttpRequest $req) use ($id) {
if ($fail = $this->model->updateContent($id, json_encode($req["content"]))) {
return new JsonHttpResponse([$fail]);
clement.freville2 commented 1 year ago 
-                 return new JsonHttpResponse([$fail]);
+                 return new JsonHttpResponse([$fail], HttpCodes::BAD_REQUEST);
```diff - return new JsonHttpResponse([$fail]); + return new JsonHttpResponse([$fail], HttpCodes::BAD_REQUEST); ```
maxime.batista marked this conversation as resolved
maxime.batista added 1 commit 1 year ago
continuous-integration/drone/push Build is passing Details
6ebf3737af
apply suggestions
maxime.batista merged commit 3de131d112 into master 1 year ago
maxime.batista deleted branch tactic/persistance 1 year ago

Reviewers

yanis.dahmane-bounoua was requested for review 1 year ago
mael.daim was requested for review 1 year ago
samuel.berion was requested for review 1 year ago
vivien.dufour approved these changes 1 year ago
clement.freville2 approved these changes 1 year ago
continuous-integration/drone/push Build is passing
Details
The pull request has been merged as 3de131d112.
Sign in to join this conversation.
Reviewers
Request review
No reviewers
yanis.dahmane-bounoua
mael.daim
samuel.berion
vivien.dufour
clement.freville2
Labels
Apply labels
Clear labels
bug
Something is not working duplicate
This issue or pull request already exists enhancement
New feature help wanted
Need some help invalid
Something is wrong question
More information is needed wontfix
This won't be fixed
No Label bug duplicate enhancement help wanted invalid question wontfix
Milestone
Set milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Assign users
Clear assignees
No Assignees
3 Participants
Notifications
Due Date

No due date set.

Dependencies

No dependencies set.

Reference: IQBall/Application-Web#23
Write Preview
Loading…
Cancel
Save
There is no content yet.
Delete Branch 'tactic/persistance'

Deleting a branch is permanent. It CANNOT be undone. Continue?

No
Yes