fix: player display invalid questions (#136)

* fix: update the context to have default current idx to -1

Author: ReidyT

cypress/e2e/Admin/create/createView.cy.ts

@@ -27,7 +27,6 @@ import {
   QUESTION_APP_SETTINGS,
 } from '../../../fixtures/appSettings';
 import { QuizNavigator } from '../../../utils/navigation';
-import { WAITING_DELAY_MS } from '../../../utils/time';
 import { fillMultipleChoiceQuestion } from './multipleChoices.cy';
 
 const newMultipleChoiceData = {
@@ -43,7 +42,7 @@ const newMultipleChoiceData = {
     },
   ],
   explanation: 'my new explanation',
-  hints: 'my new hints'
+  hints: 'my new hints',
 };
 
 describe('Create View', () => {
@@ -77,25 +76,23 @@ describe('Create View', () => {
       // Add three questions and make sure they are added to the QuestionTopBar
       cy.get(dataCyWrapper(ADD_NEW_QUESTION_TITLE_CY)).should('be.visible');
       fillMultipleChoiceQuestion(newMultipleChoiceData);
-      // eslint-disable-next-line cypress/no-unnecessary-waiting
-      cy.wait(WAITING_DELAY_MS); // Wait for the new question to appear
+      // Wait for the new question to appear
+      cy.get(`.${QUESTION_STEP_CLASSNAME}`).should('have.length', 1);
       cy.get(dataCyWrapper(NAVIGATION_ADD_QUESTION_BUTTON_CY)).click();
       cy.get(dataCyWrapper(CREATE_QUESTION_TITLE_CY))
         .should('be.visible')
         .should('have.value', '');
       fillMultipleChoiceQuestion(newMultipleChoiceData);
-      // eslint-disable-next-line cypress/no-unnecessary-waiting
-      cy.wait(WAITING_DELAY_MS);
+      // Wait for the new question to appear
+      cy.get(`.${QUESTION_STEP_CLASSNAME}`).should('have.length', 2);
       cy.get(dataCyWrapper(NAVIGATION_ADD_QUESTION_BUTTON_CY)).click();
       cy.get(dataCyWrapper(CREATE_QUESTION_TITLE_CY))
         .should('be.visible')
         .should('have.value', '');
       fillMultipleChoiceQuestion(newMultipleChoiceData);
       // Verify the questions are added to the order list by checking the number of
       // question nodes in the QuestionTopBar, as we cannot check the app settings directly
-      cy.get('html')
-        .find(`.${QUESTION_STEP_CLASSNAME}`)
-        .should('have.length', 3);
+      cy.get(`.${QUESTION_STEP_CLASSNAME}`).should('have.length', 3);
     });
   });
 
@@ -192,9 +189,7 @@ describe('Create View', () => {
       );
       cy.get(dataCyWrapper(QUESTION_BAR_CY)).should('be.visible');
       fillMultipleChoiceQuestion(newMultipleChoiceData);
-      cy.get('html')
-        .find(`.${QUESTION_STEP_CLASSNAME}`)
-        .should('have.length', 5);
+      cy.get(`.${QUESTION_STEP_CLASSNAME}`).should('have.length', 5);
     });
 
     it('Update Question type should not create a new question', () => {
@@ -210,9 +205,10 @@ describe('Create View', () => {
       cy.get(`${dataCyWrapper(CREATE_VIEW_SAVE_BUTTON_CY)}`).click();
 
       // Check the current number of questions
-      cy.get('html')
-        .find(`.${QUESTION_STEP_CLASSNAME}`)
-        .should('have.length', numberOfQuestions);
+      cy.get(`.${QUESTION_STEP_CLASSNAME}`).should(
+        'have.length',
+        numberOfQuestions
+      );
 
       // update the question type and save
       const updatedQuestion = {
@@ -236,9 +232,10 @@ describe('Create View', () => {
       cy.get(`${dataCyWrapper(CREATE_VIEW_SAVE_BUTTON_CY)}`).click();
 
       // Check that the current number of questions is still unchanged
-      cy.get('html')
-        .find(`.${QUESTION_STEP_CLASSNAME}`)
-        .should('have.length', numberOfQuestions);
+      cy.get(`.${QUESTION_STEP_CLASSNAME}`).should(
+        'have.length',
+        numberOfQuestions
+      );
 
       // Check the question title, question type, the answer and attempts.
       cy.get(`${dataCyWrapper(CREATE_QUESTION_TITLE_CY)} input`).should(

cypress/e2e/Admin/results/TableByUser.cy.ts

@@ -272,7 +272,9 @@ const testTableContent = (uId: string, ascending: boolean) => {
 
       questionTitles.push(currentQuestionName);
       if (!isOrdered(questionTitles, ascending)) {
-        throw new Error(`The questions are not ordered in ${ascending ? 'ASC' : 'DESC'}.`)
+        throw new Error(
+          `The questions are not ordered in ${ascending ? 'ASC' : 'DESC'}.`
+        );
       }
 
       const userResponse = USER_RESPONSES[uId].find(

cypress/e2e/legacy.cy.ts

@@ -3,14 +3,11 @@ import { AppSetting, Context } from '@graasp/sdk';
 import { APP_SETTING_NAMES, QuestionType } from '../../src/config/constants';
 import {
   PLAY_VIEW_QUESTION_TITLE_CY,
-  buildPlayViewTextInputCy,
   QUESTION_BAR_NEXT_CY,
+  buildPlayViewTextInputCy,
   dataCyWrapper,
 } from '../../src/config/selectors';
-import {
-  datesFactory,
-  mockAppDataFactory,
-} from '../../src/data/factories';
+import { datesFactory, mockAppDataFactory } from '../../src/data/factories';
 import { mockItem } from '../../src/data/items';
 import { mockCurrentMember } from '../../src/data/members';
 
@@ -73,7 +70,7 @@ describe('Legacy', () => {
     };
 
     // current user
-    const textAppData =  mockAppDataFactory({
+    const textAppData = mockAppDataFactory({
       item: mockItem,
       creator: mockCurrentMember,
       data: {
@@ -107,7 +104,7 @@ describe('Legacy', () => {
       },
       appContext: {
         context: Context.Builder,
-      }
+      },
     });
     cy.visit('/');
 

cypress/e2e/play/fillBlanks.cy.ts

@@ -1,5 +1,6 @@
 import { Context } from '@graasp/sdk';
 
+import { QuestionStepStyleKeys } from '../../../src/components/navigation/questionNavigation/types';
 import {
   AppSettingData,
   TextAppDataData,
@@ -29,7 +30,6 @@ import {
   QUESTION_APP_SETTINGS,
   setAttemptsOnAppSettings,
 } from '../../fixtures/appSettings';
-import { QuestionStepStyleKeys } from '../../../src/components/navigation/questionNavigation/types';
 
 const { data } = QUESTION_APP_SETTINGS.find(
   ({ name, data }) =>

cypress/fixtures/appSettings.ts

@@ -156,7 +156,7 @@ export const CAPITAL_FRANCE_SETTING = {
       },
     ],
     explanation: 'Paris is the capital of France.',
-    hints: 'Think about the iconic Eiffel Tower.'
+    hints: 'Think about the iconic Eiffel Tower.',
   },
   name: APP_SETTING_NAMES.QUESTION,
 };

cypress/utils/autoScrollableMenuSelected.ts

@@ -15,10 +15,8 @@ export const verifySelectedMenu = (
   labels: { id: string }[]
 ) => {
   labels.forEach(({ id }) => {
-    cy.get(
-      dataCyWrapper(
-        buildAutoScrollableMenuLinkCy(id)
-      )
-    ).should('be.visible');
+    cy.get(dataCyWrapper(buildAutoScrollableMenuLinkCy(id))).should(
+      'be.visible'
+    );
   });
 };

cypress/utils/time.ts

@@ -19,9 +19,9 @@ import {
   queryClient,
 } from '../config/queryClient';
 import { mockContext as defaultMockContext } from '../data/config';
+import { mockMembers } from '../data/members';
 import graaspTheme from '../layout/theme';
 import View from './views/View';
-import { mockMembers } from '../data/members';
 
 export const App = () => {
   const [mockContext, setMockContext] = useObjectState(defaultMockContext);

src/components/App.tsx

@@ -31,7 +31,7 @@ export const ReorderAnimation = <T extends DataElementType>({
 }: Props<T>) => {
   const [totalHeight, setTotalHeight] = useState<number>(0);
   // Tracks the heights of each element to recompute totalHeight when heights changed.
-  // That means, if a element's height changed, the map is updated and he totalHeight 
+  // That means, if a element's height changed, the map is updated and he totalHeight
   // recomputed with all the heights, including the updated one.
   const elementHeights = new Map<string, number>();
   // this variable is used to set the next element to the good y

src/components/common/animations/ReorderAnimation.tsx

@@ -27,6 +27,7 @@ type ContextType = {
   addQuestion: () => void;
   saveQuestion: (newData: QuestionData) => Promise<void>;
   isSettingsFetching: boolean;
+  isLoaded: boolean;
   saveOrder: (order: string[], currQuestionId?: string) => void;
 };
 
@@ -51,11 +52,16 @@ export const QuizProvider = ({ children }: Props) => {
   const { mutateAsync: patchAppSettingAsync } = mutations.usePatchAppSetting();
   // current question idx
   // -1 if we are adding a new question
-  const [currentIdx, setCurrentIdx] = useState(0);
+  const [currentIdx, setCurrentIdx] = useState(-1);
 
   const [orderSetting, setOrderSetting] = useState<AppSetting>();
   const [order, setOrder] = useState<string[]>([]);
 
+  // This state indicates if the questions were received and the question order set correctly.
+  // It allows QuizNavigation to display the Add question button when loading stops
+  // and the current index updated correctly according if there is questions or not.
+  const [isLoaded, setIsLoaded] = useState(false);
+
   // Here use type of CurrentQuestion because only the id of appSetting is needed...
   const [currentQuestion, setCurrentQuestion] =
     useState<CurrentQuestion>(DEFAULT_QUESTION);
@@ -201,12 +207,37 @@ export const QuizProvider = ({ children }: Props) => {
         APP_SETTING_NAMES.QUESTION_LIST
       );
 
+      // Get all questions id. To support legacy code, if no question id, the id is used instead.
+      const questionIds = getSettingsByName(
+        settings,
+        APP_SETTING_NAMES.QUESTION
+      ).map((appSetting) => appSetting?.data?.questionId ?? appSetting.id);
+
+      const filteredOrder: string[] = [];
       if (newOrderSetting && newOrderSetting.length > 0) {
         const value = newOrderSetting[0] as QuestionListType;
         setOrderSetting(value);
-        setOrder(value?.data?.list ?? []);
+        // Filter out questions that are not well formatted in AppSettings.
+        filteredOrder.push(
+          ...(value?.data?.list.filter((id) => questionIds.includes(id)) ?? [])
+        );
+        setOrder(filteredOrder);
+      }
+
+      // if it is first loading, set is loaded to true.
+      if (!isLoaded) {
+        // If there are questions, set current idx to the first one.
+        // If it has already set current idx, don't do it again to not reset curr question on order changed.
+        if (filteredOrder.length) {
+          setCurrentIdx(0);
+        }
+
+        setIsLoaded(true);
       }
     }
+    // Disable exhaustive-deps for isLoaded, because we don't want
+    // to reload this useEffect when isLoaded has changed.
+    // eslint-disable-next-line react-hooks/exhaustive-deps
   }, [settings]);
 
   // set current question
@@ -249,11 +280,22 @@ export const QuizProvider = ({ children }: Props) => {
   }, [patchAppSettingAsync, settings]);
 
   const value: ContextType = useMemo(() => {
+    const validIds =
+      getSettingsByName(settings, APP_SETTING_NAMES.QUESTION_LIST)[0]?.data
+        ?.list ?? [];
+
     const questions = settings
-      ? (getSettingsByName(
-          settings,
-          APP_SETTING_NAMES.QUESTION
-        ) as QuestionDataAppSetting[])
+      ? (
+          getSettingsByName(
+            settings,
+            APP_SETTING_NAMES.QUESTION
+          ) as QuestionDataAppSetting[]
+        )
+          // Filter out questions that are not well formatted in AppSettings.
+          .filter(
+            (q) =>
+              validIds.includes(q.data.questionId) || validIds.includes(q.id)
+          )
       : [];
 
     return {
@@ -268,6 +310,7 @@ export const QuizProvider = ({ children }: Props) => {
       addQuestion,
       saveQuestion,
       isSettingsFetching,
+      isLoaded,
       saveOrder,
     };
   }, [
@@ -280,6 +323,7 @@ export const QuizProvider = ({ children }: Props) => {
     moveToPreviousQuestion,
     saveQuestion,
     isSettingsFetching,
+    isLoaded,
     saveOrder,
   ]);