Skip to content

Commit c25590f

Browse files
RaymondLuong3RaymondLuong3
authored
SF-3322 Allow selecting book and chapter in community checking while filtering (#3223)
1 parent 67cc8db commit c25590f

File tree

4 files changed

+114
-38
lines changed

4 files changed

+114
-38
lines changed

‎src/SIL.XForge.Scripture/ClientApp/src/app/checking/checking/checking-questions/checking-questions.component.ts

Lines changed: 16 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -176,6 +176,9 @@ export class CheckingQuestionsComponent implements OnInit, OnChanges {
176176
this.haveQuestionsLoaded = true;
177177
this.changed.emit({ questionDoc: this.activeQuestionDoc, actionSource: { isQuestionListChange: true } });
178178
this.changeDetector.markForCheck();
179+
} else if (changes.routeBookChapter != null) {
180+
// If the route book/chapter changes, activate the first question on the new chapter
181+
this.activateStoredQuestion();
179182
}
180183
}
181184

@@ -294,18 +297,25 @@ export class CheckingQuestionsComponent implements OnInit, OnChanges {
294297
}
295298
}
296299
} else {
297-
questionToActivate = this.activeQuestionDoc;
300+
// If there is an active question, check if it matches the route book/chapter
301+
const useActiveQuestion: boolean =
302+
this.routeBookChapter == null ||
303+
(this.activeQuestionDoc?.data != null &&
304+
bookChapterMatchesVerseRef(this.routeBookChapter, this.activeQuestionDoc.data.verseRef));
305+
if (useActiveQuestion) {
306+
questionToActivate = this.activeQuestionDoc;
307+
}
298308
}
299309

300310
// No stored question, so use first question within route book/chapter if available.
301-
// Otherwise use first question.
302-
questionToActivate ??=
303-
this.questionDocs.find(
304-
qd => this.routeBookChapter == null || bookChapterMatchesVerseRef(this.routeBookChapter, qd.data!.verseRef)
305-
) ?? this.questionDocs[0];
311+
questionToActivate ??= this.questionDocs.find(
312+
qd => this.routeBookChapter == null || bookChapterMatchesVerseRef(this.routeBookChapter, qd.data!.verseRef)
313+
);
306314

307315
if (questionToActivate != null) {
308316
this.activateQuestion(questionToActivate, actionSource);
317+
} else {
318+
this.activeQuestionDoc = undefined;
309319
}
310320
}
311321

‎src/SIL.XForge.Scripture/ClientApp/src/app/checking/checking/checking.component.html

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -71,9 +71,7 @@ <h2 matSubheader>{{ t("filter_questions") }}</h2>
7171
[book]="book"
7272
[chapters]="chapters"
7373
[chapter]="chapter"
74-
[bookSelectDisabled]="activeQuestionScope === 'all'"
75-
[chapterSelectDisabled]="activeQuestionScope !== 'chapter'"
76-
[prevNextHidden]="activeQuestionScope !== 'chapter' || isScreenSmall"
74+
[prevNextHidden]="isScreenSmall"
7775
(bookChange)="onBookSelect($event)"
7876
(chapterChange)="onChapterSelect($event)"
7977
>

‎src/SIL.XForge.Scripture/ClientApp/src/app/checking/checking/checking.component.spec.ts

Lines changed: 90 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -374,6 +374,7 @@ describe('CheckingComponent', () => {
374374
env.setBookChapter('JHN', 2);
375375
expect(env.getQuestionText(question)).toBe('John 2');
376376
expect(await env.getCurrentBookAndChapter()).toBe('John 2');
377+
env.waitForQuestionTimersToComplete();
377378
}));
378379

379380
it('start question respects route book/chapter', fakeAsync(async () => {
@@ -391,6 +392,41 @@ describe('CheckingComponent', () => {
391392
discardPeriodicTasks();
392393
}));
393394

395+
it('updates active question when route book/chapter changes', fakeAsync(() => {
396+
const env = new TestEnvironment({
397+
user: CHECKER_USER,
398+
projectBookRoute: 'JHN',
399+
projectChapterRoute: 1,
400+
questionScope: 'book'
401+
});
402+
expect(env.component.questionsList!.activeQuestionDoc!.data!.dataId).toBe('q5Id');
403+
env.setBookChapter('JHN', 2);
404+
env.fixture.detectChanges();
405+
expect(env.component.questionsList!.activeQuestionDoc!.data!.dataId).toBe('q15Id');
406+
flush();
407+
discardPeriodicTasks();
408+
}));
409+
410+
it('clears active question when route book/chapter changes to chapter without questions', fakeAsync(() => {
411+
const env = new TestEnvironment({
412+
user: CHECKER_USER,
413+
projectBookRoute: 'MAT',
414+
projectChapterRoute: 1,
415+
questionScope: 'book'
416+
});
417+
expect(env.component.questionsList!.activeQuestionDoc!.data!.dataId).toBe('q16Id');
418+
env.setBookChapter('MAT', 2);
419+
env.fixture.detectChanges();
420+
expect(env.component.questionsList!.activeQuestionDoc).toBe(undefined);
421+
expect(env.component.chapter).toEqual(2);
422+
env.setBookChapter('MAT', 3);
423+
env.fixture.detectChanges();
424+
expect(env.component.questionsList!.activeQuestionDoc).toBe(undefined);
425+
expect(env.component.chapter).toEqual(3);
426+
flush();
427+
discardPeriodicTasks();
428+
}));
429+
394430
it('can select a question', fakeAsync(() => {
395431
const env = new TestEnvironment({ user: CHECKER_USER });
396432
const question = env.selectQuestion(1);
@@ -569,8 +605,7 @@ describe('CheckingComponent', () => {
569605
expect(env.isSegmentHighlighted(1, 5)).toBe(true);
570606
expect(env.segmentHasQuestion(1, 5)).toBe(true);
571607
expect(env.component.questionVerseRefs.some(verseRef => verseRef.equals(new VerseRef('JHN 1:5')))).toBe(true);
572-
tick();
573-
flush();
608+
env.waitForQuestionTimersToComplete();
574609
}));
575610

576611
it('records audio for question when button clicked', fakeAsync(() => {
@@ -851,6 +886,38 @@ describe('CheckingComponent', () => {
851886
env.waitForQuestionTimersToComplete();
852887
}));
853888

889+
it('should not reset scope after a user changes chapter', fakeAsync(() => {
890+
const env = new TestEnvironment({
891+
user: ADMIN_USER,
892+
projectBookRoute: 'JHN',
893+
projectChapterRoute: 1,
894+
questionScope: 'chapter'
895+
});
896+
expect(env.questions.length).toEqual(14);
897+
env.component.onChapterSelect(2);
898+
env.setBookChapter('JHN', 2);
899+
tick();
900+
env.fixture.detectChanges();
901+
expect(env.component.activeQuestionScope).toEqual('chapter');
902+
env.waitForQuestionTimersToComplete();
903+
}));
904+
905+
it('should not reset scope after a user changes book', fakeAsync(() => {
906+
const env = new TestEnvironment({
907+
user: ADMIN_USER,
908+
projectBookRoute: 'JHN',
909+
projectChapterRoute: 1,
910+
questionScope: 'book'
911+
});
912+
expect(env.questions.length).toEqual(15);
913+
env.component.onBookSelect(40);
914+
env.setBookChapter('MAT', 1);
915+
tick();
916+
env.fixture.detectChanges();
917+
expect(env.component.activeQuestionScope).toEqual('book');
918+
env.waitForQuestionTimersToComplete();
919+
}));
920+
854921
it('can narrow questions scope', fakeAsync(() => {
855922
const env = new TestEnvironment({
856923
user: ADMIN_USER,
@@ -921,7 +988,8 @@ describe('CheckingComponent', () => {
921988
env.waitForQuestionTimersToComplete();
922989

923990
expect(spyUpdateQuestionRefs).toHaveBeenCalledTimes(1);
924-
expect(spyRefreshSummary).toHaveBeenCalledTimes(1);
991+
// Called at least once or more depending on if another question replaces the archived question
992+
expect(spyRefreshSummary).toHaveBeenCalled();
925993
}));
926994
});
927995

@@ -1913,8 +1981,9 @@ describe('CheckingComponent', () => {
19131981
const env = new TestEnvironment({ user: CHECKER_USER });
19141982
const questionDoc = spy(env.getQuestionDoc('q6Id'));
19151983
env.selectQuestion(6);
1984+
verify(questionDoc!.updateAnswerFileCache()).times(1);
19161985
env.simulateRemoteEditAnswer(0, 'Question 6 edited answer');
1917-
verify(questionDoc!.updateAnswerFileCache()).times(3);
1986+
verify(questionDoc!.updateAnswerFileCache()).times(2);
19181987
expect().nothing();
19191988
tick();
19201989
flush();
@@ -1924,8 +1993,9 @@ describe('CheckingComponent', () => {
19241993
const env = new TestEnvironment({ user: ADMIN_USER });
19251994
const questionDoc = spy(env.getQuestionDoc('q6Id'));
19261995
env.selectQuestion(6);
1996+
verify(questionDoc!.updateAnswerFileCache()).times(1);
19271997
env.simulateRemoteDeleteAnswer('q6Id', 0);
1928-
verify(questionDoc!.updateAnswerFileCache()).times(3);
1998+
verify(questionDoc!.updateAnswerFileCache()).times(2);
19291999
expect().nothing();
19302000
tick();
19312001
flush();
@@ -2760,7 +2830,11 @@ class TestEnvironment {
27602830
{
27612831
bookNum: 40,
27622832
hasSource: false,
2763-
chapters: [{ number: 1, lastVerse: 28, isValid: true, permissions: {} }],
2833+
chapters: [
2834+
{ number: 1, lastVerse: 28, isValid: true, permissions: {} },
2835+
{ number: 2, lastVerse: 23, isValid: true, permissions: {} },
2836+
{ number: 3, lastVerse: 26, isValid: true, permissions: {} }
2837+
],
27642838
permissions: {}
27652839
}
27662840
],
@@ -3258,6 +3332,16 @@ class TestEnvironment {
32583332
id: getTextDocId(projectId, 40, 1),
32593333
data: this.createTextDataForChapter(1),
32603334
type: RichText.type.name
3335+
},
3336+
{
3337+
id: getTextDocId(projectId, 40, 2),
3338+
data: this.createTextDataForChapter(2),
3339+
type: RichText.type.name
3340+
},
3341+
{
3342+
id: getTextDocId(projectId, 40, 3),
3343+
data: this.createTextDataForChapter(3),
3344+
type: RichText.type.name
32613345
}
32623346
]);
32633347
when(mockedProjectService.getText(anything())).thenCall(id =>

‎src/SIL.XForge.Scripture/ClientApp/src/app/checking/checking/checking.component.ts

Lines changed: 7 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -580,7 +580,6 @@ export class CheckingComponent extends DataLoadingComponent implements OnInit, A
580580
};
581581
} else {
582582
const suggestedBookChapter: BookChapter = await this.getSuggestedNavBookChapter(routeBookNum);
583-
584583
this.navigateBookChapter(
585584
routeProjectId,
586585
routeScope!,
@@ -607,7 +606,6 @@ export class CheckingComponent extends DataLoadingComponent implements OnInit, A
607606
(routeScope === 'chapter' && (routeChapter == null ? undefined : parseInt(routeChapter)) !== prevChapterNum)
608607
) {
609608
this.cleanup();
610-
611609
this.questionsQuery = await this.checkingQuestionsService.queryQuestions(
612610
routeProjectId,
613611
{
@@ -648,9 +646,6 @@ export class CheckingComponent extends DataLoadingComponent implements OnInit, A
648646

649647
if (this.onlineStatusService.isOnline) {
650648
qd.updateFileCache();
651-
if (isActiveQuestionDoc) {
652-
qd.updateAnswerFileCache();
653-
}
654649
}
655650

656651
this.updateAdjacentQuestions(this.questionsList!.activeQuestionDoc!);
@@ -695,9 +690,6 @@ export class CheckingComponent extends DataLoadingComponent implements OnInit, A
695690
this.updateVisibleQuestions();
696691
});
697692
} else {
698-
// Visible questions didn't change, but active question must update on route change
699-
this.questionsList?.activateStoredQuestion();
700-
701693
// Ensure refs updated if book changed, but no new questions query (scope is 'all')
702694
if (routeBookNum !== prevBookNum) {
703695
this.updateQuestionRefs();
@@ -733,7 +725,6 @@ export class CheckingComponent extends DataLoadingComponent implements OnInit, A
733725
.pipe(quietTakeUntilDestroyed(this.destroyRef))
734726
.subscribe((state: BreakpointState) => {
735727
this.calculateScriptureSliderPosition();
736-
737728
// `questionsPanel` is undefined until ngAfterViewInit, but setting `isQuestionListPermanent`
738729
// here causes `ExpressionChangedAfterItHasBeenCheckedError`, so wrap in setTimeout
739730
setTimeout(() => {
@@ -746,7 +737,8 @@ export class CheckingComponent extends DataLoadingComponent implements OnInit, A
746737
.observe(this.mediaBreakpointService.width('<', Breakpoint.MD))
747738
.pipe(quietTakeUntilDestroyed(this.destroyRef))
748739
.subscribe((state: BreakpointState) => {
749-
this.isScreenSmall = state.matches;
740+
// setting isScreenSmall causes `ExpressionChangedAfterItHasBeenCheckedError`, so wrap in setTimeout
741+
setTimeout(() => (this.isScreenSmall = state.matches));
750742
});
751743
}
752744

@@ -940,9 +932,7 @@ export class CheckingComponent extends DataLoadingComponent implements OnInit, A
940932
}
941933

942934
// Ensure navigation is set to book/chapter of selected question
943-
if (this.navigateQuestionChapter(questionDoc)) {
944-
return;
945-
}
935+
this.navigateQuestionChapter(questionDoc);
946936
}
947937
}
948938

@@ -1087,23 +1077,17 @@ export class CheckingComponent extends DataLoadingComponent implements OnInit, A
10871077
}
10881078

10891079
/**
1090-
* Navigate to book/chapter of specified question if necessary and select question.
1080+
* Activate a question which should automatically navigate to the book/chapter
10911081
*/
10921082
activateQuestion(questionDoc: QuestionDoc | undefined, { withFilterReset = false } = {}): void {
10931083
if (questionDoc == null) {
10941084
return;
10951085
}
10961086

1097-
if (!this.navigateQuestionChapter(questionDoc)) {
1098-
if (withFilterReset) {
1099-
this.resetFilter();
1100-
}
1101-
1102-
this.questionsList?.activateQuestion(questionDoc);
1103-
} else if (withFilterReset) {
1104-
// Reset filter, but don't update visible questions yet if navigating
1105-
this.activeQuestionFilter = QuestionFilter.None;
1087+
if (withFilterReset) {
1088+
this.resetFilter();
11061089
}
1090+
this.questionsList?.activateQuestion(questionDoc);
11071091
}
11081092

11091093
/**

0 commit comments

Comments
 (0)