Fix oppia#19433: Fixing self loop warning in exploration editor (oppi…

…a#19805) * Fixed inaccurate self-loop warning by updating current state everytime the warning function runs. * created a new getter function for active state name and removed outdated state name variable. * made required changes to the test file. * fixed some lint errors * fixed failing type checks * added an e2e test to test the fix * simplified the test * improved the test further * shifted infrastructural part to corresponding page object * made the yesy more robust * addressed reviewer comments * addressed reviewer comments * addressed reviewer comments * adding comments to the test to make it more comprehensive
yash1378 · Mar 2, 2024 · 215c0aa · 215c0aa
1 parent d9b7c38
commit 215c0aa
Show file tree

Hide file tree

Showing 7 changed files with 77 additions and 24 deletions.
diff --git a/core/templates/components/state-editor/state-responses-editor/state-responses.component.html b/core/templates/components/state-editor/state-responses-editor/state-responses.component.html
@@ -39,7 +39,7 @@ <h3 class="oppia-exp-answer-card-header" tabindex="0">Learner's Answers and Oppi
                 <span *ngIf="editabilityService.isEditable()" class="fas fa-grip-vertical draggable-icon-indicator"></span>
               </span>
 
-              <div class="oppia-rule-header-warning-placement" *ngIf="isSelfLoopThatIsMarkedCorrect(answerGroup.outcome) || isSelfLoopWithNoFeedback(answerGroup.outcome)" (click)="changeActiveAnswerGroupIndex(index)"
+              <div class="oppia-rule-header-warning-placement e2e-test-response-self-loop-warning" *ngIf="isSelfLoopThatIsMarkedCorrect(answerGroup.outcome) || isSelfLoopWithNoFeedback(answerGroup.outcome)" (click)="changeActiveAnswerGroupIndex(index)"
                    ngbTooltip="{{getOutcomeTooltip(answerGroup.outcome)}}"
                    tabindex="0"
                    data-container="body"

diff --git a/...emplates/components/state-editor/state-responses-editor/state-responses.component.spec.ts b/...emplates/components/state-editor/state-responses-editor/state-responses.component.spec.ts
@@ -318,7 +318,6 @@ describe('State Responses Component', () => {
     expect(component.responseCardIsShown).toBe(false);
     expect(component.enableSolicitAnswerDetailsFeature).toBe(false);
     expect(component.SHOW_TRAINABLE_UNRESOLVED_ANSWERS).toBe(false);
-    expect(component.stateName).toBeUndefined();
     expect(component.misconceptionsBySkill).toBeUndefined();
     expect(component.inapplicableSkillMisconceptionIds).toBeUndefined();
 
@@ -327,7 +326,7 @@ describe('State Responses Component', () => {
     expect(component.responseCardIsShown).toBe(true);
     expect(component.enableSolicitAnswerDetailsFeature).toBe(true);
     expect(component.SHOW_TRAINABLE_UNRESOLVED_ANSWERS).toBe(false);
-    expect(component.stateName).toBe('Hola');
+    expect(component.getActiveStateName()).toBe('Hola');
     expect(component.misconceptionsBySkill).toEqual({});
     expect(component.inapplicableSkillMisconceptionIds).toEqual(['id1']);
 
@@ -647,7 +646,8 @@ describe('State Responses Component', () => {
   });
 
   it('should check if outcome has no feedback with self loop', () => {
-    component.stateName = 'State Name';
+    spyOn(stateEditorService, 'getActiveStateName').and.returnValue(
+      'State Name');
     let outcome1 = outcomeObjectFactory.createNew(
       'State Name', '1', '', []);
     let outcome2 = outcomeObjectFactory.createNew(
@@ -678,12 +678,11 @@ describe('State Responses Component', () => {
       refresher_exploration_id: 'test',
       missing_prerequisite_skill_id: 'test_skill_id'
     });
-    component.stateName = 'State Name';
+    spyOn(stateEditorService, 'getActiveStateName').and.returnValues(
+      'State Name', 'Hola');
 
     expect(component.isSelfLoopThatIsMarkedCorrect(outcome)).toBe(true);
 
-    component.stateName = 'Hola';
-
     expect(component.isSelfLoopThatIsMarkedCorrect(outcome)).toBe(false);
   });
 
@@ -701,7 +700,8 @@ describe('State Responses Component', () => {
       refresher_exploration_id: 'test',
       missing_prerequisite_skill_id: 'test_skill_id'
     });
-    component.stateName = 'State Name';
+    spyOn(stateEditorService, 'getActiveStateName').and.returnValue(
+      'State Name');
 
     expect(component.isSelfLoopThatIsMarkedCorrect(outcome)).toBe(true);
   });
@@ -760,7 +760,8 @@ describe('State Responses Component', () => {
       refresher_exploration_id: 'test',
       missing_prerequisite_skill_id: 'test_skill_id'
     });
-    component.stateName = 'State Name';
+    spyOn(stateEditorService, 'getActiveStateName').and.returnValue(
+      'State Name');
 
     expect(component.getOutcomeTooltip(outcome)).toBe(
       'Self-loops should not be labelled as correct.');
@@ -1211,7 +1212,7 @@ describe('State Responses Component', () => {
   });
 
   it('should check if outcome is looping', () => {
-    component.stateName = 'Hola';
+    spyOn(stateEditorService, 'getActiveStateName').and.returnValue('Hola');
     expect(component.isOutcomeLooping(outcomeObjectFactory.createNew(
       'Hola', '', '', []))).toBe(true);
     expect(component.isOutcomeLooping(outcomeObjectFactory.createNew(

diff --git a/core/templates/components/state-editor/state-responses-editor/state-responses.component.ts b/core/templates/components/state-editor/state-responses-editor/state-responses.component.ts
@@ -63,7 +63,6 @@ export class StateResponsesComponent implements OnInit, OnDestroy {
   activeAnswerGroupIndex!: number;
   // State name is null if their is no state selected or have no active state.
   // This is the case when the user is creating a new state.
-  stateName!: string | null;
   @Output() onResponsesInitialized = new EventEmitter<void>();
   @Output() onSaveInteractionAnswerGroups = (
     new EventEmitter<AnswerGroup[] | AnswerGroup>());
@@ -143,19 +142,23 @@ export class StateResponsesComponent implements OnInit, OnDestroy {
     this.stateSolicitAnswerDetailsService.saveDisplayedValue();
   }
 
+  getActiveStateName(): string | null {
+    return this.stateEditorService.getActiveStateName();
+  }
+
   isSelfLoopWithNoFeedback(outcome: Outcome): boolean | void {
-    if (outcome && typeof outcome === 'object' && this.stateName &&
+    let currentStateName = this.getActiveStateName();
+    if (outcome && typeof outcome === 'object' && currentStateName &&
       outcome.constructor.name === 'Outcome') {
-      return outcome.isConfusing(this.stateName);
+      return outcome.isConfusing(currentStateName);
     }
   }
 
   isSelfLoopThatIsMarkedCorrect(outcome: Outcome): boolean {
     if (!outcome) {
       return false;
     } else {
-      let currentStateName = this.stateName;
-
+      const currentStateName = this.getActiveStateName();
       return (
         (outcome.dest === currentStateName) &&
         outcome.labelledAsCorrect);
@@ -478,7 +481,7 @@ export class StateResponsesComponent implements OnInit, OnDestroy {
   }
 
   isOutcomeLooping(outcome: Outcome): boolean {
-    let activeStateName = this.stateName;
+    let activeStateName = this.getActiveStateName();
     return outcome && (outcome.dest === activeStateName);
   }
 
@@ -572,7 +575,6 @@ export class StateResponsesComponent implements OnInit, OnDestroy {
     this.SHOW_TRAINABLE_UNRESOLVED_ANSWERS = (
       AppConstants.SHOW_TRAINABLE_UNRESOLVED_ANSWERS);
     this.responseCardIsShown = true;
-    this.stateName = this.stateEditorService.getActiveStateName();
     this.enableSolicitAnswerDetailsFeature = (
       AppConstants.ENABLE_SOLICIT_ANSWER_DETAILS_FEATURE);
     this.misconceptionsBySkill = {};

diff --git a/...mplates/pages/exploration-player-page/learner-experience/conversation-skin.component.html b/...mplates/pages/exploration-player-page/learner-experience/conversation-skin.component.html
@@ -100,7 +100,7 @@
 <div *ngIf="isProgressClearanceMessageShown()" class="oppia-exploration-checkpoints-message">
   <div id="toast-container" class="toast-top-center toast-container">
     <div class="toast-info checkpoints-toast-info ngx-toastr ng-trigger ng-trigger-flyInOut">
-      <div aria-live="polite" role="alertdialog" class="toast-message" attr.aria-label="{{'I18N_EXPLORATION_STARTING_FROM_BEGINNING' | translate}}">
+      <div aria-live="polite" role="alertdialog" class="toast-message e2e-test-lesson-completion-message" attr.aria-label="{{'I18N_EXPLORATION_STARTING_FROM_BEGINNING' | translate}}">
         {{ 'I18N_EXPLORATION_STARTING_FROM_BEGINNING' | translate }}
       </div>
     </div>

diff --git a/core/tests/webdriverio_desktop/coreEditorAndPlayerFeatures.js b/core/tests/webdriverio_desktop/coreEditorAndPlayerFeatures.js
@@ -79,7 +79,7 @@ describe('Enable correctness feedback and set correctness', function() {
   });
 
   it('should allow selecting correct feedback from the response editor ' +
-     'after the interaction is created', async function() {
+    'after the interaction is created', async function() {
     await workflow.createExploration(true);
     await explorationEditorPage.navigateToSettingsTab();
     await explorationEditorSettingsTab.setTitle(explorationTitle);
@@ -120,7 +120,7 @@ describe('Enable correctness feedback and set correctness', function() {
   });
 
   it('should allow selecting correct feedback from the response editor ' +
-     'during set the interaction', async function() {
+    'during set the interaction', async function() {
     await workflow.createExploration(false);
     await explorationEditorPage.navigateToSettingsTab();
     await explorationEditorSettingsTab.setTitle(explorationTitle);
@@ -411,8 +411,45 @@ describe('Core exploration functionality', function() {
       await explorationPlayerPage.expectExplorationToBeOver();
     });
 
+  it('should not show self-loop warning when navigating between ' +
+    'different states', async function() {
+    // Setup initial state.
+    await explorationEditorMainTab.setContent(
+      await forms.toRichText('some content'));
+    await explorationEditorMainTab.setInteraction('NumericInput');
+    await explorationEditorMainTab.addResponse(
+      'NumericInput', async function(richTextEditor) {
+        await richTextEditor.appendBoldText('correct');
+      }, 'final card', true, 'IsInclusivelyBetween', -1, 3);
+    var firstResponseEditor = (
+      await explorationEditorMainTab.getResponseEditor(0));
+    await firstResponseEditor.markAsCorrect();
+    var defaultResponseEditor = (
+      await explorationEditorMainTab.getResponseEditor('default'));
+    await defaultResponseEditor.setFeedback(
+      await forms.toRichText('try again'));
+    await defaultResponseEditor.setDestination(null, false, null);
+
+    // Setup terminating state.
+    await explorationEditorMainTab.moveToState('final card');
+    await explorationEditorMainTab.setInteraction('EndExploration');
+    await explorationEditorPage.saveChanges();
+
+    // Test the flow.
+    await explorationEditorMainTab.moveToState('Introduction');
+    await explorationEditorPage.navigateToPreviewTab();
+    await explorationEditorPage.waitForPreviewTabToLoad();
+    await explorationPlayerPage.submitAnswer('NumericInput', 1);
+    await explorationPlayerPage.clickThroughToNextCard();
+    await explorationPlayerPage.waitForLessonCompletionMessageToDisappear();
+    await explorationEditorPage.navigateToMainTab();
+    await explorationEditorMainTab.moveToState('Introduction');
+    await (
+      explorationEditorMainTab.checkSelfLoopWarningIsNotShown());
+  });
+
   it('should skip the customization modal for interactions having no ' +
-      'customization options', async function() {
+    'customization options', async function() {
     await explorationEditorMainTab.setContent(
       await forms.toRichText('some content'));
 
@@ -427,7 +464,7 @@ describe('Core exploration functionality', function() {
   });
 
   it('should open appropriate modal on re-clicking an interaction to ' +
-     'customize it', async function() {
+    'customize it', async function() {
     await explorationEditorMainTab.setContent(
       await forms.toRichText('some content'));
 
@@ -465,7 +502,7 @@ describe('Core exploration functionality', function() {
     await action.click('Use Image Button', useImageBtn);
     var svgElem = $('.e2e-test-svg');
     await svgElem.moveTo(0, 0);
-    await svgElem.dragAndDrop({x: 1, y: 1});
+    await svgElem.dragAndDrop({ x: 1, y: 1 });
     await action.click('Save Interaction Button', saveInteractionBtn);
     var closeAddResponseButton = $('.e2e-test-close-add-response-modal');
     await action.click('Close Add Response Button', closeAddResponseButton);
@@ -559,7 +596,7 @@ describe('Core exploration functionality', function() {
     });
 
   it('should be able to create new category which is not' +
-  ' in the dropdown menu', async function() {
+    ' in the dropdown menu', async function() {
     await explorationEditorPage.navigateToSettingsTab();
 
     await explorationEditorSettingsTab.expectCategoryToBe('');

diff --git a/core/tests/webdriverio_utils/ExplorationEditorMainTab.js b/core/tests/webdriverio_utils/ExplorationEditorMainTab.js
@@ -420,6 +420,12 @@ var ExplorationEditorMainTab = function() {
     };
   };
 
+  this.checkSelfLoopWarningIsNotShown = async function() {
+    var responseSelfLoopWarning = $('.e2e-test-response-self-loop-warning');
+    await waitFor.invisibilityOf(
+      responseSelfLoopWarning, 'inaccurate self-loop warning');
+  };
+
   this.expectCannotAddResponse = async function() {
     expect(await addResponseButton.isExisting()).toBeFalsy();
   };

diff --git a/core/tests/webdriverio_utils/ExplorationPlayerPage.js b/core/tests/webdriverio_utils/ExplorationPlayerPage.js
@@ -220,6 +220,13 @@ var ExplorationPlayerPage = function() {
     ).toMatch(richTextInstructions);
   };
 
+  this.waitForLessonCompletionMessageToDisappear = async function() {
+    var lessonCompletionMessage = $('.e2e-test-lesson-completion-message');
+    await waitFor.invisibilityOf(
+      lessonCompletionMessage,
+      'Lesson Completion Message took long to disappear');
+  };
+
   this.expectExplorationToBeOver = async function() {
     await waitFor.visibilityOf(
       conversationContent, 'Conversation not visible');