Integrate onboarding form #3612

Nateowami · 2025-12-11T21:35:59Z

Two tests are still failing, and I haven't had time to fix them yet.

Here's a quick walkthrough.

Enable the feature flag

Fill out the form

View the list of submissions

View/manage an individual submission

This change is

...entApp/src/app/translate/draft-generation/draft-apply-dialog/draft-apply-dialog.component.ts

Copilot

Pull request overview

This PR integrates an in-app onboarding form for draft generation signup, replacing the previous external link approach. The implementation spans backend (C#) and frontend (Angular) with comprehensive CRUD operations, admin management interface, and E2E testing.

Key Changes

Added backend RPC controller and database models for onboarding requests with admin management capabilities
Created frontend form component with validation, conditional fields, and project/resource selection
Implemented Serval admin interface for viewing, managing, and processing onboarding requests
Refactored ProjectSelectComponent validation to support custom validators and error message mapping

Reviewed changes

Copilot reviewed 47 out of 47 changed files in this pull request and generated 13 comments.

Show a summary per file

File	Description
`src/SIL.XForge/Controllers/UrlConstants.cs`	Added URL constant for onboarding requests endpoint
`src/SIL.XForge.Scripture/Services/SFJsonRpcApplicationBuilderExtensions.cs`	Registered onboarding RPC controller
`src/SIL.XForge.Scripture/Models/OnboardingRequest.cs`	Backend data models for onboarding requests, submissions, and comments
`src/SIL.XForge.Scripture/DataAccess/SFDataAccessServiceCollectionExtensions.cs`	MongoDB repository configuration for onboarding requests
`src/SIL.XForge.Scripture/Controllers/OnboardingRequestRpcController.cs`	RPC endpoints for submission, retrieval, and admin management
`src/SIL.XForge.Scripture/ClientApp/tsconfig.json`	Disabled `noUnusedLocals` compiler option
`src/SIL.XForge.Scripture/ClientApp/src/xforge-common/url-constants.ts`	Added frontend URL constant
`src/SIL.XForge.Scripture/ClientApp/src/xforge-common/notice.service.ts`	Changed loading state implementation from `setTimeout` to `queueMicrotask`
`src/SIL.XForge.Scripture/ClientApp/src/xforge-common/feature-flags/feature-flag.service.ts`	Added feature flag for in-app draft signup form
`src/SIL.XForge.Scripture/ClientApp/src/material-styles.scss`	Imported theme files for new components
`src/SIL.XForge.Scripture/ClientApp/src/assets/i18n/non_checking_en.json`	Added localization strings for signup form and related UI
`src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/drafting-signup.service.ts`	Service for onboarding request CRUD operations
`src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-signup-form/draft-onboarding-form.component.*`	Main signup form component with reactive forms and validation
`src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-generation.component.*`	Updated to show signup button and existing request status
`src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-apply-dialog/*`	Refactored validation logic from custom validators to inline functions
`src/SIL.XForge.Scripture/ClientApp/src/app/shared/sfvalidators.ts`	Removed `CustomValidatorState` enum and `customValidator` method
`src/SIL.XForge.Scripture/ClientApp/src/app/shared/dev-only/*`	New component for dev-only content visibility
`src/SIL.XForge.Scripture/ClientApp/src/app/settings/settings.component.ts`	Changed to use `MatButtonModule` and `MatIconModule`
`src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-administration.service.ts`	Added `getByParatextId` method
`src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-administration.component.*`	Added draft requests tab
`src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/draft-request-detail.component.*`	Detail view for individual onboarding requests
`src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/draft-request-constants.ts`	Constants for status and resolution labels
`src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/draft-onboarding-requests/*`	Admin list view for managing onboarding requests
`src/SIL.XForge.Scripture/ClientApp/src/app/project-select/project-select.component.*`	Refactored to support external validators and custom error messages
`src/SIL.XForge.Scripture/ClientApp/src/app/app.routes.ts`	Added routes for signup form and request detail
`src/SIL.XForge.Scripture/ClientApp/e2e/workflows/submit-draft-signup.ts`	E2E test for form submission
`.github/copilot-instructions.md`	Added guideline about using CSS variables/themes instead of hard-coded colors

Copilot · 2025-12-11T21:44:51Z

src/SIL.XForge.Scripture/ClientApp/src/xforge-common/notice.service.ts

    });
  }

+  private nextLoadingValue = this.isAppLoading;


The nextLoadingValue field is not initialized with a type annotation. According to the coding guidelines, specify types when declaring variables. This should be private nextLoadingValue: boolean = this.isAppLoading;

Copilot · 2025-12-11T21:44:51Z

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

+  async approveRequest(options: { requestId: string; sfProjectId: string }): Promise<OnboardingRequest> {
+    const requestUpdateResult = await this.onlineInvoke<OnboardingRequest | undefined>('setResolution', {
+      requestId: options.requestId,
+      resolution: 'approved'
+    });
+    await this.projectService.onlineSetPreTranslate(options.sfProjectId, true);
+    return requestUpdateResult!;
+  }
+
+  /** Submits a new signup request. */
+  async submitOnboardingRequest(projectId: string, formData: DraftingSignupFormData): Promise<string> {
+    return (await this.onlineInvoke<string>('submitOnboardingRequest', { projectId, formData }))!;
+  }
+
+  /** Gets the existing signup request for the specified project, if any. */
+  async getOpenOnboardingRequest(projectId: string): Promise<OnboardingRequest | null> {
+    return (await this.onlineInvoke<OnboardingRequest | null>('getOpenOnboardingRequest', { projectId }))!;
+  }
+
+  /** Gets all onboarding requests (Serval admin only). */
+  async getAllRequests(): Promise<OnboardingRequest[]> {
+    return (await this.onlineInvoke<OnboardingRequest[]>('getAllRequests'))!;
+  }
+
+  /** Sets the assignee for an onboarding request (Serval admin only). */
+  async setAssignee(requestId: string, assigneeId: string): Promise<OnboardingRequest> {
+    return (await this.onlineInvoke<OnboardingRequest | undefined>('setAssignee', { requestId, assigneeId }))!;
+  }
+
+  /** Sets the resolution of an onboarding request (Serval admin only). */
+  async setResolution(requestId: string, resolution: string | null): Promise<OnboardingRequest> {
+    return (await this.onlineInvoke<OnboardingRequest | undefined>('setResolution', { requestId, resolution }))!;


The onlineInvoke method is called with non-null assertion operators (!) on lines 67, 72, 77, 82, 92, and 97. According to the coding guidelines, prefer explicit null/undefined checks rather than relying on truthy/falsy or assertions. These should return the type Promise<T | undefined> without the non-null assertion, or should explicitly check for undefined before returning.

src/SIL.XForge.Scripture/ClientApp/src/assets/i18n/non_checking_en.json

...c/app/serval-administration/draft-onboarding-requests/draft-onboarding-requests.component.ts

src/SIL.XForge.Scripture/ClientApp/tsconfig.json

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

...entApp/src/app/translate/draft-generation/draft-apply-dialog/draft-apply-dialog.component.ts

src/SIL.XForge.Scripture/Controllers/OnboardingRequestRpcController.cs

+            catch (Exception exception)
+            {
+                _exceptionHandler.RecordEndpointInfoForException(
+                    new Dictionary<string, string>
+                    {
+                        { "method", "SubmitOnboardingRequest" },
+                        { "projectId", projectId },
+                        { "userId", UserId },
+                    }
+                );
+                // report the exception without failing the whole request
+                _exceptionHandler.ReportException(exception);
+            }


The best way to fix this problem, in alignment with .NET best practices, is to catch non-fatal exceptions only, to avoid masking critical application bugs or runtime errors. This can be accomplished by catching only Exception types that are not considered fatal (excluding, for example, OutOfMemoryException, StackOverflowException, ThreadAbortException, etc.).

The standard solution is to use a helper method (provided as an extension or utility) to filter for non-critical exceptions. Since we cannot assume the existence of such a helper, the recommended direct fix is to check the exception type in the catch block and rethrow it if it's a critical exception, or, for more explicitness, to use exception filters (e.g., catch (Exception ex) when (!IsCriticalException(ex))). However, the exception filter approach is preferable and more idiomatic in modern C#.

So:

Add a private static method IsCriticalException(Exception ex) in this class, which returns true for critical exceptions (usually those inheriting from System.SystemException but not Exception itself; standard practice is to filter out: OutOfMemoryException, StackOverflowException, ThreadAbortException, etc.).

Change the catch block from catch (Exception exception) to catch (Exception exception) when (!IsCriticalException(exception)).

Place the helper method definition at the bottom of the class.

All changes are to be made inside the file src/SIL.XForge.Scripture/Controllers/OnboardingRequestRpcController.cs, within the shown code.

codecov · 2025-12-11T21:50:05Z

❌ 2 Tests Failed:

Tests completed	Failed	Passed	Skipped
4609	2	4607	3

View the top 2 failed test(s) by shortest run time

.DraftApplyDialogComponent::DraftApplyDialogComponent notifies user if no edit permissions

Stack Traces | 0.065s run time

Error: Expected true to be false.
    at <Jasmine>
    at .../draft-generation/draft-apply-dialog/draft-apply-dialog.component.spec.ts:141:35
    at Generator.next (<anonymous>)
    at fulfilled (chunk-4ZGUQGYD.js:84:24)

.DraftApplyDialogComponent::DraftApplyDialogComponent updates the target project info when updating the project in the selector

Stack Traces | 0.072s run time

Error: Expected <div _ngcontent-a-c2460869635 class="target-project-content">...</div> to be null.
    at <Jasmine>
    at .../draft-generation/draft-apply-dialog/draft-apply-dialog.component.spec.ts:253:38
    at Generator.next (<anonymous>)
    at fulfilled (chunk-4ZGUQGYD.js:84:24)

To view more test analytics, go to the Test Analytics Dashboard
_{📋 Got 3 mins? Take this short survey to help us improve Test Analytics.}

RaymondLuong3

I tested this branch out and it is working well. I deleted a request and that deleted the record in mongo. I think this is probably OK. It allows the project to submit a new request, which is what I expected would happen.

@RaymondLuong3 partially reviewed 47 files and all commit messages, and made 14 comments.
Reviewable status: all files reviewed, 17 unresolved discussions (waiting on @Nateowami).

src/SIL.XForge.Scripture/ClientApp/e2e/workflows/submit-draft-signup.ts line 131 at r3 (raw file):

    // Back translation project (required when stage is written)
    await selectProjectByFieldName(page, user, 'Select your back translation', 'DHH94');

Not really a back translation but I'll let that slide.

Code quote:

    await selectProjectByFieldName(page, user, 'Select your back translation', 'DHH94');

src/SIL.XForge.Scripture/ClientApp/e2e/test_characterization.json line 24 at r3 (raw file):

  "submit_draft_signup": {
    "success": 0,
    "failure": 0

This looks suspicious that both are assigned the value 0.

Code quote:

  "submit_draft_signup": {
    "success": 0,
    "failure": 0

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-signup-form/draft-onboarding-form.component.ts line 52 at r3 (raw file):

  // selected) even after the user has made the selection (though in the ProjectSelectComponent itself the selection
  // exists and is known to be valid).
  changeDetection: ChangeDetectionStrategy.OnPush,

Thanks for the explanation.

Code quote:

  changeDetection: ChangeDetectionStrategy.OnPush,

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-signup-form/draft-onboarding-form.component.ts line 222 at r3 (raw file):

  }

  private async loadProjectsAndResources(): Promise<void> {

These private functions should be moved to the end of the file.

Code quote:

  private async loadProjectsAndResources(): Promise<void> {

src/SIL.XForge.Scripture/Models/OnboardingRequest.cs line 107 at r3 (raw file):

    public string TranslationLanguageIsoCode { get; set; } = string.Empty;
    public int[]? CompletedBooks { get; set; }
    public int[]? NextBooksToDraft { get; set; }

This can probably be initialized to an empty array and not be nullable.

Code quote:

    public int[]? CompletedBooks { get; set; }
    public int[]? NextBooksToDraft { get; set; }

src/SIL.XForge.Scripture/Controllers/OnboardingRequestRpcController.cs line 36 at r3 (raw file):

    private readonly ISFProjectService _projectService = projectService;

    public async Task<IRpcMethodResult> SubmitOnboardingRequest(string projectId, OnboardingRequestFormData formData)

Add a summary statement here.

Code quote:

   public async Task<IRpcMethodResult> SubmitOnboardingRequest(string projectId, OnboardingRequestFormData formData)

src/SIL.XForge.Scripture/Controllers/OnboardingRequestRpcController.cs line 135 at r3 (raw file):

                {
                    // Create the resource/source project and add the user to it
                    string sourceProjectId = await _projectService.CreateResourceProjectAsync(

What is the reason we need to create the project on SF at this time? Is it so that the serval admin has the ability to download the PT project?

Code quote:

                    string sourceProjectId = await _projectService.CreateResourceProjectAsync(

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-signup-form/draft-onboarding-form.component.html line 46 at r3 (raw file):

                <mat-option value="Bolshoi Group">Bolshoi Group</mat-option>
                <mat-option value="Seed Company">Seed Company</mat-option>
                <mat-option value="none">{{ t("partner_none") }}</mat-option>

We should probably read these companies from the component and iterate on the names rather than coding it directly into the template.

Code quote:

                <mat-option value="Bolshoi Group">Bolshoi Group</mat-option>
                <mat-option value="Seed Company">Seed Company</mat-option>
                <mat-option value="none">{{ t("partner_none") }}</mat-option>

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-signup-form/draft-onboarding-form.component.html line 65 at r3 (raw file):

          <mat-card-content>
            <div>
              <div class="form-field-label">{{ t("translation_language_name_label") }}</div>

This can probably be removed since it is in the textbox label.

Code quote:

             <div class="form-field-label">{{ t("translation_language_name_label") }}</div>

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-signup-form/draft-onboarding-form.component.html line 77 at r3 (raw file):

            <div>
              <!-- <div class="form-field-label">{{ t("translation_language_iso_label") }}</div> -->

This line can be removed since the label is already in the textbox.

Code quote:

              <!-- <div class="form-field-label">{{ t("translation_language_iso_label") }}</div> -->

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/draft-request-detail.component.html line 76 at r3 (raw file):

            <mat-icon>check_circle</mat-icon> Approve & Enable
          </button>
          <button mat-button color="warn" (click)="deleteRequest()"><mat-icon>delete</mat-icon> Delete Request</button>

This doesn't look like it has an effect.

Code quote:

color="warn"

src/SIL.XForge.Scripture/ClientApp/src/xforge-common/feature-flags/feature-flag.service.ts line 366 at r3 (raw file):

  );

  readonly inAppDraftSignupForm: ObservableFeatureFlag = new FeatureFlagFromStorage(

When adding a new feature flag you should ad it to parse-verse.ts also.

Code quote:

  readonly inAppDraftSignupForm: ObservableFeatureFlag = new FeatureFlagFromStorage(

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/onboarding-requests/onboarding-requests.component.ts line 134 at r3 (raw file):

   * Called after loading all requests or after updating individual requests.
   */
  private initializeRequestData(): void {

I normally expect private functions to come at the end of the file. Can you move these private functions to the end?

Code quote:

  private initializeRequestData(): void {

Nateowami requested a review from Copilot December 11, 2025 21:35

Nateowami added the will require testing PR should not be merged until testers confirm testing is complete label Dec 11, 2025

github-advanced-security bot found potential problems Dec 11, 2025

View reviewed changes

...entApp/src/app/translate/draft-generation/draft-apply-dialog/draft-apply-dialog.component.ts Fixed Show fixed Hide fixed

...entApp/src/app/translate/draft-generation/draft-apply-dialog/draft-apply-dialog.component.ts Fixed Show fixed Hide fixed

Nateowami force-pushed the feature/integrated-onboarding-form branch from 644e5f9 to 6aceae5 Compare December 11, 2025 21:39

Copilot started reviewing on behalf of Nateowami December 11, 2025 21:40 View session

Nateowami force-pushed the feature/integrated-onboarding-form branch from 6aceae5 to b64c163 Compare December 11, 2025 21:42

Copilot AI reviewed Dec 11, 2025

View reviewed changes

github-advanced-security bot found potential problems Dec 11, 2025

View reviewed changes

Integrate onboarding form

e612e74

Nateowami force-pushed the feature/integrated-onboarding-form branch from b64c163 to e612e74 Compare December 11, 2025 22:19

RaymondLuong3 self-assigned this Jan 2, 2026

RaymondLuong3 reviewed Jan 2, 2026

View reviewed changes

@@ -94,7 +94,7 @@
                                 await emailService.SendEmailAsync(email, subject, body);
                             }
                         }
-                        catch (Exception exception)
+                        catch (Exception exception) when (!IsCriticalException(exception))
                         {
                             _exceptionHandler.RecordEndpointInfoForException(
                                 new Dictionary<string, string>
@@ -153,6 +153,21 @@
                             }
                         }
+                /// <summary>
+                /// Determines whether the exception is critical and should not be caught.
+                /// </summary>
+                private static bool IsCriticalException(Exception ex)
+                {
+                    return ex is OutOfMemoryException
+                        || ex is StackOverflowException
+                        || ex is AccessViolationException
+                        || ex is AppDomainUnloadedException
+                        || ex is BadImageFormatException
+                        || ex is CannotUnloadAppDomainException
+                        || ex is InvalidProgramException
+                        || ex is ThreadAbortException;
+                }
                         return Ok(request.Id);
                     }
                     catch (Exception)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Integrate onboarding form #3612

Integrate onboarding form #3612

Uh oh!

Nateowami commented Dec 11, 2025 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Dec 11, 2025

Uh oh!

Copilot AI Dec 11, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Check notice

Copilot Autofix

codecov bot commented Dec 11, 2025 •

edited

Loading

Uh oh!

RaymondLuong3 left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Integrate onboarding form #3612

Are you sure you want to change the base?

Integrate onboarding form #3612

Uh oh!

Conversation

Nateowami commented Dec 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Enable the feature flag

Fill out the form

View the list of submissions

View/manage an individual submission

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Key Changes

Reviewed changes

Uh oh!

Copilot AI Dec 11, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 11, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Check notice

Copilot Autofix

codecov bot commented Dec 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

❌ 2 Tests Failed:

Uh oh!

RaymondLuong3 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Nateowami commented Dec 11, 2025 •

edited

Loading

codecov bot commented Dec 11, 2025 •

edited

Loading