Draft jobs duration mean: only include successful jobs (#3611)

Author: marksvc

.github/copilot-instructions.md

@@ -106,6 +106,10 @@ This repository contains three interconnected applications:
 - Corner-cases happen. They should be handled in code.
 - Please don't change existing code without good justification. Existing code largely works and changing it will cause work for code review. Leave existing code as is when possible.
 
+# Frontend code
+
+- Pay attention to available types and type guards in src/SIL.XForge.Scripture/ClientApp/src/type-utils.ts.
+
 # Running commands
 
 - If you run frontend tests, run them in the `src/SIL.XForge.Scripture/ClientApp` directory with a command such as `npm run test:headless -- --watch=false --include '**/text.component.spec.ts' --include '**/settings.component.spec.ts'`

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/_draft-jobs-theme.scss

@@ -7,7 +7,6 @@
   --sf-draft-jobs-project-link-color: #{mat.get-theme-color($theme, primary, if($is-dark, 70, 40))};
   --sf-draft-jobs-book-count-color: #{mat.get-theme-color($theme, neutral, if($is-dark, 70, 40))};
   --sf-draft-jobs-disabled-link-color: #{mat.get-theme-color($theme, neutral, if($is-dark, 50, 50))};
-  --sf-draft-jobs-statistics-background: #{mat.get-theme-color($theme, neutral-variant, if($is-dark, 30, 90))};
   --sf-draft-jobs-statistics-text: #{mat.get-theme-color($theme, on-surface)};
 }
 

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/draft-jobs-export.service.spec.ts

@@ -90,12 +90,12 @@ describe('DraftJobsExportService', () => {
 
       // Check mean duration row (index 3) - (30 + 60) / 2 = 45 minutes
       // Label in servalBuildId, value in startTime
-      expect(spreadsheetRows[3].servalBuildId).toBe('Mean duration minutes');
+      expect(spreadsheetRows[3].servalBuildId).toMatch(/Mean/);
       expect(spreadsheetRows[3].startTime).toBe('45');
 
       // Check max duration row (index 4) - max(30, 60) = 60 minutes
       // Label in servalBuildId, value in startTime
-      expect(spreadsheetRows[4].servalBuildId).toBe('Max duration minutes');
+      expect(spreadsheetRows[4].servalBuildId).toMatch(/Max/);
       expect(spreadsheetRows[4].startTime).toBe('60');
     });
 
@@ -109,10 +109,10 @@ describe('DraftJobsExportService', () => {
 
       // Check statistics are 0 when no data
       // Label in servalBuildId, value in startTime
-      expect(spreadsheetRows[1].servalBuildId).toBe('Mean duration minutes');
+      expect(spreadsheetRows[1].servalBuildId).toMatch(/Mean/);
       expect(spreadsheetRows[1].startTime).toBe('0');
 
-      expect(spreadsheetRows[2].servalBuildId).toBe('Max duration minutes');
+      expect(spreadsheetRows[2].servalBuildId).toMatch(/Max/);
       expect(spreadsheetRows[2].startTime).toBe('0');
     });
   });

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/draft-jobs-export.service.ts

@@ -124,7 +124,7 @@ export class DraftJobsExportService {
 
     // Append mean duration row. This is not in a 'correct column', but puts some desired data into the output.
     const meanRow: SpreadsheetRow = {
-      servalBuildId: 'Mean duration minutes',
+      servalBuildId: 'Mean successful duration minutes',
       startTime: meanDuration != null ? this.msToMinutes(meanDuration).toFixed(0) : '0',
       status: '',
       sfProjectId: '',
@@ -135,7 +135,7 @@ export class DraftJobsExportService {
 
     // Append max duration row
     const maxRow: SpreadsheetRow = {
-      servalBuildId: 'Max duration minutes',
+      servalBuildId: 'Max all durations minutes',
       startTime: maxDuration != null ? this.msToMinutes(maxDuration).toFixed(0) : '0',
       status: '',
       sfProjectId: '',

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/draft-jobs.component.html

@@ -16,16 +16,16 @@
         <span>Export RSV</span>
       </button>
     </mat-menu>
-    @if (meanDurationFormatted != null || maxDurationFormatted != null) {
-      <div class="duration-statistics">
-        @if (meanDurationFormatted != null) {
-          <span class="stat-item">Mean duration: {{ meanDurationFormatted }}</span>
-        }
-        @if (maxDurationFormatted != null) {
-          <span class="stat-item">Max duration: {{ maxDurationFormatted }}</span>
-        }
+    <div class="duration-area">
+      <div class="duration-area-stat">
+        <span class="duration-area-stat-label"> Mean successful duration </span>
+        <span> {{ meanDurationFormatted ?? "n/a" }} </span>
       </div>
-    }
+      <div class="duration-area-stat">
+        <span class="duration-area-stat-label"> Max all durations </span
+        ><span> {{ maxDurationFormatted ?? "n/a" }} </span>
+      </div>
+    </div>
     <div class="grouping-toggle">
       <div class="grouping-toggle-label">
         Determination

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/draft-jobs.component.scss

@@ -116,19 +116,21 @@
     }
   }
 
-  .duration-statistics {
+  .duration-area {
+    padding: 1rem;
     display: flex;
-    gap: 16px;
-    padding: 8px 16px;
-    background-color: var(--sf-draft-jobs-statistics-background);
-    border-radius: 4px;
-    font-size: 14px;
-    margin-bottom: 2rem;
-
-    .stat-item {
-      color: var(--sf-draft-jobs-statistics-text);
-      font-weight: 500;
-    }
+    flex-direction: column;
+    gap: 0.5rem;
+  }
+
+  .duration-area-stat {
+    display: flex;
+    justify-content: space-between;
+    gap: 1rem;
+  }
+
+  .duration-area-stat-label {
+    font-weight: 500;
   }
 
   .grouping-toggle {

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/draft-jobs.component.spec.ts

@@ -21,7 +21,7 @@ import { configureTestingModule, getTestTranslocoModule } from 'xforge-common/te
 import { SF_TYPE_REGISTRY } from '../core/models/sf-type-registry';
 import { SFProjectService } from '../core/sf-project.service';
 import { EventMetric, EventScope } from '../event-metrics/event-metric';
-import { DraftJob, DraftJobsComponent } from './draft-jobs.component';
+import { DraftJob, DraftJobsComponent, DraftJobsTableRow, DraftJobStatus } from './draft-jobs.component';
 import { JobDetailsDialogComponent } from './job-details-dialog.component';
 import sampleEvents from './sample-events.json';
 import { ServalAdministrationService } from './serval-administration.service';
@@ -523,6 +523,19 @@ describe('DraftJobsComponent', () => {
         // Mean: (3600000 + 7200000) / 2 = 5400000
         expect(env.component.meanDuration).toBe(5400000);
       }));
+
+      it('should ignore non-successful jobs when calculating mean', fakeAsync(() => {
+        const env = new TestEnvironment({ hasEvents: false });
+        env.wait();
+        env.component.rows = [
+          env.createRowWithDuration(3600000, 'success'), // counts
+          env.createRowWithDuration(5400000, 'running'), // excluded because not success
+          env.createRowWithDuration(7200000, 'failed') // excluded because not success
+        ];
+
+        // Mean: only the successful job's duration should be counted
+        expect(env.component.meanDuration).toBe(3600000);
+      }));
     });
 
     describe('maxDuration', () => {
@@ -727,12 +740,13 @@ class TestEnvironment {
     this.component.onGroupingModeChange(mode);
   }
 
-  createRowWithDuration(duration: number | undefined): any {
+  createRowWithDuration(duration: number | undefined, statusOverride?: DraftJobStatus): DraftJobsTableRow {
+    const jobStatus: DraftJobStatus = statusOverride ?? (duration != null ? 'success' : 'running');
     return {
       job: {
         buildId: 'build-123',
         projectId: 'project1',
-        status: duration != null ? 'success' : 'running',
+        status: jobStatus,
         startTime: new Date('2025-01-15T10:00:00Z'),
         finishTime: duration != null ? new Date('2025-01-15T11:00:00Z') : undefined,
         duration: duration,
@@ -747,7 +761,7 @@ class TestEnvironment {
       projectDeleted: false,
       startTimeStamp: '2025-01-15 10:00 UTC',
       duration: duration != null ? '1h 0m 0s' : undefined,
-      status: duration != null ? 'Success' : 'Running',
+      status: this.component['getStatusDisplay'](jobStatus),
       userId: 'user123',
       trainingBooks: [],
       translationBooks: []

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/draft-jobs.component.ts

@@ -29,8 +29,7 @@ import { I18nService } from 'xforge-common/i18n.service';
 import { NoticeService } from 'xforge-common/notice.service';
 import { OnlineStatusService } from 'xforge-common/online-status.service';
 import { OwnerComponent } from 'xforge-common/owner/owner.component';
-import { isPopulatedString } from 'xforge-common/utils';
-import { hasElements, notNull, PopulatedArray } from '../../type-utils';
+import { hasElements, isPopulatedString, notNull, PopulatedArray } from '../../type-utils';
 import { SFProjectService } from '../core/sf-project.service';
 import { EventMetric } from '../event-metrics/event-metric';
 import { InfoComponent } from '../shared/info/info.component';
@@ -40,6 +39,10 @@ import { DateRangePickerComponent, NormalizedDateRange } from './date-range-pick
 import { DraftJobsExportService } from './draft-jobs-export.service';
 import { JobDetailsDialogComponent } from './job-details-dialog.component';
 import { ServalAdministrationService } from './serval-administration.service';
+
+/** Outcome or in-progress situation of a draft generation request job. */
+export type DraftJobStatus = 'running' | 'success' | 'failed' | 'cancelled' | 'incomplete';
+
 interface ProjectBooks {
   projectId: string;
   books: string[];
@@ -60,7 +63,7 @@ export interface DraftJob {
   events: EventMetric[];
   /** Events with the same build ID that weren't included in the main job tracking */
   additionalEvents: EventMetric[];
-  status: 'running' | 'success' | 'failed' | 'cancelled' | 'incomplete';
+  status: DraftJobStatus;
   startTime: Date | undefined;
   finishTime: Date | undefined;
   duration: number | undefined;
@@ -183,9 +186,12 @@ export class DraftJobsComponent extends DataLoadingComponent implements OnInit {
 
   /** Of jobs in the table. */
   get meanDuration(): number | undefined {
-    const durations = this.rows.map(r => r.job.duration).filter((d): d is number => d != null);
-    if (durations.length === 0) return undefined;
-    return durations.reduce((sum, d) => sum + d, 0) / durations.length;
+    const successfulDurations: number[] = this.rows
+      .filter(row => row.job.status === 'success')
+      .map(row => row.job.duration)
+      .filter(notNull);
+    if (successfulDurations.length === 0) return undefined;
+    return successfulDurations.reduce((sum, duration) => sum + duration, 0) / successfulDurations.length;
   }
 
   get maxDuration(): number | undefined {
@@ -660,7 +666,7 @@ export class DraftJobsComponent extends DataLoadingComponent implements OnInit {
   }
 
   private finalizeJobStatus(job: DraftJob): void {
-    let status: 'running' | 'success' | 'failed' | 'cancelled' | 'incomplete';
+    let status: DraftJobStatus;
     let finishTime: Date | undefined = undefined;
     let duration: number | undefined = undefined;
     let errorMessage: string | undefined;

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/job-details-dialog.component.ts

@@ -13,7 +13,7 @@ import { MatTab, MatTabGroup } from '@angular/material/tabs';
 import { MatTooltip } from '@angular/material/tooltip';
 import { I18nService } from 'xforge-common/i18n.service';
 import { L10nNumberPipe } from 'xforge-common/l10n-number.pipe';
-import { isPopulatedString } from 'xforge-common/utils';
+import { isPopulatedString } from '../../type-utils';
 import { EventMetric } from '../event-metrics/event-metric';
 import { JsonViewerComponent } from '../shared/json-viewer/json-viewer.component';
 import { NoticeComponent } from '../shared/notice/notice.component';

src/SIL.XForge.Scripture/ClientApp/src/type-utils.ts

@@ -131,3 +131,10 @@ export type PopulatedArray<T> = [T, ...T[]] | [...T[], T] | [T, ...T[], T];
 export function hasElements<T>(array: T[]): array is PopulatedArray<T> {
   return array.length > 0;
 }
+
+/** The input is a string, and it is not empty (''). It is not null or undefined. This method could alternatively be known as "isNonEmptyString".
+ *
+ * If the negation of this is true, then the input is '' or null or undefined or another non-string. */
+export function isPopulatedString(value: unknown): value is Exclude<string, ''> {
+  return typeof value === 'string' && value !== '';
+}