From e0e398fa8bd867bd8030a34b9359a509c00c9795 Mon Sep 17 00:00:00 2001 From: Adrian Cowan Date: Sun, 29 Jan 2023 02:24:30 +1100 Subject: [PATCH 1/2] website: Improve reliability of e2e tests --- website/cypress/support/commands.ts | 1 + .../src/components/Loading/LoadingScreen.jsx | 2 +- .../src/components/Survey/TaskControls.tsx | 85 ++++++++++--------- website/src/components/TaskPage/TaskPage.tsx | 45 +++++----- website/src/components/Tasks/CreateTask.tsx | 4 +- website/src/components/Tasks/EvaluateTask.tsx | 7 +- .../components/Tasks/Task/Task.stories.tsx | 13 +-- website/src/components/Tasks/Task/Task.tsx | 29 ++++--- website/src/hooks/tasks/useGenericTaskAPI.tsx | 47 ++++++---- website/src/pages/api/new_task/[task_type].ts | 11 ++- website/src/types/Hooks.ts | 32 +++---- website/src/types/Task.ts | 16 +++- website/src/types/Tasks.ts | 8 +- 13 files changed, 176 insertions(+), 124 deletions(-) diff --git a/website/cypress/support/commands.ts b/website/cypress/support/commands.ts index dd78aae5..2507116d 100644 --- a/website/cypress/support/commands.ts +++ b/website/cypress/support/commands.ts @@ -46,6 +46,7 @@ Cypress.Commands.add("signInUsingEmailedLink", (emailAddress) => { // Find and use login link const loginLink = emails.pop().html.match(/href="[^"]+(\/api\/auth\/callback\/[^"]+?)"/)[1]; cy.visit(loginLink); + cy.url().should("include", "/dashboard"); }); }); diff --git a/website/src/components/Loading/LoadingScreen.jsx b/website/src/components/Loading/LoadingScreen.jsx index 645544e6..9578ff24 100644 --- a/website/src/components/Loading/LoadingScreen.jsx +++ b/website/src/components/Loading/LoadingScreen.jsx @@ -1,4 +1,4 @@ -import { Box, Center, Progress, Text, useColorModeValue } from "@chakra-ui/react"; +import { Box, Center, Progress, Text } from "@chakra-ui/react"; export const LoadingScreen = ({ text = "Loading..." } = {}) => { return ( diff --git a/website/src/components/Survey/TaskControls.tsx b/website/src/components/Survey/TaskControls.tsx index a3c3ffdd..a0b85fc3 100644 --- a/website/src/components/Survey/TaskControls.tsx +++ b/website/src/components/Survey/TaskControls.tsx @@ -1,4 +1,4 @@ -import { Box, Flex, IconButton, Tooltip, useColorModeValue } from "@chakra-ui/react"; +import { Box, Flex, IconButton, Progress, Tooltip, useColorModeValue } from "@chakra-ui/react"; import { Edit2 } from "lucide-react"; import { SkipButton } from "src/components/Buttons/Skip"; import { SubmitButton } from "src/components/Buttons/Submit"; @@ -9,55 +9,58 @@ import { BaseTask } from "src/types/Task"; export interface TaskControlsProps { task: BaseTask; taskStatus: TaskStatus; + isLoading: boolean; onEdit: () => void; onReview: () => void; onSubmit: () => void; onSkip: (reason: string) => void; } -export const TaskControls = ({ task, taskStatus, onEdit, onReview, onSubmit, onSkip }: TaskControlsProps) => { +export const TaskControls = ({ + task, + taskStatus, + isLoading, + onEdit, + onReview, + onSubmit, + onSkip, +}: TaskControlsProps) => { const backgroundColor = useColorModeValue("white", "gray.800"); return ( - - - - {taskStatus.mode === "EDIT" ? ( - <> - - - Review - - - ) : ( - <> - - } /> - - - Submit - - - )} + + {isLoading && } + + + + {taskStatus.mode === "EDIT" ? ( + <> + + + Review + + + ) : ( + <> + + } /> + + + Submit + + + )} + ); diff --git a/website/src/components/TaskPage/TaskPage.tsx b/website/src/components/TaskPage/TaskPage.tsx index e1ecc79c..8b3577f5 100644 --- a/website/src/components/TaskPage/TaskPage.tsx +++ b/website/src/components/TaskPage/TaskPage.tsx @@ -4,9 +4,10 @@ import { TaskEmptyState } from "src/components/EmptyState"; import { LoadingScreen } from "src/components/Loading/LoadingScreen"; import { Task } from "src/components/Tasks/Task"; import { TaskInfos } from "src/components/Tasks/TaskTypes"; -import { ERROR_CODES, taskApiHooks } from "src/lib/constants"; +import { taskApiHooks } from "src/lib/constants"; import { getTypeSafei18nKey } from "src/lib/i18n"; import { TaskType } from "src/types/Task"; +import { KnownTaskType } from "src/types/Tasks"; type TaskPageProps = { type: TaskType; @@ -15,26 +16,30 @@ type TaskPageProps = { export const TaskPage = ({ type }: TaskPageProps) => { const { t } = useTranslation(["tasks", "common"]); const taskApiHook = taskApiHooks[type]; - const { tasks, isLoading, reset, trigger, error } = taskApiHook(type); + const { response, isLoading, completeTask, skipTask } = taskApiHook(type); const taskInfo = TaskInfos.find((taskType) => taskType.type === type); - if (isLoading) { - return ; + switch (response.status) { + case "AWAITING_INITIAL": + return ; + case "NONE_AVAILABLE": + return ; + case "AVAILABLE": + return ( + <> + + {t(getTypeSafei18nKey(`${taskInfo.id}.label`))} + + + + + ); } - - if (tasks.length === 0 || error?.errorCode === ERROR_CODES.TASK_REQUESTED_TYPE_NOT_AVAILABLE) { - return ; - } - - const task = tasks[0]; - - return ( - <> - - {t(getTypeSafei18nKey(`${taskInfo.id}.label`))} - - - - - ); }; diff --git a/website/src/components/Tasks/CreateTask.tsx b/website/src/components/Tasks/CreateTask.tsx index 5276a183..c29b2491 100644 --- a/website/src/components/Tasks/CreateTask.tsx +++ b/website/src/components/Tasks/CreateTask.tsx @@ -8,7 +8,7 @@ import { TaskSurveyProps } from "src/components/Tasks/Task"; import { TaskHeader } from "src/components/Tasks/TaskHeader"; import { getTypeSafei18nKey } from "src/lib/i18n"; import { TaskType } from "src/types/Task"; -import { CreateAssistantReplyTask, CreateInitialPromptTask, CreatePrompterReplyTask } from "src/types/Tasks"; +import { CreateTaskType } from "src/types/Tasks"; export const CreateTask = ({ task, @@ -17,7 +17,7 @@ export const CreateTask = ({ isDisabled, onReplyChanged, onValidityChanged, -}: TaskSurveyProps) => { +}: TaskSurveyProps) => { const { t, i18n } = useTranslation(["tasks", "common"]); const cardColor = useColorModeValue("gray.50", "gray.800"); const titleColor = useColorModeValue("gray.800", "gray.300"); diff --git a/website/src/components/Tasks/EvaluateTask.tsx b/website/src/components/Tasks/EvaluateTask.tsx index b554ffd4..e2f07749 100644 --- a/website/src/components/Tasks/EvaluateTask.tsx +++ b/website/src/components/Tasks/EvaluateTask.tsx @@ -6,7 +6,7 @@ import { SurveyCard } from "src/components/Survey/SurveyCard"; import { TaskSurveyProps } from "src/components/Tasks/Task"; import { TaskHeader } from "src/components/Tasks/TaskHeader"; import { TaskType } from "src/types/Task"; -import { RankAssistantRepliesTask, RankInitialPromptsTask, RankPrompterRepliesTask } from "src/types/Tasks"; +import { RankTaskType } from "src/types/Tasks"; export const EvaluateTask = ({ task, @@ -15,10 +15,7 @@ export const EvaluateTask = ({ isDisabled, onReplyChanged, onValidityChanged, -}: TaskSurveyProps< - RankInitialPromptsTask | RankAssistantRepliesTask | RankPrompterRepliesTask, - { ranking: number[] } ->) => { +}: TaskSurveyProps) => { const cardColor = useColorModeValue("gray.50", "gray.800"); const [ranking, setRanking] = useState(null); diff --git a/website/src/components/Tasks/Task/Task.stories.tsx b/website/src/components/Tasks/Task/Task.stories.tsx index bb0da0e3..fe7122b9 100644 --- a/website/src/components/Tasks/Task/Task.stories.tsx +++ b/website/src/components/Tasks/Task/Task.stories.tsx @@ -7,8 +7,10 @@ export default { component: Task, }; -const Template = ({ frontendId, task, trigger, mutate }) => { - return ; +const Template = ({ frontendId, task, isLoading, completeTask, skipTask }) => { + return ( + + ); }; export const Default = Template.bind({}); @@ -23,10 +25,11 @@ Default.args = { type: "label_prompter_reply", valid_labels: ["spam", "fails_task"], }, - trigger: (id, update_type, content) => { + isLoading: false, + completeTask: (id, update_type, content) => { console.log(content); }, - mutate: () => { - console.log("mutate"); + skipTask: () => { + console.log("skip"); }, }; diff --git a/website/src/components/Tasks/Task/Task.tsx b/website/src/components/Tasks/Task/Task.tsx index 51ba6fa3..738c3667 100644 --- a/website/src/components/Tasks/Task/Task.tsx +++ b/website/src/components/Tasks/Task/Task.tsx @@ -10,6 +10,7 @@ import { UnchangedWarning } from "src/components/Tasks/UnchangedWarning"; import { post } from "src/lib/api"; import { getTypeSafei18nKey } from "src/lib/i18n"; import { BaseTask, TaskContent, TaskReplyValidity } from "src/types/Task"; +import { CreateTaskType, KnownTaskType, LabelTaskType, RankTaskType } from "src/types/Tasks"; import useSWRMutation from "swr/mutation"; interface EditMode { @@ -62,7 +63,15 @@ export interface TaskSurveyProps { onValidityChanged: (validity: TaskReplyValidity) => void; } -export const Task = ({ frontendId, task, trigger, mutate }) => { +interface TaskProps { + frontendId: string; + task: KnownTaskType; + isLoading: boolean; + completeTask: (TaskContent) => void; + skipTask: () => void; +} + +export const Task = ({ frontendId, task, isLoading, completeTask, skipTask }: TaskProps) => { const { t } = useTranslation("tasks"); const [taskStatus, taskEvent] = useReducer( ( @@ -117,14 +126,13 @@ export const Task = ({ frontendId, task, trigger, mutate }) => { const rootEl = useRef(null); - const taskType = useMemo( - () => TaskInfos.find((taskType) => taskType.type === task.type && taskType.mode === task.mode), - [task.type, task.mode] - ); + const taskType = useMemo(() => { + return TaskInfos.find((taskType) => taskType.type === task.type); + }, [task.type]); const { trigger: sendRejection } = useSWRMutation("/api/reject_task", post, { onSuccess: async () => { - mutate(); + skipTask(); }, }); @@ -144,7 +152,7 @@ export const Task = ({ frontendId, task, trigger, mutate }) => { const submitResponse = () => { if (taskStatus.mode === "REVIEW") { - trigger({ + completeTask({ id: frontendId, update_type: taskType.update_type, content: replyContent.current, @@ -159,7 +167,7 @@ export const Task = ({ frontendId, task, trigger, mutate }) => { case TaskCategory.Create: return ( { case TaskCategory.Evaluate: return ( { case TaskCategory.Label: return ( { taskEvent({ action: "RETURN_EDIT" })} onReview={() => taskEvent({ action: "REVIEW" })} onSubmit={submitResponse} diff --git a/website/src/hooks/tasks/useGenericTaskAPI.tsx b/website/src/hooks/tasks/useGenericTaskAPI.tsx index d258ff47..a58583c1 100644 --- a/website/src/hooks/tasks/useGenericTaskAPI.tsx +++ b/website/src/hooks/tasks/useGenericTaskAPI.tsx @@ -1,26 +1,43 @@ import { useState } from "react"; import { get, post } from "src/lib/api"; -import { BaseTask, TaskResponse, TaskType as TaskTypeEnum } from "src/types/Task"; +import { TaskApiHook } from "src/types/Hooks"; +import { BaseTask, TaskAvailableResponse, TaskResponse, TaskType as TaskTypeEnum } from "src/types/Task"; import useSWRImmutable from "swr/immutable"; import useSWRMutation from "swr/mutation"; -export const useGenericTaskAPI = (taskType: TaskTypeEnum) => { - type ConcreteTaskResponse = TaskResponse; +export const useGenericTaskAPI = (taskType: TaskTypeEnum): TaskApiHook => { + const [response, setReponse] = useState>({ status: "AWAITING_INITIAL" }); + const [isLoading, setIsLoading] = useState(true); - const [tasks, setTasks] = useState([]); + const { mutate: requestNewTask } = useSWRImmutable>( + "/api/new_task/" + taskType, + get, + { + onSuccess: (response) => { + setIsLoading(false); + setReponse({ status: "AVAILABLE", ...response }); + }, + onError: () => { + // We could check for code 503 here for truely unavailable, but we need to do something with other errors anyway. + setIsLoading(false); + setReponse({ status: "NONE_AVAILABLE" }); + }, + revalidateOnMount: true, + dedupingInterval: 500, + } + ); - const { isLoading, mutate, error } = useSWRImmutable("/api/new_task/" + taskType, get, { - onSuccess: (data) => setTasks([data]), - revalidateOnMount: true, - dedupingInterval: 500, - }); - - const { trigger } = useSWRMutation("/api/update_task", post, { - onSuccess: async (newTask: ConcreteTaskResponse) => { - setTasks((oldTasks) => [...oldTasks, newTask]); - mutate(); + const { trigger: completeTask } = useSWRMutation>("/api/update_task", post, { + onSuccess: () => { + setIsLoading(true); + requestNewTask(); + }, + onError: () => { + // We could check for code 503 here for truely unavailable, but we need to do something with other errors anyway. + setIsLoading(false); + setReponse({ status: "NONE_AVAILABLE" }); }, }); - return { tasks, isLoading, trigger, error, reset: mutate }; + return { response, isLoading, completeTask, skipTask: requestNewTask }; }; diff --git a/website/src/pages/api/new_task/[task_type].ts b/website/src/pages/api/new_task/[task_type].ts index 34993f4f..6b405c7e 100644 --- a/website/src/pages/api/new_task/[task_type].ts +++ b/website/src/pages/api/new_task/[task_type].ts @@ -1,4 +1,6 @@ import { withoutRole } from "src/lib/auth"; +import { ERROR_CODES } from "src/lib/constants"; +import { OasstError } from "src/lib/oasst_api_client"; import { createApiClientFromUser } from "src/lib/oasst_client_factory"; import prisma from "src/lib/prismadb"; import { getBackendUserCore, getUserLanguage } from "src/lib/users"; @@ -22,8 +24,13 @@ const handler = withoutRole("banned", async (req, res, token) => { try { task = await oasstApiClient.fetchTask(task_type as string, user, userLanguage); } catch (err) { - console.error(err); - res.status(500).json(err); + if (err instanceof OasstError && err.errorCode === ERROR_CODES.TASK_REQUESTED_TYPE_NOT_AVAILABLE) { + res.status(503).json({}); + return; + } else { + console.error(err); + res.status(500).json(err); + } return; } diff --git a/website/src/types/Hooks.ts b/website/src/types/Hooks.ts index 8fd9aa4f..8da40527 100644 --- a/website/src/types/Hooks.ts +++ b/website/src/types/Hooks.ts @@ -1,26 +1,16 @@ -import { MutatorCallback, MutatorOptions } from "swr"; +import { BaseTask, TaskContent, TaskResponse, TaskType } from "src/types/Task"; -import { BaseTask, TaskResponse, TaskType } from "./Task"; +interface TaskInteraction { + id: string; + update_type: string; + content: TaskContent; +} -type ConcreteTaskResponse = TaskResponse; -type TaskError = { errorCode: number; message: string }; - -type Trigger = ( - extraArgument?: unknown, - options?: MutatorOptions -) => Promise; - -type Reset = ( - data?: ConcreteTaskResponse | Promise | MutatorCallback, - opts?: boolean | MutatorOptions -) => Promise; - -type TaskAPIHook = { - tasks: TaskResponse[]; +export type TaskApiHook = { + response: TaskResponse; isLoading: boolean; - error: TaskError; - trigger: Trigger; - reset: Reset; + completeTask: (interaction: TaskInteraction) => void; + skipTask: () => void; }; -export type TaskApiHooks = Record TaskAPIHook>; +export type TaskApiHooks = Record TaskApiHook>; diff --git a/website/src/types/Task.ts b/website/src/types/Task.ts index 7ae48138..4f8571d8 100644 --- a/website/src/types/Task.ts +++ b/website/src/types/Task.ts @@ -29,12 +29,26 @@ export interface BaseTask { type: TaskType; } -export interface TaskResponse { +export interface TaskAvailableResponse { id: string; userId: string; task: Task; } +interface TaskAvailable extends TaskAvailableResponse { + status: "AVAILABLE"; +} + +interface AwaitingInitialTask { + status: "AWAITING_INITIAL"; +} + +interface NoTaskAvailable { + status: "NONE_AVAILABLE"; +} + +export type TaskResponse = TaskAvailable | AwaitingInitialTask | NoTaskAvailable; + export type TaskReplyValidity = "DEFAULT" | "VALID" | "INVALID"; export type AvailableTasks = { [taskType in TaskType]: number }; diff --git a/website/src/types/Tasks.ts b/website/src/types/Tasks.ts index 5fbc84c7..737392bd 100644 --- a/website/src/types/Tasks.ts +++ b/website/src/types/Tasks.ts @@ -16,6 +16,8 @@ export interface CreatePrompterReplyTask extends BaseTask { conversation: Conversation; } +export type CreateTaskType = CreateInitialPromptTask | CreateAssistantReplyTask | CreatePrompterReplyTask; + export interface RankInitialPromptsTask extends BaseTask { type: TaskType.rank_initial_prompts; prompts: string[]; @@ -33,6 +35,8 @@ export interface RankPrompterRepliesTask extends BaseTask { replies: string[]; } +export type RankTaskType = RankInitialPromptsTask | RankAssistantRepliesTask | RankPrompterRepliesTask; + export interface Label { display_text: string; help_text: string; @@ -68,4 +72,6 @@ export interface LabelInitialPromptTask extends BaseLabelTask { prompt: string; } -export type LabelTaskType = LabelAssistantReplyTask | LabelPrompterReplyTask | LabelInitialPromptTask; +export type LabelTaskType = LabelInitialPromptTask | LabelAssistantReplyTask | LabelPrompterReplyTask; + +export type KnownTaskType = CreateTaskType | RankTaskType | LabelTaskType; From 21c4a1263abe20d5516f295b0541a9fe64f2be89 Mon Sep 17 00:00:00 2001 From: Adrian Cowan Date: Sun, 29 Jan 2023 12:09:58 +1100 Subject: [PATCH 2/2] website: Add head to task pages even if no task and improve var name --- website/src/components/TaskPage/TaskPage.tsx | 44 +++++++++++-------- website/src/hooks/tasks/useGenericTaskAPI.tsx | 8 ++-- website/src/pages/api/new_task/[task_type].ts | 1 - website/src/types/Task.ts | 6 +-- 4 files changed, 33 insertions(+), 26 deletions(-) diff --git a/website/src/components/TaskPage/TaskPage.tsx b/website/src/components/TaskPage/TaskPage.tsx index 8b3577f5..77b1bfe9 100644 --- a/website/src/components/TaskPage/TaskPage.tsx +++ b/website/src/components/TaskPage/TaskPage.tsx @@ -19,27 +19,35 @@ export const TaskPage = ({ type }: TaskPageProps) => { const { response, isLoading, completeTask, skipTask } = taskApiHook(type); const taskInfo = TaskInfos.find((taskType) => taskType.type === type); - switch (response.status) { + let body; + switch (response.taskAvailability) { case "AWAITING_INITIAL": - return ; + body = ; + break; case "NONE_AVAILABLE": - return ; + body = ; + break; case "AVAILABLE": - return ( - <> - - {t(getTypeSafei18nKey(`${taskInfo.id}.label`))} - - - - + body = ( + ); + break; } + + return ( + <> + + {t(getTypeSafei18nKey(`${taskInfo.id}.label`))} + + + {body} + + ); }; diff --git a/website/src/hooks/tasks/useGenericTaskAPI.tsx b/website/src/hooks/tasks/useGenericTaskAPI.tsx index a58583c1..1dc12f81 100644 --- a/website/src/hooks/tasks/useGenericTaskAPI.tsx +++ b/website/src/hooks/tasks/useGenericTaskAPI.tsx @@ -6,7 +6,7 @@ import useSWRImmutable from "swr/immutable"; import useSWRMutation from "swr/mutation"; export const useGenericTaskAPI = (taskType: TaskTypeEnum): TaskApiHook => { - const [response, setReponse] = useState>({ status: "AWAITING_INITIAL" }); + const [response, setReponse] = useState>({ taskAvailability: "AWAITING_INITIAL" }); const [isLoading, setIsLoading] = useState(true); const { mutate: requestNewTask } = useSWRImmutable>( @@ -15,12 +15,12 @@ export const useGenericTaskAPI = (taskType: TaskTypeE { onSuccess: (response) => { setIsLoading(false); - setReponse({ status: "AVAILABLE", ...response }); + setReponse({ taskAvailability: "AVAILABLE", ...response }); }, onError: () => { // We could check for code 503 here for truely unavailable, but we need to do something with other errors anyway. setIsLoading(false); - setReponse({ status: "NONE_AVAILABLE" }); + setReponse({ taskAvailability: "NONE_AVAILABLE" }); }, revalidateOnMount: true, dedupingInterval: 500, @@ -35,7 +35,7 @@ export const useGenericTaskAPI = (taskType: TaskTypeE onError: () => { // We could check for code 503 here for truely unavailable, but we need to do something with other errors anyway. setIsLoading(false); - setReponse({ status: "NONE_AVAILABLE" }); + setReponse({ taskAvailability: "NONE_AVAILABLE" }); }, }); diff --git a/website/src/pages/api/new_task/[task_type].ts b/website/src/pages/api/new_task/[task_type].ts index 6b405c7e..08c0e42b 100644 --- a/website/src/pages/api/new_task/[task_type].ts +++ b/website/src/pages/api/new_task/[task_type].ts @@ -26,7 +26,6 @@ const handler = withoutRole("banned", async (req, res, token) => { } catch (err) { if (err instanceof OasstError && err.errorCode === ERROR_CODES.TASK_REQUESTED_TYPE_NOT_AVAILABLE) { res.status(503).json({}); - return; } else { console.error(err); res.status(500).json(err); diff --git a/website/src/types/Task.ts b/website/src/types/Task.ts index 4f8571d8..5e2abf16 100644 --- a/website/src/types/Task.ts +++ b/website/src/types/Task.ts @@ -36,15 +36,15 @@ export interface TaskAvailableResponse { } interface TaskAvailable extends TaskAvailableResponse { - status: "AVAILABLE"; + taskAvailability: "AVAILABLE"; } interface AwaitingInitialTask { - status: "AWAITING_INITIAL"; + taskAvailability: "AWAITING_INITIAL"; } interface NoTaskAvailable { - status: "NONE_AVAILABLE"; + taskAvailability: "NONE_AVAILABLE"; } export type TaskResponse = TaskAvailable | AwaitingInitialTask | NoTaskAvailable;