From 9114c5ca1bff8ebbe3d2d20d873f8f5b7a78be43 Mon Sep 17 00:00:00 2001 From: tobi <31960611+tsmethurst@users.noreply.github.com> Date: Sat, 21 Oct 2023 17:23:05 +0200 Subject: [PATCH] [bugfix/frontend] fix typo and other oddness in patchRemoteEmojis (#2281) * fix emoji test model * found the bug! * remove unused 'current' import * comment useChecklistReducer * wah * lint * fix cleaner tests --- internal/cleaner/emoji_test.go | 29 +++- testrig/testmodels.go | 4 +- .../admin/emoji/remote/parse-from-toot.js | 11 +- web/source/settings/lib/form/check-list.tsx | 124 ++------------ web/source/settings/lib/form/types.ts | 2 +- .../lib/query/admin/custom-emoji/index.ts | 56 +++++-- web/source/settings/redux/checklist.ts | 158 ++++++++++++++++++ 7 files changed, 247 insertions(+), 137 deletions(-) create mode 100644 web/source/settings/redux/checklist.ts diff --git a/internal/cleaner/emoji_test.go b/internal/cleaner/emoji_test.go index 81fde6e48..30642a818 100644 --- a/internal/cleaner/emoji_test.go +++ b/internal/cleaner/emoji_test.go @@ -9,8 +9,21 @@ import ( "github.com/superseriousbusiness/gotosocial/internal/db" "github.com/superseriousbusiness/gotosocial/internal/gtscontext" "github.com/superseriousbusiness/gotosocial/internal/gtsmodel" + "github.com/superseriousbusiness/gotosocial/internal/util" ) +func copyMap(in map[string]*gtsmodel.Emoji) map[string]*gtsmodel.Emoji { + out := make(map[string]*gtsmodel.Emoji, len(in)) + + for k, v1 := range in { + v2 := new(gtsmodel.Emoji) + *v2 = *v1 + out[k] = v2 + } + + return out +} + func (suite *CleanerTestSuite) TestEmojiUncacheRemote() { suite.testEmojiUncacheRemote( context.Background(), @@ -54,16 +67,28 @@ func (suite *CleanerTestSuite) TestEmojiPruneUnusedDryRun() { } func (suite *CleanerTestSuite) TestEmojiFixCacheStates() { + // Copy testrig emojis + mark + // rainbow emoji as uncached + // so there's something to fix. + emojis := copyMap(suite.emojis) + emojis["rainbow"].Cached = util.Ptr(false) + suite.testEmojiFixCacheStates( context.Background(), - mapvals(suite.emojis), + mapvals(emojis), ) } func (suite *CleanerTestSuite) TestEmojiFixCacheStatesDryRun() { + // Copy testrig emojis + mark + // rainbow emoji as uncached + // so there's something to fix. + emojis := copyMap(suite.emojis) + emojis["rainbow"].Cached = util.Ptr(false) + suite.testEmojiFixCacheStates( gtscontext.SetDryRun(context.Background()), - mapvals(suite.emojis), + mapvals(emojis), ) } diff --git a/testrig/testmodels.go b/testrig/testmodels.go index fa6ff92ff..5279ec725 100644 --- a/testrig/testmodels.go +++ b/testrig/testmodels.go @@ -1129,7 +1129,7 @@ func NewTestEmojis() map[string]*gtsmodel.Emoji { ImageRemoteURL: "", ImageStaticRemoteURL: "", ImageURL: "http://localhost:8080/fileserver/01AY6P665V14JJR0AFVRT7311Y/emoji/original/01F8MH9H8E4VG3KDYJR9EGPXCQ.png", - ImagePath: "/01AY6P665V14JJR0AFVRT7311Y/emoji/original/01F8MH9H8E4VG3KDYJR9EGPXCQ.png", + ImagePath: "01AY6P665V14JJR0AFVRT7311Y/emoji/original/01F8MH9H8E4VG3KDYJR9EGPXCQ.png", ImageStaticURL: "http://localhost:8080/fileserver/01AY6P665V14JJR0AFVRT7311Y/emoji/static/01F8MH9H8E4VG3KDYJR9EGPXCQ.png", ImageStaticPath: "01AY6P665V14JJR0AFVRT7311Y/emoji/static/01F8MH9H8E4VG3KDYJR9EGPXCQ.png", ImageContentType: "image/png", @@ -1141,7 +1141,7 @@ func NewTestEmojis() map[string]*gtsmodel.Emoji { URI: "http://localhost:8080/emoji/01F8MH9H8E4VG3KDYJR9EGPXCQ", VisibleInPicker: util.Ptr(true), CategoryID: "01GGQ8V4993XK67B2JB396YFB7", - Cached: util.Ptr(false), + Cached: util.Ptr(true), }, "yell": { ID: "01GD5KP5CQEE1R3X43Y1EHS2CW", diff --git a/web/source/settings/admin/emoji/remote/parse-from-toot.js b/web/source/settings/admin/emoji/remote/parse-from-toot.js index 503a341c8..7c29cccfd 100644 --- a/web/source/settings/admin/emoji/remote/parse-from-toot.js +++ b/web/source/settings/admin/emoji/remote/parse-from-toot.js @@ -127,11 +127,12 @@ function CopyEmojiForm({ localEmojiCodes, type, emojiList }) { { changedOnly: false, onFinish: ({ data }) => { - if (data != undefined) { - form.selectedEmoji.updateMultiple( - // uncheck all successfully processed emoji - data.map(([id]) => [id, { checked: false }]) - ); + if (data) { + // uncheck all successfully processed emoji + const processed = data.map((emoji) => { + return [emoji.id, { checked: false }]; + }); + form.selectedEmoji.updateMultiple(processed); } } } diff --git a/web/source/settings/lib/form/check-list.tsx b/web/source/settings/lib/form/check-list.tsx index c08e5022f..e44daefff 100644 --- a/web/source/settings/lib/form/check-list.tsx +++ b/web/source/settings/lib/form/check-list.tsx @@ -18,15 +18,12 @@ */ import { - useReducer, useRef, useEffect, useCallback, useMemo, } from "react"; -import { PayloadAction, createSlice } from "@reduxjs/toolkit"; - import type { Checkable, ChecklistInputHook, @@ -34,106 +31,12 @@ import type { HookOpts, } from "./types"; -// https://immerjs.github.io/immer/installation#pick-your-immer-version -import { enableMapSet } from "immer"; -enableMapSet(); - -interface ChecklistState { - entries: { [k: string]: Checkable }, - selectedEntries: Set, -} - -const initialState: ChecklistState = { - entries: {}, - selectedEntries: new Set(), -}; - -const { reducer, actions } = createSlice({ - name: "checklist", - initialState, // not handled by slice itself - reducers: { - updateAll: (state, { payload: checked }: PayloadAction) => { - const selectedEntries = new Set(); - const entries = Object.fromEntries( - Object.values(state.entries).map((entry) => { - if (checked) { - // Cheekily add this to selected - // entries while we're here. - selectedEntries.add(entry.key); - } - - return [entry.key, { ...entry, checked } ]; - }) - ); - - return { entries, selectedEntries }; - }, - update: (state, { payload: { key, value } }: PayloadAction<{key: string, value: Checkable}>) => { - if (value.checked !== undefined) { - if (value.checked === true) { - state.selectedEntries.add(key); - } else { - state.selectedEntries.delete(key); - } - } - - state.entries[key] = { - ...state.entries[key], - ...value - }; - }, - updateMultiple: (state, { payload }: PayloadAction>) => { - payload.forEach(([key, value]) => { - if (value.checked !== undefined) { - if (value.checked === true) { - state.selectedEntries.add(key); - } else { - state.selectedEntries.delete(key); - } - } - - state.entries[key] = { - ...state.entries[key], - ...value - }; - }); - } - } -}); - -function initialHookState({ - entries, - uniqueKey, - initialValue, -}: { - entries: Checkable[], - uniqueKey: string, - initialValue: boolean, -}): ChecklistState { - const selectedEntries = new Set(); - const mappedEntries = Object.fromEntries( - entries.map((entry) => { - const key = entry[uniqueKey]; - const checked = entry.checked ?? initialValue; - - if (checked) { - selectedEntries.add(key); - } else { - selectedEntries.delete(key); - } - - return [ key, { ...entry, key, checked } ]; - }) - ); - - return { - entries: mappedEntries, - selectedEntries - }; -} +import { + useChecklistReducer, + actions, +} from "../../redux/checklist"; const _default: { [k: string]: Checkable } = {}; - export default function useCheckListInput( /* eslint-disable no-unused-vars */ { name, Name }: CreateHookNames, @@ -143,12 +46,7 @@ export default function useCheckListInput( initialValue = false, }: HookOpts ): ChecklistInputHook { - const [state, dispatch] = useReducer( - reducer, - initialState, - (_) => initialHookState({ entries, uniqueKey, initialValue }) // initial state - ); - + const [state, dispatch] = useChecklistReducer(entries, uniqueKey, initialValue); const toggleAllRef = useRef(null); useEffect(() => { @@ -167,17 +65,17 @@ export default function useCheckListInput( const reset = useCallback( () => dispatch(actions.updateAll(initialValue)), - [initialValue] + [initialValue, dispatch] ); const onChange = useCallback( - (key, value) => dispatch(actions.update({ key, value })), - [] + (key: string, value: Checkable) => dispatch(actions.update({ key, value })), + [dispatch] ); const updateMultiple = useCallback( - (entries) => dispatch(actions.updateMultiple(entries)), - [] + (entries: [key: string, value: Partial][]) => dispatch(actions.updateMultiple(entries)), + [dispatch] ); return useMemo(() => { @@ -215,5 +113,5 @@ export default function useCheckListInput( onChange: toggleAll } }); - }, [state, reset, name, onChange, updateMultiple]); + }, [state, reset, name, onChange, updateMultiple, dispatch]); } diff --git a/web/source/settings/lib/form/types.ts b/web/source/settings/lib/form/types.ts index 45db9e0b8..f9fd2cbdd 100644 --- a/web/source/settings/lib/form/types.ts +++ b/web/source/settings/lib/form/types.ts @@ -152,7 +152,7 @@ interface _withSomeSelected { } interface _withUpdateMultiple { - updateMultiple: (_entries: any) => void; + updateMultiple: (entries: [key: string, value: Partial][]) => void; } export interface TextFormInputHook extends FormInputHook, diff --git a/web/source/settings/lib/query/admin/custom-emoji/index.ts b/web/source/settings/lib/query/admin/custom-emoji/index.ts index 204fb1c1f..56684f03b 100644 --- a/web/source/settings/lib/query/admin/custom-emoji/index.ts +++ b/web/source/settings/lib/query/admin/custom-emoji/index.ts @@ -199,13 +199,16 @@ const extended = gtsApi.injectEndpoints({ }); if (errors.length !== 0) { + const errData = errors.map(e => JSON.stringify(e.data)).join(","); return { error: { status: 400, statusText: 'Bad Request', - data: {"error":`One or more errors fetching custom emojis: ${errors}`}, + data: { + error: `One or more errors fetching custom emojis: [${errData}]` + }, }, - }; + }; } // Return our ID'd @@ -222,14 +225,18 @@ const extended = gtsApi.injectEndpoints({ patchRemoteEmojis: build.mutation({ async queryFn({ action, ...formData }, _api, _extraOpts, fetchWithBQ) { - const data: CustomEmoji[] = []; const errors: FetchBaseQueryError[] = []; - - formData.selectEmoji.forEach(async(emoji: CustomEmoji) => { - let body = { + const selectedEmoji: CustomEmoji[] = formData.selectedEmoji; + + // Map function to get a promise + // of an emoji (or null). + const copyEmoji = async(emoji: CustomEmoji) => { + let body: { + type: string; + shortcode?: string; + category?: string; + } = { type: action, - shortcode: "", - category: "", }; if (action == "copy") { @@ -243,22 +250,43 @@ const extended = gtsApi.injectEndpoints({ method: "PATCH", url: `/api/v1/admin/custom_emojis/${emoji.id}`, asForm: true, - body: body + body: body, }); + if (emojiRes.error) { errors.push(emojiRes.error); - } else { - // Got it! - data.push(emojiRes.data as CustomEmoji); + return null; } - }); + + // Instead of mapping to the emoji we just got in emojiRes.data, + // we map here to the existing emoji. The reason for this is that + // if we return the new emoji, it has a new ID, and the checklist + // component calling this function gets its state mixed up. + // + // For example, say you copy an emoji with ID "some_emoji"; the + // result would return an emoji with ID "some_new_emoji_id". The + // checklist state would then contain one emoji with ID "some_emoji", + // and the new copy of the emoji with ID "some_new_emoji_id", leading + // to weird-looking bugs where it suddenly appears as if the searched + // status has another blank emoji attached to it. + return emoji; + }; + + // Wait for all the promises to + // resolve and remove any nulls. + const data = ( + await Promise.all(selectedEmoji.map(copyEmoji)) + ).flatMap((emoji) => emoji || []); if (errors.length !== 0) { + const errData = errors.map(e => JSON.stringify(e.data)).join(","); return { error: { status: 400, statusText: 'Bad Request', - data: {"error":`One or more errors patching custom emojis: ${errors}`}, + data: { + error: `One or more errors patching custom emojis: [${errData}]` + }, }, }; } diff --git a/web/source/settings/redux/checklist.ts b/web/source/settings/redux/checklist.ts new file mode 100644 index 000000000..d99f749ba --- /dev/null +++ b/web/source/settings/redux/checklist.ts @@ -0,0 +1,158 @@ +/* + GoToSocial + Copyright (C) GoToSocial Authors admin@gotosocial.org + SPDX-License-Identifier: AGPL-3.0-or-later + + This program is free software: you can redistribute it and/or modify + it under the terms of the GNU Affero General Public License as published by + the Free Software Foundation, either version 3 of the License, or + (at your option) any later version. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU Affero General Public License for more details. + + You should have received a copy of the GNU Affero General Public License + along with this program. If not, see . +*/ + +import { PayloadAction, createSlice } from "@reduxjs/toolkit"; +import type { Checkable } from "../lib/form/types"; +import { useReducer } from "react"; + +// https://immerjs.github.io/immer/installation#pick-your-immer-version +import { enableMapSet } from "immer"; +enableMapSet(); + +export interface ChecklistState { + entries: { [k: string]: Checkable }, + selectedEntries: Set, +} + +const initialState: ChecklistState = { + entries: {}, + selectedEntries: new Set(), +}; + +function initialHookState({ + entries, + uniqueKey, + initialValue, +}: { + entries: Checkable[], + uniqueKey: string, + initialValue: boolean, +}): ChecklistState { + const selectedEntries = new Set(); + const mappedEntries = Object.fromEntries( + entries.map((entry) => { + const key = entry[uniqueKey]; + const checked = entry.checked ?? initialValue; + + if (checked) { + selectedEntries.add(key); + } else { + selectedEntries.delete(key); + } + + return [ key, { ...entry, key, checked } ]; + }) + ); + + return { + entries: mappedEntries, + selectedEntries + }; +} + +const checklistSlice = createSlice({ + name: "checklist", + initialState, // not handled by slice itself + reducers: { + updateAll: (state, { payload: checked }: PayloadAction) => { + const selectedEntries = new Set(); + const entries = Object.fromEntries( + Object.values(state.entries).map((entry) => { + if (checked) { + // Cheekily add this to selected + // entries while we're here. + selectedEntries.add(entry.key); + } + + return [entry.key, { ...entry, checked } ]; + }) + ); + + return { entries, selectedEntries }; + }, + update: (state, { payload: { key, value } }: PayloadAction<{key: string, value: Partial}>) => { + if (value.checked !== undefined) { + if (value.checked) { + state.selectedEntries.add(key); + } else { + state.selectedEntries.delete(key); + } + } + + state.entries[key] = { + ...state.entries[key], + ...value + }; + }, + updateMultiple: (state, { payload }: PayloadAction]>>) => { + payload.forEach(([ key, value ]) => { + if (value.checked !== undefined) { + if (value.checked) { + state.selectedEntries.add(key); + } else { + state.selectedEntries.delete(key); + } + } + + state.entries[key] = { + ...state.entries[key], + ...value + }; + }); + } + } +}); + +export const actions = checklistSlice.actions; + +/** + * useChecklistReducer wraps the react 'useReducer' + * hook with logic specific to the checklist reducer. + * + * Use it in components where you need to keep track + * of checklist state. + * + * To update it, use dispatch with the actions + * exported from this module. + * + * @example + * + * ```javascript + * // Start with one entry with "checked" set to "false". + * const initialEntries = [{ key: "some_key", id: "some_id", value: "some_value", checked: false }]; + * const [state, dispatch] = useChecklistReducer(initialEntries, "id", false); + * + * // Dispatch an action to set "checked" of all entries to "true". + * let checked = true; + * dispatch(actions.updateAll(checked)); + * + * // Will log `["some_id"]` + * console.log(state.selectedEntries) + * + * // Will log `{ key: "some_key", id: "some_id", value: "some_value", checked: true }` + * console.log(state.entries["some_id"]) + * ``` + */ +export const useChecklistReducer = (entries: Checkable[], uniqueKey: string, initialValue: boolean) => { + return useReducer( + checklistSlice.reducer, + initialState, + (_) => initialHookState({ entries, uniqueKey, initialValue }) + ); +};