Merge pull request #3331 from SeedCompany/reextract-pnps

Author: CarsonF

src/components/product-progress/handlers/extract-pnp-progress.handler.ts

@@ -1,12 +1,14 @@
 import { mapOf } from '@seedcompany/common';
+import { oneLine } from 'common-tags';
 import { EventsHandler, ILogger, Logger } from '~/core';
 import { ReportType } from '../../periodic-report/dto';
 import { PeriodicReportUploadedEvent } from '../../periodic-report/events';
 import { ProductService } from '../../product';
-import { ProducibleType } from '../../product/dto';
+import { ProducibleType, ProductStep } from '../../product/dto';
 import { isScriptureEqual } from '../../scripture';
 import { ProgressReportVariantProgress as Progress } from '../dto';
 import { ProductProgressService } from '../product-progress.service';
+import { StepNotPlannedException } from '../step-not-planned.exception';
 import { StepProgressExtractor } from '../step-progress-extractor.service';
 
 @EventsHandler(PeriodicReportUploadedEvent)
@@ -62,7 +64,7 @@ export class ExtractPnpProgressHandler {
       if (row.story) {
         const productId = storyProducts.get(row.story);
         if (productId) {
-          return { productId, steps };
+          return { extracted: row, productId, steps };
         }
       }
 
@@ -73,15 +75,19 @@ export class ExtractPnpProgressHandler {
             isScriptureEqual(ref.scriptureRanges, row.scripture),
         );
         if (exactScriptureMatch) {
-          return { productId: exactScriptureMatch.id, steps };
+          return { extracted: row, productId: exactScriptureMatch.id, steps };
         }
 
         const unspecifiedScriptureMatch = scriptureProducts.find(
           (ref) =>
             ref.book === row.bookName && ref.totalVerses === row.totalVerses,
         );
         if (unspecifiedScriptureMatch) {
-          return { productId: unspecifiedScriptureMatch.id, steps };
+          return {
+            extracted: row,
+            productId: unspecifiedScriptureMatch.id,
+            steps,
+          };
         }
       }
 
@@ -95,16 +101,48 @@ export class ExtractPnpProgressHandler {
 
     // Update progress for report & product
     await Promise.all(
-      updates.map(async (input) => {
-        await this.progress.update(
-          {
-            ...input,
-            reportId: event.report.id,
-            // TODO this seems fine for now as only this variant will upload PnPs.
-            variant: Progress.FallbackVariant,
-          },
-          event.session,
-        );
+      updates.map(async ({ extracted, ...input }) => {
+        try {
+          await this.progress.update(
+            {
+              ...input,
+              reportId: event.report.id,
+              // TODO this seems fine for now as only this variant will upload PnPs.
+              variant: Progress.FallbackVariant,
+            },
+            event.session,
+          );
+        } catch (e) {
+          if (
+            !(
+              e instanceof AggregateError &&
+              e.message === 'Invalid Progress Input'
+            )
+          ) {
+            throw e;
+          }
+          for (const error of e.errors) {
+            if (!(error instanceof StepNotPlannedException)) {
+              continue;
+            }
+            const stepLabel = ProductStep.entry(error.step).label;
+            // kinda. close enough, I think, we give the cell ref as well.
+            const goalLabel = extracted.bookName ?? extracted.story;
+            result.addProblem({
+              severity: 'Error',
+              groups: [
+                'Step is not planned',
+                `_${goalLabel}_ has progress reported on steps that have not been declared to be worked in this engagement`,
+                `_${goalLabel}_ has not declared _${stepLabel}_ \`${extracted.cell.ref}\` as a step that will be worked in this engagement`,
+              ],
+              message: oneLine`
+                Please update the goal in CORD to mark this step as planned
+                or upload an updated PnP file to the "Planning Spreadsheet" on the engagement page.
+              `,
+              source: extracted.cell,
+            });
+          }
+        }
       }),
     );
   }

src/components/product-progress/product-progress.service.ts

@@ -152,17 +152,21 @@ export class ProductProgressService {
       );
     }
 
-    input.steps.forEach((step, index) => {
+    const errors = input.steps.flatMap((step, index) => {
       if (!scope.steps.includes(step.step)) {
-        throw new StepNotPlannedException(input.productId, step.step, index);
+        return new StepNotPlannedException(input.productId, step.step, index);
       }
       if (step.completed && step.completed > scope.progressTarget) {
-        throw new InputException(
+        return new InputException(
           "Completed value cannot exceed product's progress target",
           `steps.${index}.completed`,
         );
       }
+      return [];
     });
+    if (errors.length > 0) {
+      throw new AggregateError(errors, 'Invalid Progress Input');
+    }
 
     const progress = await this.repo.update(input);
     return this.secure(progress, this.privilegesFor(session, scope))!;

src/components/product-progress/step-progress-extractor.service.ts

@@ -39,6 +39,7 @@ type ExtractedRow = MergeExclusive<
    */
   rowIndex: number;
   steps: ReadonlyArray<{ step: Step; completed?: number | null }>;
+  cell: Cell<ProgressSheet>;
 };
 
 @Injectable()
@@ -100,6 +101,7 @@ const parseProgressRow =
       rowIndex: rowIndex + 1,
       order: index + 1,
       steps,
+      cell,
     };
 
     if (sheet.isOBS()) {

src/components/progress-report/migrations/reextract-all-progress-reports.migration.ts

@@ -8,7 +8,7 @@ import { FileVersion } from '../../file/dto';
 import { PeriodicReportUploadedEvent } from '../../periodic-report/events';
 import { ProgressReport } from '../dto';
 
-@Migration('2024-11-11T16:00:04')
+@Migration('2024-11-11T16:00:06')
 export class ReextractPnpProgressReportsMigration extends BaseMigration {
   constructor(
     private readonly eventBus: IEventBus,
@@ -26,7 +26,7 @@ export class ReextractPnpProgressReportsMigration extends BaseMigration {
       try {
         const pnp = this.files.asDownloadable(fv);
         const event = new PeriodicReportUploadedEvent(report, pnp, session);
-        await this.eventBus.publish(event);
+        await this.db.conn.runInTransaction(() => this.eventBus.publish(event));
       } catch (e) {
         this.logger.error('Failed to re-extract PnP', {
           report: report.id,

src/core/exception/exception.normalizer.ts

@@ -42,12 +42,14 @@ export interface ExceptionJson {
   stack: string;
   code: string;
   codes: ReadonlySet<string>;
+  /** From {@link AggregateError.errors} */
+  aggregatees?: readonly ExceptionJson[];
   [key: string]: unknown;
 }
 
 /**
  * Denote normalization has already happened for this error.
- * So the GQL server can know acturately if needs to normalize or not.
+ * So the GQL server can know accurately if it needs to normalize or not.
  */
 export class NormalizedException extends Error {
   constructor(readonly normalized: ExceptionJson) {
@@ -132,6 +134,26 @@ export class ExceptionNormalizer {
       };
     }
 
+    if (
+      ex instanceof AggregateError &&
+      // not subclassed
+      ex.name === 'AggregateError'
+    ) {
+      const aggregatees = ex.errors.map((e) =>
+        this.normalize({
+          ...params,
+          ex: e,
+        }),
+      );
+      return {
+        aggregatees,
+        // shrug?
+        codes: [
+          aggregatees.every((e) => e.codes.has('Client')) ? 'Client' : 'Server',
+        ],
+      };
+    }
+
     const gqlContext =
       context && context.getType() === 'graphql'
         ? GqlExecutionContext.create(context as any)

src/core/graphql/graphql-error-formatter.ts

@@ -34,7 +34,7 @@ export class GraphqlErrorFormatter {
     () =>
     ({ result, setResult }) => {
       if (result.length > 0) {
-        const errors = result.map((error) => this.formatError(error));
+        const errors = result.flatMap((error) => this.formatError(error));
         setResult(errors);
       }
     };
@@ -43,7 +43,9 @@ export class GraphqlErrorFormatter {
     onExecuteDone: (params) =>
       handleStreamOrSingleExecutionResult(params, ({ result, setResult }) => {
         if (result.errors && result.errors.length > 0) {
-          const errors = result.errors.map((error) => this.formatError(error));
+          const errors = result.errors.flatMap((error) =>
+            this.formatError(error),
+          );
           setResult({ ...result, errors });
         }
       }),
@@ -71,20 +73,23 @@ export class GraphqlErrorFormatter {
       this.filter.logIt(normalized, error.originalError ?? error);
     }
 
-    const { message, stack, code: _, ...extensions } = normalized;
-    const { codes } = extensions;
+    // Unwrap AggregateError's errors to flat gql errors
+    return (normalized.aggregatees ?? [normalized]).map((innerEx) => {
+      const { message, stack, code: _, ...extensions } = innerEx;
+      const { codes } = extensions;
 
-    // Schema & validation errors don't have meaningful stack traces, so remove them
-    const worthlessTrace = codes.has('Validation') || codes.has('GraphQL');
-    if (!worthlessTrace) {
-      extensions.stacktrace = stack.split('\n');
-    }
+      // Schema & validation errors don't have meaningful stack traces, so remove them
+      const worthlessTrace = codes.has('Validation') || codes.has('GraphQL');
+      if (!worthlessTrace) {
+        extensions.stacktrace = stack.split('\n');
+      }
 
-    return new GraphQLError(message, {
-      nodes: error.nodes,
-      positions: error.positions,
-      path: error.path,
-      extensions: { ...error.extensions, ...extensions },
+      return new GraphQLError(message, {
+        nodes: error.nodes,
+        positions: error.positions,
+        path: error.path,
+        extensions: { ...error.extensions, ...extensions },
+      });
     });
   };
 }