Fix part of oppia#18373: make default destIfStuck to null and update …

…it properly (oppia#19039) * fix Part oppia#18373. do not hide destIfReallyStuck when it has data * add tests, make the approach clearer * fix lint * remove redundant newline --------- Co-authored-by: Shivkant Chauhan <[email protected]>
sanketn2k04 · Nov 4, 2023 · a10ce05 · a10ce05
1 parent f8b77a2
commit a10ce05
Show file tree

Hide file tree

Showing 8 changed files with 65 additions and 76 deletions.
diff --git a/core/templates/components/state-directives/outcome-editor/outcome-editor.component.html b/core/templates/components/state-directives/outcome-editor/outcome-editor.component.html
@@ -181,7 +181,7 @@
   </div>
 </div>
 
-<div *ngIf="!destinationIfStuckEditorIsOpen && !isInQuestionMode() && outcome && !outcome.labelledAsCorrect"
+<div *ngIf="!destinationIfStuckEditorIsOpen && !isInQuestionMode() && outcome && shouldShowDestIfReallyStuck()"
      class="h-100"
      title="{{ isEditable ? 'Change the if stuck destination card' : '' }}">
   <div class="oppia-readonly-rule-tile protractor-test-edit-outcome-dest-if-stuck-button"
@@ -197,10 +197,10 @@
       <div>
         <strong *ngIf="displayFeedback">(Optional) If the learner is really stuck, direct them to</strong>
         <strong *ngIf="!displayFeedback">(Optional) Oppia directs the stuck learner to...</strong>
-        <span *ngIf="outcome && !isSelfLoopDestStuck(outcome)" class="position-relative">
+        <span *ngIf="outcome && outcome.destIfReallyStuck !== null" class="position-relative">
           {{ outcome.destIfReallyStuck }}
         </span>
-        <span *ngIf="isSelfLoopDestStuck(outcome)" class="position-relative"></span>
+        <span *ngIf="!outcome || outcome.destIfReallyStuck === null" class="position-relative"></span>
       </div>
     </div>
   </div>

diff --git a/core/templates/components/state-directives/outcome-editor/outcome-editor.component.spec.ts b/core/templates/components/state-directives/outcome-editor/outcome-editor.component.spec.ts
@@ -377,35 +377,6 @@ describe('Outcome Editor Component', () => {
     expect(component.invalidStateAfterDestinationSave()).toBeTrue();
   });
 
-  it('should check if a destination for stuck learner forms a loop', () => {
-    let outcome = new Outcome(
-      'Me Llamo',
-      'Hola',
-      new SubtitledHtml('<p> Previous HTML string </p>', 'Id'),
-      true,
-      [],
-      null,
-      null,
-    );
-    component.outcome = outcome;
-    spyOn(stateEditorService, 'getActiveStateName')
-      .and.returnValue('Hola');
-
-    expect(component.isSelfLoopDestStuck(outcome)).toBeTrue();
-
-    outcome = new Outcome(
-      'Ma Llamo',
-      'Me Llmao',
-      new SubtitledHtml('<p> Previous HTML string </p>', 'Id'),
-      true,
-      [],
-      null,
-      null,
-    );
-    component.outcome = outcome;
-    expect(component.isSelfLoopDestStuck(outcome)).toBeFalse();
-  });
-
   it('should open feedback editor if it is editable', () => {
     component.feedbackEditorIsOpen = false;
     component.isEditable = true;
@@ -478,7 +449,7 @@ describe('Outcome Editor Component', () => {
     );
     component.openDestinationIfStuckEditor();
     expect(component.destinationIfStuckEditorIsOpen).toBeTrue();
-    expect(component.outcome.destIfReallyStuck).toBe('first');
+    expect(component.outcome.destIfReallyStuck).toBeNull();
   });
 
   it('should save correctness label when it is changed', () => {

diff --git a/core/templates/components/state-directives/outcome-editor/outcome-editor.component.ts b/core/templates/components/state-directives/outcome-editor/outcome-editor.component.ts
@@ -81,6 +81,11 @@ export class OutcomeEditorComponent implements OnInit {
     return this.stateEditorService.isInQuestionMode();
   }
 
+  shouldShowDestIfReallyStuck(): boolean {
+    return !this.savedOutcome.labelledAsCorrect ||
+      this.savedOutcome.destIfReallyStuck !== null;
+  }
+
   isFeedbackLengthExceeded(): boolean {
     // TODO(#13764): Edit this check after appropriate limits are found.
     return (this.outcome.feedback._html.length > 10000);
@@ -133,13 +138,6 @@ export class OutcomeEditorComponent implements OnInit {
       outcome.dest === this.stateEditorService.getActiveStateName());
   }
 
-  isSelfLoopDestStuck(outcome: Outcome): boolean {
-    return Boolean (
-      outcome &&
-      outcome.destIfReallyStuck === (
-        this.stateEditorService.getActiveStateName()));
-  }
-
   getCurrentInteractionId(): string {
     return this.stateInteractionIdService.savedMemento;
   }
@@ -197,10 +195,6 @@ export class OutcomeEditorComponent implements OnInit {
   openDestinationIfStuckEditor(): void {
     if (this.isEditable) {
       this.destinationIfStuckEditorIsOpen = true;
-      let activeStateName = this.stateEditorService.getActiveStateName();
-      if (!this.savedOutcome.destIfReallyStuck) {
-        this.outcome.destIfReallyStuck = activeStateName;
-      }
     }
   }
 

diff --git a/...onents/state-directives/outcome-editor/outcome-if-stuck-destination-editor.component.html b/...onents/state-directives/outcome-editor/outcome-if-stuck-destination-editor.component.html
@@ -12,7 +12,7 @@
       <select class="form-control oppia-form-control"
               [(ngModel)]="outcome.destIfReallyStuck"
               (ngModelChange)="onDestIfStuckSelectorChange()">
-        <option *ngFor="let choice of destinationChoices" [value]="choice.id">{{ choice.text }}</option>
+        <option *ngFor="let choice of destinationChoices" [ngValue]="choice.id">{{ choice.text }}</option>
       </select>
     </span>
   </div>

diff --git a/...nts/state-directives/outcome-editor/outcome-if-stuck-destination-editor.component.spec.ts b/...nts/state-directives/outcome-editor/outcome-if-stuck-destination-editor.component.spec.ts
@@ -20,7 +20,7 @@ import { HttpClientTestingModule } from '@angular/common/http/testing';
 import { EventEmitter, NO_ERRORS_SCHEMA } from '@angular/core';
 import { ComponentFixture, fakeAsync, TestBed, tick, waitForAsync } from '@angular/core/testing';
 import { FormsModule } from '@angular/forms';
-import { StateGraphLayoutService } from 'components/graph-services/graph-layout.service';
+import { NodeDataDict, StateGraphLayoutService } from 'components/graph-services/graph-layout.service';
 import { StateEditorService } from 'components/state-editor/state-editor-properties-services/state-editor.service';
 import { Outcome } from 'domain/exploration/OutcomeObjectFactory';
 import { SubtitledHtml } from 'domain/exploration/subtitled-html.model';
@@ -39,6 +39,7 @@ describe('Outcome Destination If Stuck Editor', () => {
   let stateGraphLayoutService: StateGraphLayoutService;
   let focusManagerService: FocusManagerService;
   let PLACEHOLDER_OUTCOME_DEST_IF_STUCK = '/';
+  let DEFAULT_LAYOUT: NodeDataDict;
 
   beforeEach(waitForAsync(() => {
     TestBed.configureTestingModule({
@@ -68,14 +69,7 @@ describe('Outcome Destination If Stuck Editor', () => {
     focusManagerService = TestBed.inject(FocusManagerService);
     stateEditorService = TestBed.inject(StateEditorService);
     stateGraphLayoutService = TestBed.inject(StateGraphLayoutService);
-  });
-
-  afterEach(() => {
-    component.ngOnDestroy();
-  });
-
-  it('should set component properties on initialization', fakeAsync(() => {
-    let computedLayout = stateGraphLayoutService.computeLayout(
+    DEFAULT_LAYOUT = stateGraphLayoutService.computeLayout(
       {
         Introduction: 'Introduction',
         State1: 'State1',
@@ -94,6 +88,14 @@ describe('Outcome Destination If Stuck Editor', () => {
           connectsDestIfStuck: false
         }
       ], 'Introduction', ['End']);
+  });
+
+  afterEach(() => {
+    component.ngOnDestroy();
+  });
+
+  it('should set component properties on initialization', fakeAsync(() => {
+    let computedLayout = DEFAULT_LAYOUT;
     spyOn(stateEditorService, 'getStateNames')
       .and.returnValue(['Introduction', 'State1', 'NewState', 'End']);
     spyOn(stateGraphLayoutService, 'getLastComputedArrangement')
@@ -154,25 +156,7 @@ describe('Outcome Destination If Stuck Editor', () => {
 
   it('should update option names when state name is changed', fakeAsync(() => {
     let onStateNamesChangedEmitter = new EventEmitter();
-    let computedLayout = stateGraphLayoutService.computeLayout(
-      {
-        Introduction: 'Introduction',
-        State1: 'State1',
-        End: 'End'
-      }, [
-        {
-          source: 'Introduction',
-          target: 'State1',
-          linkProperty: '',
-          connectsDestIfStuck: false
-        },
-        {
-          source: 'State1',
-          target: 'End',
-          linkProperty: '',
-          connectsDestIfStuck: false
-        }
-      ], 'Introduction', ['End']);
+    let computedLayout = DEFAULT_LAYOUT;
     spyOnProperty(stateEditorService, 'onStateNamesChanged')
       .and.returnValue(onStateNamesChangedEmitter);
     spyOn(stateEditorService, 'getStateNames')
@@ -296,4 +280,31 @@ describe('Outcome Destination If Stuck Editor', () => {
 
     expect(component.getChanges.emit).toHaveBeenCalled();
   });
+
+  it('should not show active state', fakeAsync(() => {
+    let computedLayout = DEFAULT_LAYOUT;
+    spyOn(stateGraphLayoutService, 'getLastComputedArrangement')
+      .and.returnValue(computedLayout);
+    spyOn(stateEditorService, 'getActiveStateName')
+      .and.returnValue('Introduction');
+    spyOn(stateEditorService, 'getStateNames')
+      .and.returnValues(['Introduction', 'State1', 'End']);
+
+    component.ngOnInit();
+    tick(10);
+
+    expect(component.destinationChoices).toEqual([{
+      id: null,
+      text: 'None'
+    }, {
+      id: 'State1',
+      text: 'State1'
+    }, {
+      id: 'End',
+      text: 'End'
+    }, {
+      id: '/',
+      text: 'A New Card Called...'
+    }]);
+  }));
 });
diff --git a/...mponents/state-directives/outcome-editor/outcome-if-stuck-destination-editor.component.ts b/...mponents/state-directives/outcome-editor/outcome-if-stuck-destination-editor.component.ts
@@ -109,7 +109,7 @@ export class OutcomeIfStuckDestinationEditorComponent implements OnInit {
       // represent all states, as well as an option to create a
       // new state.
       this.destinationChoices = [{
-        id: this.currentStateName,
+        id: null,
         text: 'None'
       }];
 

diff --git a/core/templates/pages/exploration-editor-page/editor-tab/services/responses.service.spec.ts b/core/templates/pages/exploration-editor-page/editor-tab/services/responses.service.spec.ts
@@ -297,7 +297,6 @@ describe('Responses Service', () => {
       rules: [new Rule('Contains', { x: 'correct'}, {})],
       outcome: {
         dest: 'State',
-        destIfReallyStuck: null,
         feedback: new SubtitledHtml('', 'This is a new feedback text'),
         refresherExplorationId: 'test',
         missingPrerequisiteSkillId: 'test_skill_id',
@@ -311,7 +310,7 @@ describe('Responses Service', () => {
       taggedSkillMisconceptionId: '',
       feedback: new SubtitledHtml('', 'This is a new feedback text'),
       dest: 'State',
-      dest_if_really_stuck: null,
+      destIfReallyStuck: 'destIfReallyStuck',
       refresherExplorationId: 'test',
       missingPrerequisiteSkillId: 'test_skill_id',
       labelledAsCorrect: true,
@@ -335,6 +334,8 @@ describe('Responses Service', () => {
     expectedAnswerGroup[0].outcome.feedback =
       updatedAnswerGroup.outcome.feedback;
     expectedAnswerGroup[0].outcome.dest = updatedAnswerGroup.outcome.dest;
+    expectedAnswerGroup[0].outcome.destIfReallyStuck =
+      updatedAnswerGroup.destIfReallyStuck;
     expectedAnswerGroup[0].outcome.refresherExplorationId =
       updatedAnswerGroup.outcome.refresherExplorationId;
     expectedAnswerGroup[0].outcome.missingPrerequisiteSkillId =

diff --git a/core/templates/pages/exploration-editor-page/editor-tab/services/responses.service.ts b/core/templates/pages/exploration-editor-page/editor-tab/services/responses.service.ts
@@ -63,12 +63,17 @@ interface UpdateAnswerGroupFeedback {
   feedback: SubtitledHtml;
 }
 
+interface DestIfReallyStuck {
+  destIfReallyStuck: string | null;
+}
+
 interface UpdateRule {
   rules: Rule[];
 }
 
 export type UpdateActiveAnswerGroup = (
   AnswerGroup |
+  DestIfReallyStuck |
   UpdateAnswerGroupFeedback |
   UpdateAnswerGroupCorrectnessLabel |
   UpdateActiveAnswerGroupDest |
@@ -225,6 +230,13 @@ export class ResponsesService {
         answerGroup.outcome.labelledAsCorrect = (
           labelledAsCorrectUpdates.labelledAsCorrect);
       }
+      if (updates.hasOwnProperty('destIfReallyStuck')) {
+        let destIfReallyStuckUpdates = updates as {
+          destIfReallyStuck: string | null;
+        };
+        answerGroup.outcome.destIfReallyStuck = (
+          destIfReallyStuckUpdates.destIfReallyStuck);
+      }
       if (updates.hasOwnProperty('trainingData')) {
         let trainingDataUpdates = updates as AnswerGroup;
         answerGroup.trainingData = trainingDataUpdates.trainingData;