Skip to content

Commit 007cedc

Browse files
pmachapmanpmachapman
committed
Code review updates
1 parent b2ef296 commit 007cedc

13 files changed

+145
-152
lines changed

‎src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-generation.service.spec.ts

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -168,7 +168,7 @@ describe('DraftGenerationService', () => {
168168
it('should return undefined for a 401 error', fakeAsync(() => {
169169
// SUT
170170
service.getBuildHistory(projectId).subscribe(result => {
171-
expect(result).toEqual(undefined);
171+
expect(result).toBeUndefined();
172172
verify(mockNoticeService.showError(anything())).once();
173173
});
174174
tick();
@@ -185,7 +185,7 @@ describe('DraftGenerationService', () => {
185185
it('should return undefined for a 404 error', fakeAsync(() => {
186186
// SUT
187187
service.getBuildHistory(projectId).subscribe(result => {
188-
expect(result).toEqual(undefined);
188+
expect(result).toBeUndefined();
189189
verify(mockNoticeService.showError(anything())).never();
190190
});
191191
tick();
@@ -617,7 +617,7 @@ describe('DraftGenerationService', () => {
617617

618618
// SUT
619619
service.getGeneratedDraftHistory(projectId, book, chapter).subscribe(result => {
620-
expect(result).toEqual(undefined);
620+
expect(result).toBeUndefined();
621621
verify(mockNoticeService.showError(anything())).once();
622622
});
623623
tick();
@@ -637,7 +637,7 @@ describe('DraftGenerationService', () => {
637637

638638
// SUT
639639
service.getGeneratedDraftHistory(projectId, book, chapter).subscribe(result => {
640-
expect(result).toEqual(undefined);
640+
expect(result).toBeUndefined();
641641
verify(mockNoticeService.showError(anything())).never();
642642
});
643643
tick();
@@ -658,7 +658,7 @@ describe('DraftGenerationService', () => {
658658

659659
// SUT
660660
service.getGeneratedDraftHistory(projectId, book, chapter).subscribe(result => {
661-
expect(result).toEqual(undefined);
661+
expect(result).toBeUndefined();
662662
});
663663
tick();
664664
}));

‎src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-generation.service.ts

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -314,12 +314,13 @@ export class DraftGenerationService {
314314
const usfmFiles: Promise<void>[] = [];
315315

316316
// Build the list of book numbers, first checking the build, then the project document if that is null
317-
const books: number[] =
317+
const books = new Set<number>(
318318
lastCompletedBuild?.additionalInfo?.translationScriptureRanges?.flatMap(range =>
319319
booksFromScriptureRange(range.scriptureRange)
320-
) ?? projectDoc.data.texts.filter(text => text.chapters.some(c => c.hasDraft)).map(text => text.bookNum);
320+
) ?? projectDoc.data.texts.filter(text => text.chapters.some(c => c.hasDraft)).map(text => text.bookNum)
321+
);
321322

322-
const zipProgress: DraftZipProgress = { current: 0, total: books.length };
323+
const zipProgress: DraftZipProgress = { current: 0, total: books.size };
323324
observer.next(zipProgress);
324325

325326
// Get the date the draft was generated and written to Scripture Forge

‎src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-handling.service.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -162,7 +162,7 @@ export class DraftHandlingService {
162162
// If the corpus does not support USFM, use the legacy format.
163163
// The legacy format does not support a timestamp
164164
if (err.status === 405 && timestamp == null) {
165-
return this.getDraft(textDocId, { isDraftLegacy: true, timestamp });
165+
return this.getDraft(textDocId, { isDraftLegacy: true, timestamp: undefined });
166166
}
167167

168168
return throwError(() => err);

‎src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-history-list/draft-history-entry/_draft-history-entry-theme.scss

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3,9 +3,9 @@
33
@mixin color($theme) {
44
$is-dark: mat.get-theme-type($theme) == dark;
55

6-
--draft-history-entry-heading-background-color: #{mat.get-theme-color($theme, neutral, if($is-dark, 20, 95))};
7-
--draft-history-entry-heading-background-hover-color: #{mat.get-theme-color($theme, neutral, if($is-dark, 30, 90))};
8-
--draft-history-entry-details-background-color: #{mat.get-theme-color($theme, neutral, if($is-dark, 10, 98))};
6+
--draft-history-entry-red-color: #{if($is-dark, red, darkRed)};
7+
--draft-history-entry-green-color: #{if($is-dark, lightGreen, darkGreen)};
8+
--draft-history-entry-grey-color: lightGrey;
99
}
1010

1111
@mixin theme($theme) {

‎src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-history-list/draft-history-entry/draft-history-entry.component.html

Lines changed: 44 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -1,44 +1,54 @@
11
<ng-container *transloco="let t; read: 'draft_history_entry'">
22
@if (entry != null) {
3-
<div class="draft-entry-heading" (click)="headerClicked()" [class.expandable]="!forceDetailsOpen">
4-
<div class="entry-description">
5-
<span class="drafted-books">{{ i18n.enumerateList(bookNames) }}</span>
6-
<span class="date">{{ formatDate(entry.additionalInfo?.dateFinished) }}</span>
7-
</div>
8-
<span class="spacer"></span>
9-
<span class="status" [style.color]="getStatus(entry.state).color">
10-
<mat-icon class="status-icon">{{ getStatus(entry.state).icons }}</mat-icon>
11-
{{ t(getStatus(entry.state).text) }}
12-
</span>
13-
@if (!forceDetailsOpen && hasDetails) {
14-
<mat-icon class="expand-icon">expand_{{ detailsOpen ? "less" : "more" }}</mat-icon>
15-
}
16-
</div>
17-
@if (detailsOpen && hasDetails) {
3+
<mat-expansion-panel
4+
[disabled]="forceDetailsOpen || !hasDetails"
5+
[expanded]="forceDetailsOpen"
6+
[hideToggle]="forceDetailsOpen || !hasDetails"
7+
>
8+
<mat-expansion-panel-header [collapsedHeight]="'auto'" [expandedHeight]="'auto'">
9+
<mat-panel-title>
10+
<span class="drafted-books">{{ i18n.enumerateList(bookNames) }}</span>
11+
<span class="date">{{ formatDate(entry.additionalInfo?.dateFinished) }}</span>
12+
</mat-panel-title>
13+
<mat-panel-description>
14+
<span class="status" [ngClass]="getStatus(entry.state).color">
15+
<mat-icon class="status-icon">{{ getStatus(entry.state).icons }}</mat-icon>
16+
{{ t(getStatus(entry.state).text) }}
17+
</span>
18+
</mat-panel-description>
19+
</mat-expansion-panel-header>
1820
<div class="draft-entry-body">
19-
<ng-container *ngIf="canDownloadBuild">
21+
@if (canDownloadBuild) {
2022
<p>{{ t("click_book_to_preview") }}</p>
2123
<p class="book-buttons">
2224
<app-draft-preview-books [build]="entry"></app-draft-preview-books>
2325
</p>
2426
<app-draft-download-button [build]="entry" [flat]="true"></app-draft-download-button>
25-
</ng-container>
26-
<ng-container *ngIf="hasTrainingData">
27-
<p *ngIf="canDownloadBuild">
28-
<a
29-
href="javascript:;"
30-
(click)="$event.preventDefault(); trainingDataOpen = !trainingDataOpen"
31-
class="training-data-link"
32-
>
33-
<mat-icon>expand_{{ trainingDataOpen ? "less" : "more" }}</mat-icon>
34-
{{ t(trainingDataOpen ? "hide_model_training_data" : "show_model_training_data") }}
35-
</a>
36-
</p>
37-
@if (trainingDataOpen) {
38-
<p class="training-data">
39-
<strong>{{ t("training_data_configuration") }}</strong>
27+
}
28+
@if (hasTrainingConfiguration) {
29+
@if (canDownloadBuild) {
30+
<p>
31+
<a
32+
href="javascript:;"
33+
(click)="trainingConfigurationOpen = !trainingConfigurationOpen"
34+
class="training-configuration-link"
35+
>
36+
<mat-icon>expand_{{ trainingConfigurationOpen ? "less" : "more" }}</mat-icon>
37+
{{
38+
t(
39+
trainingConfigurationOpen
40+
? "hide_model_training_configuration"
41+
: "show_model_training_configuration"
42+
)
43+
}}
44+
</a>
45+
</p>
46+
}
47+
@if (trainingConfigurationOpen) {
48+
<p>
49+
<strong>{{ t("training_model_description") }}</strong>
4050
</p>
41-
<table mat-table [dataSource]="trainingData">
51+
<table mat-table [dataSource]="trainingConfiguration">
4252
<ng-container matColumnDef="bookNames">
4353
<th mat-header-cell *matHeaderCellDef></th>
4454
<td mat-cell *matCellDef="let element">{{ i18n.enumerateList(element.bookNames) }}</td>
@@ -59,8 +69,8 @@
5969
<tr mat-row *matRowDef="let row; columns: columnsToDisplay"></tr>
6070
</table>
6171
}
62-
</ng-container>
72+
}
6373
</div>
64-
}
74+
</mat-expansion-panel>
6575
}
6676
</ng-container>

‎src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-history-list/draft-history-entry/draft-history-entry.component.scss

Lines changed: 28 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -1,37 +1,24 @@
1-
:host {
2-
background-color: var(--draft-history-entry-details-background-color);
3-
border-radius: 8px;
4-
> * {
5-
padding: 8px 16px;
6-
border-radius: 8px;
7-
}
1+
mat-expansion-panel-header {
2+
padding: 8px 16px;
83
}
94

10-
.draft-entry-heading {
11-
background-color: var(--draft-history-entry-heading-background-color);
5+
mat-panel-title {
126
display: flex;
13-
align-items: center;
14-
gap: 1em;
15-
&.expandable {
16-
cursor: pointer;
17-
}
18-
&.expandable:hover {
19-
background-color: var(--draft-history-entry-heading-background-hover-color);
20-
}
21-
}
22-
23-
.drafted-books {
24-
font-size: 1.1em;
7+
align-items: flex-start;
8+
flex-direction: column;
9+
flex-grow: 1;
2510
}
2611

27-
.spacer {
28-
flex-grow: 1;
12+
mat-panel-description {
13+
flex-grow: 0;
2914
}
3015

31-
.entry-description {
32-
display: flex;
33-
flex-direction: column;
34-
gap: 4px;
16+
.drafted-books {
17+
color: var(
18+
--mat-expansion-header-text-color,
19+
var(--mat-app-on-surface)
20+
); /* Keep the text color, even when disabled */
21+
font-size: 1.1em;
3522
}
3623

3724
.date {
@@ -49,7 +36,7 @@
4936
flex-shrink: 0;
5037
}
5138

52-
.training-data-link {
39+
.training-configuration-link {
5340
display: flex;
5441
align-items: center;
5542
}
@@ -62,3 +49,16 @@ strong {
6249
display: flex;
6350
gap: 4px;
6451
}
52+
53+
/* Color classes for the status icon */
54+
.grey {
55+
color: var(--draft-history-entry-grey-color);
56+
}
57+
58+
.green {
59+
color: var(--draft-history-entry-green-color);
60+
}
61+
62+
.red {
63+
color: var(--draft-history-entry-red-color);
64+
}

‎src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-history-list/draft-history-entry/draft-history-entry.component.spec.ts

Lines changed: 13 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -1,17 +1,22 @@
11
import { ComponentFixture, fakeAsync, TestBed, tick } from '@angular/core/testing';
2+
import { NoopAnimationsModule } from '@angular/platform-browser/animations';
23
import { createTestUserProfile } from 'realtime-server/lib/esm/common/models/user-test-data';
34
import { createTestProjectProfile } from 'realtime-server/lib/esm/scriptureforge/models/sf-project-test-data';
45
import { anything, mock, when } from 'ts-mockito';
56
import { I18nService } from 'xforge-common/i18n.service';
67
import { UserProfileDoc } from 'xforge-common/models/user-profile-doc';
8+
import { TestRealtimeModule } from 'xforge-common/test-realtime.module';
79
import { configureTestingModule, TestTranslocoModule } from 'xforge-common/test-utils';
810
import { UserService } from 'xforge-common/user.service';
911
import { SFProjectProfileDoc } from '../../../../core/models/sf-project-profile-doc';
12+
import { SF_TYPE_REGISTRY } from '../../../../core/models/sf-type-registry';
1013
import { SFProjectService } from '../../../../core/sf-project.service';
1114
import { BuildDto } from '../../../../machine-api/build-dto';
1215
import { BuildStates } from '../../../../machine-api/build-states';
16+
import { DraftGenerationService } from '../../draft-generation.service';
1317
import { DraftHistoryEntryComponent } from './draft-history-entry.component';
1418

19+
const mockedDraftGenerationService = mock(DraftGenerationService);
1520
const mockedI18nService = mock(I18nService);
1621
const mockedSFProjectService = mock(SFProjectService);
1722
const mockedUserService = mock(UserService);
@@ -21,8 +26,9 @@ describe('DraftHistoryEntryComponent', () => {
2126
let fixture: ComponentFixture<DraftHistoryEntryComponent>;
2227

2328
configureTestingModule(() => ({
24-
imports: [TestTranslocoModule],
29+
imports: [NoopAnimationsModule, TestTranslocoModule, TestRealtimeModule.forRoot(SF_TYPE_REGISTRY)],
2530
providers: [
31+
{ provide: DraftGenerationService, useMock: mockedDraftGenerationService },
2632
{ provide: I18nService, useMock: mockedI18nService },
2733
{ provide: SFProjectService, useMock: mockedSFProjectService },
2834
{ provide: UserService, useMock: mockedUserService }
@@ -39,24 +45,6 @@ describe('DraftHistoryEntryComponent', () => {
3945
expect(component).toBeTruthy();
4046
});
4147

42-
it('forceDetailsOpen should set detailsOpen', () => {
43-
expect(component.detailsOpen).toBe(false);
44-
component.forceDetailsOpen = true;
45-
expect(component.detailsOpen).toBe(true);
46-
expect(component.forceDetailsOpen).toBe(true);
47-
component.forceDetailsOpen = false;
48-
expect(component.detailsOpen).toBe(true);
49-
expect(component.forceDetailsOpen).toBe(false);
50-
});
51-
52-
it('headerClicked should toggle detailsOpen', () => {
53-
expect(component.detailsOpen).toBe(false);
54-
component.headerClicked();
55-
expect(component.detailsOpen).toBe(true);
56-
component.headerClicked();
57-
expect(component.detailsOpen).toBe(false);
58-
});
59-
6048
describe('entry', () => {
6149
it('should handle undefined values', () => {
6250
component.entry = undefined;
@@ -114,14 +102,14 @@ describe('DraftHistoryEntryComponent', () => {
114102
expect(component.entry).toBe(entry);
115103
expect(component.sourceLanguage).toBe('fr');
116104
expect(component.targetLanguage).toBe('en');
117-
expect(component.trainingData).toEqual([
105+
expect(component.trainingConfiguration).toEqual([
118106
{
119107
bookNames: ['Exodus'],
120108
source: 'src',
121109
target: 'tar'
122110
}
123111
]);
124-
expect(component.trainingDataOpen).toBe(false);
112+
expect(component.trainingConfigurationOpen).toBe(false);
125113
}));
126114

127115
it('should handle builds where the draft cannot be downloaded yet', fakeAsync(() => {
@@ -159,14 +147,14 @@ describe('DraftHistoryEntryComponent', () => {
159147
expect(component.entry).toBe(entry);
160148
expect(component.sourceLanguage).toBe('');
161149
expect(component.targetLanguage).toBe('');
162-
expect(component.trainingData).toEqual([
150+
expect(component.trainingConfiguration).toEqual([
163151
{
164152
bookNames: ['Exodus'],
165153
source: '',
166154
target: ''
167155
}
168156
]);
169-
expect(component.trainingDataOpen).toBe(true);
157+
expect(component.trainingConfigurationOpen).toBe(true);
170158
}));
171159

172160
it('should handle builds with additional info referencing a deleted user', fakeAsync(() => {
@@ -233,11 +221,11 @@ describe('DraftHistoryEntryComponent', () => {
233221

234222
describe('getStatus', () => {
235223
it('should handle known build state strings', () => {
236-
expect(component.getStatus(BuildStates.Active)).not.toBeUndefined();
224+
expect(component.getStatus(BuildStates.Active)).toBeDefined();
237225
});
238226

239227
it('should handle unknown build state strings', () => {
240-
expect(component.getStatus('unknown build state' as BuildStates)).not.toBeUndefined();
228+
expect(component.getStatus('unknown build state' as BuildStates)).toBeDefined();
241229
});
242230
});
243231
});

0 commit comments

Comments
 (0)