SF-3818 Allow Serval admins to configure sources

[Nateowami]

This change is 

[codecov]

Codecov Report

❌ Patch coverage is 86.74699% with 22 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.90%. Comparing base (eec50dc) to head (7217272).
⚠️ Report is 7 commits behind head on master.
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
...e.Scripture/Controllers/SFProjectsRpcController.cs	74.19%	8 Missing ⚠️
...request-dialog/approve-request-dialog.component.ts	86.36%	3 Missing and 3 partials ⚠️
...uest-detail/onboarding-request-detail.component.ts	88.63%	4 Missing and 1 partial ⚠️
.../SIL.XForge.Scripture/Services/SFProjectService.cs	95.55%	1 Missing and 1 partial ⚠️
...pture/ClientApp/src/app/core/sf-project.service.ts	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3922      +/-   ##
==========================================
+ Coverage   80.66%   80.90%   +0.24%     
==========================================
  Files         634      635       +1     
  Lines       41090    41161      +71     
  Branches     6681     6679       -2     
==========================================
+ Hits        33144    33303     +159     
+ Misses       6893     6816      -77     
+ Partials     1053     1042      -11     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

[marksvc]

This is reviewable in Devin Review.

[marksvc]

Thank you for your work on this.

@marksvc reviewed 14 files and all commit messages, and made 31 comments.
Reviewable status: all files reviewed, 30 unresolved discussions (waiting on Nateowami).

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/onboarding-request-detail/onboarding-request-detail.component.html line 80 at r2 (raw file):
We can change "Configure Sources" to "configure sources" to conform to

All text, including titles, headings, labels, menu items, navigation components, app bars, and buttons should use sentence-style capitalization.

https://m3.material.io/foundations/content-design/style-guide/ux-writing-best-practices

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/onboarding-request-detail/approve-request-dialog/approve-request-dialog.component.ts line 94 at r2 (raw file):
To help me when I come wonder what this class is in the future, can we write a comment on it that tips me off for what its purpose is and where its place is in the system? How about something like

Dialog for Serval admins to apply final configuration and approval to a draft generation onboarding request.

---

src/SIL.XForge.Scripture/Services/SFProjectService.cs line 1289 at r2 (raw file):
You might consider modifying this to point a bit more to communicating that the project wasn't found in SF. So that might just be a change to

Source paratext project {paratextId} not found.

While you're at it, Paratext will look a bit nicer capitalized.

---

src/SIL.XForge.Scripture/Services/SFProjectService.cs line 2333 at r2 (raw file):
Hmm. I guess that's here, too. Well, you might consider changing this one as well to

Source Paratext project {paratextId} not found.

---

src/SIL.XForge.Scripture/ClientApp/e2e/workflows/onboarding-flow.ts line 208 at r2 (raw file):

  // Approve the request
  await adminUser.click(adminBrowser.page.getByRole('button', { name: 'Approve & Configure Sources' }));

(If you change the label to sentence case, or & to &, remember to adjust it here, too.)

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/onboarding-request-detail/onboarding-request-detail.component.html line 80 at r2 (raw file):

          @if (!isResolved) {
            <button mat-button color="primary" (click)="approveRequest()">
              <mat-icon>check_circle</mat-icon> Approve & Configure Sources

Looks like this isn't causing a problem, and it wasn't introduced in this PR, but my understanding is that & should be replaced by & here.

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/onboarding-request-detail/onboarding-request-detail.component.ts line 425 at r2 (raw file):

    >(ApproveRequestDialogComponent, { data: dialogData });
    const result = await lastValueFrom(dialogRef.afterClosed());
    if (result == null || this.request == null) return;

Okay. Are you expecting that this.request will go back to undefined sometimes while we are awaiting?

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/onboarding-request-detail/onboarding-request-detail.component.ts line 428 at r2 (raw file):

    this.loadingStarted();
    try {

Devin is concerned that this method is not atomic. It doesn't look to me too bad for us to abruptly stop part-way through the method. It looks like something that can just be tried again.

We aren't too informative with error messaging about what to do if something goes wrong, so that's a potential area for improvement.

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/onboarding-request-detail/onboarding-request-detail.component.ts line 462 at r2 (raw file):

  private buildApproveDialogData(): ApproveRequestDialogData {
    const formData = this.formData;

Whoa, this is getting me confused. We are going to show a form. And to do it, we take the form data and compose a form to solicit data. I was getting confused about whether this method was being called before opening the approve dialog, and preparing its form data, or after closing the approve dialog, and processing its form data.

I gather that this.formData is the non-admin user's form data, and we are preparing a form for an admin user to fill out, from the non-admin user's form data. Can we make this more clear in our naming?

I don't think we need to specify that the user's request form data is even "form data" here, so we might write something like

const onboardingRequestData = this.formData;

I know this whole file is about an onboarding request. But I was getting confused in the interplay between form processing.

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/onboarding-request-detail/onboarding-request-detail.component.ts line 482 at r2 (raw file):

      )
    ]
      .map(toSourceOption)

This line is nice and clean, using the toSourceOption function.

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/onboarding-request-detail/onboarding-request-detail.component.ts line 495 at r2 (raw file):

          name: projectLabel(btData),
          languageCode: btData.writingSystem.tag,
          draftingAlreadyEnabled: btConfig.projectType === ProjectType.BackTranslation || btConfig.preTranslate === true

So if the user's onboarding request form had a back translation project, and that back translation project has a TranslateConfig where the projectType is BackTranslation, then we tell the Serval admin that draftgen is already enabled. Even if the back translation project has preTranslate=false. Can you shed light on what is happening and what is meant?

This would have made more sense to me if the || were &&. Or even if just

draftingAlreadyEnabled: btConfig.preTranslate

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/onboarding-request-detail/onboarding-request-detail.component.ts line 502 at r2 (raw file):

    const trainingSourceOptions =
      backTranslation != null && !draftingSourceOptions.some(o => o.paratextId === backTranslation.paratextId)
        ? [...draftingSourceOptions, backTranslation]

Hmm. Now, I didn't see this discussed in the JIRA issue. So here we are saying that if the user's draft generation onboarding request has a back translation specified, and the user did not specify the back translation as a drafting source, then we will add all the drafting source options (most likely the single drafting source option) into the training sources. So If the user listed a back translation, then we include their drafting source as a training source.

If the user did specify the back translation as a drafting source, we don't add the drafting sources to the training sources.
If the user's request does not have a Back Translation specified, we don't add the drafting sources to the training sources.

Can you explain what the bigger picture is here? I looked on the help site about this but didn't see something that explained.

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/onboarding-request-detail/onboarding-request-detail.component.ts line 515 at r2 (raw file):

      draftingSourceOptions,
      trainingSourceOptions,
      defaultTrainingSource: formData.sourceProjectA,

Hmm, okay, so even if the drafting source is included in the training sources list, we don't set that to the default but we always use A.

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/onboarding-request-detail/onboarding-request-detail.component.ts line 515 at r2 (raw file):

      draftingSourceOptions,
      trainingSourceOptions,
      defaultTrainingSource: formData.sourceProjectA,

Devin mentions that if sourceProjectA was deleted after the onboarding form was submitted, we would end up getting an error on the backend when submitting the Serval Admin approval form. It might be useful to do a check during buildApproveDialogData as to whether each project ID still corresponds to something SF knows about.

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/onboarding-request-detail/onboarding-request-detail.component.spec.ts line 205 at r2 (raw file):

      verify(
        mockedProjectService.onlineSetDraftSources('project01', deepEqual(['ptproject02']), deepEqual(['ptproject01']))

Okay, that's confusing :). project01 and ptproject01 refer to different projects; not the same project. Can you use different identifiers that don't make them as easy to accidentally think they are the same?

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/onboarding-request-detail/approve-request-dialog/approve-request-dialog.component.html line 25 at r2 (raw file):

            {{ option.name }} <span class="language-code">{{ option.languageCode }}</span>
          </mat-checkbox>
          @if (trainingSourceOrder(option.paratextId); as trainingSourceOrder) {

You might consider using a different name for variable trainingSourceOrder (eg order) since that is also the name of a class method.

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/onboarding-request-detail/approve-request-dialog/approve-request-dialog.component.html line 45 at r2 (raw file):

        @if (bt.draftingAlreadyEnabled) {
          Draft generation is already enabled for this project.
        } @else if (backTranslationLanguageMatchesTarget) {

I wonder if this should be an if rather than an else if, so it shows up even if bt.draftingAlreadyEnabled===true. ?

I guess, if BT draftgen is already enabled, and there is a problem, we aren't really going to solve it with this form. But it might be helpful for the message to indicate the situation?

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/onboarding-request-detail/approve-request-dialog/approve-request-dialog.component.scss line 37 at r2 (raw file):

  color: colors.$info-color;
  background-color: #e8f0fe;

For color and background-color, I encourage you to use something like one of the following, rather than hard-code colours.

color: var(--mat-sys-on-primary);
background-color: var(--mat-sys-primary);

color: var(--mat-sys-on-primary-container);
background-color: var(--mat-sys-primary-container);

color: var(--mat-sys-on-secondary);
background-color: var(--mat-sys-secondary);

color: var(--mat-sys-on-secondary-container);
background-color: var(--mat-sys-secondary-container);

There are also a number of background-color shades of grey you can use, such as

var(--mat-sys-surface-container-highest)
var(--mat-sys-surface-container-high)
var(--mat-sys-surface-container)
var(--mat-sys-surface-container-low)
var(--mat-sys-surface-container-lowest)

I find it helpful to reference this page: https://material.angular.dev/guide/theming-your-components

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/onboarding-request-detail/approve-request-dialog/approve-request-dialog.component.ts line 52 at r2 (raw file):

/** Validates that the number of selected training sources is 1 or 2. */
function trainingSourcesValidator(control: AbstractControl): ValidationErrors | null {
  const value: string[] = control.value ?? [];

I see that this isn't a problem for non-arrays, but it does make my type-usage hairs stand up :)
You might choose instead to write something like

 function trainingSourcesValidator(control: AbstractControl): ValidationErrors | null {
-  const value: string[] = control.value ?? [];
-  return value.length >= 1 && value.length <= 2 ? null : { trainingSourceCount: true };
+  const value: unknown = control.value;
+  if (!Array.isArray(value) || value.length < 1 || value.length > 2) return { trainingSourceCount: true };
+  else return null;
 }

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/onboarding-request-detail/approve-request-dialog/approve-request-dialog.component.ts line 60 at r2 (raw file):

    ids
      .map(id => languageCodes.get(id))
      .filter((c): c is string => c != null && c !== '')

Just a reminder that you can write this line with the type-utils.ts helper, as

-      .filter((c): c is string => c != null && c !== '')
+      .filter(isPopulatedString)

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/onboarding-request-detail/approve-request-dialog/approve-request-dialog.component.ts line 70 at r2 (raw file):

    const draftingSourceId: string = control.get('draftingSource')?.value;
    const trainingSourceIds: string[] = control.get('trainingSources')?.value ?? [];
    const allIds = [draftingSourceId, ...trainingSourceIds].filter((id): id is string => id != null);

Just a reminder that you can write this line with another type-utils.ts helper, as

-    const allIds = [draftingSourceId, ...trainingSourceIds].filter((id): id is string => id != null);
+    const allIds = [draftingSourceId, ...trainingSourceIds].filter(isString);

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/onboarding-request-detail/approve-request-dialog/approve-request-dialog.component.ts line 128 at r2 (raw file):

      { validators: languageCodesValidator(this.languageCodes) }
    );
    this.enableBackTranslationDrafting = new FormControl<boolean>(!this.backTranslationLanguageMatchesTarget, {

I'm finding the control, the value, the meaning, and the application of "enable BT" to be frustrating on this form :-).
We're sort of overloading by combining "Is BT draft generation enabled?" and "Do you want to enable BT draft generation?", and going as far as setting this.enableBackTranslationDrafting control to true when there is no back translation (and then hiding the control and ignoring its value).

I did some thinking about this for a bit. Suppose we think of the control as describing whether back generation drafting is enabled, whether or not the admin can toggle its enablement. And we can separate the considerations of "Is BT draftgen enabled?" and "Should this dialog cause BT draftgen to be enabled?". With code like the following:

# approve-request-dialog.component.html
@@ -35,8 +35,8 @@
     </div>
     @if (data.backTranslation; as bt) {
       <h3>Back translation project</h3>
-      <mat-checkbox [formControl]="enableBackTranslationDrafting">
-        Enable draft generation for {{ bt.name }}
+      <mat-checkbox [formControl]="backTranslationDraftingEnabled">
+        Draft generation enabled for {{ bt.name }}
         <span class="language-code">{{ bt.languageCode }}</span>
       </mat-checkbox>
       <p class="hint">
# approve-request-dialog.component.ts
@@ -95,7 +95,7 @@ export class ApproveRequestDialogComponent {
   readonly draftingSource: FormControl<string>;
   readonly trainingSources: FormControl<string[]>;
   readonly form: FormGroup;
-  readonly enableBackTranslationDrafting: FormControl<boolean>;
+  readonly backTranslationDraftingEnabled: FormControl<boolean>;
   readonly backTranslationLanguageMatchesTarget: boolean;
   private readonly languageCodes: Map<string, string>;
   private readonly normalizedTargetLanguageCode: string;
@@ -125,15 +125,19 @@ export class ApproveRequestDialogComponent {
       { draftingSource: this.draftingSource, trainingSources: this.trainingSources },
       { validators: languageCodesValidator(this.languageCodes) }
     );
-    this.enableBackTranslationDrafting = new FormControl<boolean>(!this.backTranslationLanguageMatchesTarget, {
-      nonNullable: true
-    });
-    if (!this.canEnableBackTranslationDrafting) {
-      this.enableBackTranslationDrafting.disable();
+    this.backTranslationDraftingEnabled = new FormControl<boolean>(
+      this.data.backTranslation != null &&
+        (this.data.backTranslation.draftingAlreadyEnabled || !this.backTranslationLanguageMatchesTarget),
+      {
+        nonNullable: true
+      }
+    );
+    if (!this.canToggleBackTranslationDrafting) {
+      this.backTranslationDraftingEnabled.disable();
     }
   }
 
-  get canEnableBackTranslationDrafting(): boolean {
+  get canToggleBackTranslationDrafting(): boolean {
     return (
       this.data.backTranslation != null &&
       !this.data.backTranslation.draftingAlreadyEnabled &&
@@ -168,10 +172,12 @@ export class ApproveRequestDialogComponent {
 
   approve(): void {
     if (this.form.valid) {
+      const backTranslationDraftingNewlyEnabled: boolean =
+        this.canToggleBackTranslationDrafting && this.backTranslationDraftingEnabled.value;
       this.dialogRef.close({
         draftingSourceParatextId: this.draftingSource.value,
         trainingSourceParatextIds: this.trainingSources.value,
-        enableBackTranslationDrafting: this.canEnableBackTranslationDrafting && this.enableBackTranslationDrafting.value
+        enableBackTranslationDrafting: backTranslationDraftingNewlyEnabled
       });
     }
   }

This comment is non-blocking. If the overloaded meaning bothers you too, the above might be a solution.

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/onboarding-request-detail/approve-request-dialog/approve-request-dialog.component.ts line 136 at r2 (raw file):

  }

  get canEnableBackTranslationDrafting(): boolean {

I wonder if this would be better named canToggleBackTranslationDrafting. It's weird that the control might be checked and yet canEnable..is false. But, I get the idea, and canEnable.. and canToggle.. are considering different aspects of the situation.

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/onboarding-request-detail/approve-request-dialog/approve-request-dialog.component.ts line 144 at r2 (raw file):

  }

  get sourcesMatchTargetLanguage(): boolean {

Okay. I am not thrilled that this is called sourcesMatchTargetLanguage and returns false if codes.size !== 1. Except I see that its warning message is shown below an error if any source language codes differ.

An alternate way to implement this could be

# approve-request-dialog/approve-request-dialog.component.html
     }
     @if (form.hasError("languageCodesDiffer")) {
       <app-notice type="error" mode="outline">All source projects must be in the same language.</app-notice>
-    }
-    @if (sourcesMatchTargetLanguage) {
+    } @else if (sourceMatchesTargetLanguage) {
       <app-notice type="warning" mode="outline">
-        Sources are in the same language as the target project. This may be a mistake.
+        One or more sources are in the same language as the target project. This may be a mistake.
       </app-notice>
     }
     @if (trainingSources.hasError("trainingSourceCount")) {

# approve-request-dialog/approve-request-dialog.component.ts
     );
   }
 
-  get sourcesMatchTargetLanguage(): boolean {
+  get sourceMatchesTargetLanguage(): boolean {
     const allIds = [this.draftingSource.value, ...this.trainingSources.value].filter(id => id !== '');
     const codes = uniqueNormalizedCodes(allIds, this.languageCodes);
-    if (codes.size !== 1) return false;
     return codes.has(this.normalizedTargetLanguageCode);
   }

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/onboarding-request-detail/approve-request-dialog/approve-request-dialog.component.ts line 165 at r2 (raw file):

  toggleTrainingSource(id: string, checked: boolean): void {
    const current = this.trainingSources.value;
    this.trainingSources.setValue(checked ? [...current, id] : current.filter(s => s !== id));

Clever.

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/onboarding-request-detail/approve-request-dialog/approve-request-dialog.component.spec.ts line 26 at r2 (raw file):

};

const BT_NEEDS_ENABLING_DATA: ApproveRequestDialogData = {

I like how you have these data cases defined.

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/onboarding-request-detail/approve-request-dialog/approve-request-dialog.component.spec.ts line 185 at r2 (raw file):

      setup();
      fixture.detectChanges();
      expect(fixture.nativeElement.querySelector('h3 ~ mat-checkbox')).toBeNull();

Can we change this query selector so that (1) it is more obvious that it is correct, and (2) so it is more resilient to the form being rearranged?

---

src/SIL.XForge.Scripture/Services/SFProjectService.cs line 1310 at r2 (raw file):

        List<TranslateSource> draftingSources = [.. draftingIds.Select(BuildSource)];
        List<TranslateSource> trainingSources = [.. trainingIds.Select(BuildSource)];

Nice transforming.

---

src/SIL.XForge.Scripture/Services/SFProjectService.cs line 1324 at r2 (raw file):

        foreach (string sourceProjectRef in processedSourceRefs)
        {
            _backgroundJobClient.Enqueue<ISFProjectService>(r => r.SyncUserRoleAsync(curUserId, sourceProjectRef));

You're syncing the Serval admin user role with this, is that correct and what you intend?
And even if that's a mistake and you're meaning to use the admin or translator with this line, can you explain why we are doing this? Mightn't something like this already have happened when the user was working on their onboarding request? (Unless we are concerned that it got stale by the time the Serval admin processed the request?)

---

test/SIL.XForge.Scripture.Tests/Services/SFProjectServiceTests.cs line 4048 at r2 (raw file):

        // Verify Paratext credential lookup was never needed
        _ = env.ParatextService.DidNotReceive().GetProjectsAsync(Arg.Any<UserSecret>());

Can you explain a bit more about why this is being tested? Oh, maybe this is an artifact of the AI following a specific instruction or changing from one thing to another, but might not make sense on its own? I see the test title includes "FromDatabases".

[Nateowami]

Thanks for your thorough review.

@Nateowami partially reviewed 13 files, made 27 comments, and resolved 9 discussions.
Reviewable status: all files reviewed, 21 unresolved discussions (waiting on marksvc).

---

src/SIL.XForge.Scripture/ClientApp/e2e/workflows/onboarding-flow.ts line 208 at r2 (raw file):

Previously, marksvc wrote…

(If you change the label to sentence case, or & to &, remember to adjust it here, too.)

👍

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/onboarding-request-detail/onboarding-request-detail.component.html line 80 at r2 (raw file):

Previously, marksvc wrote…

Looks like this isn't causing a problem, and it wasn't introduced in this PR, but my understanding is that & should be replaced by & here.

I don't think it's a problem.

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/onboarding-request-detail/onboarding-request-detail.component.html line 80 at r2 (raw file):

Previously, marksvc wrote…

We can change "Configure Sources" to "configure sources" to conform to

All text, including titles, headings, labels, menu items, navigation components, app bars, and buttons should use sentence-style capitalization.

https://m3.material.io/foundations/content-design/style-guide/ux-writing-best-practices

This is not a rule that we have been following in SF (look around and I find more often we go the other way), and I don't think this is the right place to propose such a style change. If we want to make such a style decision it should be discussed, documented, and changes made systematically.

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/onboarding-request-detail/onboarding-request-detail.component.ts line 425 at r2 (raw file):

Previously, marksvc wrote…

Okay. Are you expecting that this.request will go back to undefined sometimes while we are awaiting?

No, it's just logically possible

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/onboarding-request-detail/onboarding-request-detail.component.ts line 428 at r2 (raw file):

Previously, marksvc wrote…

Devin is concerned that this method is not atomic. It doesn't look to me too bad for us to abruptly stop part-way through the method. It looks like something that can just be tried again.

We aren't too informative with error messaging about what to do if something goes wrong, so that's a potential area for improvement.

I've seen that concern from Devin, and it doesn't concern me. Operations are done in a logical order such that the onboarding request is only marked approved once everything succeeds. Project is activated only if sources first get set correctly. So yes, it may fail part way through, but atomicity was not a goal.

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/onboarding-request-detail/onboarding-request-detail.component.ts line 462 at r2 (raw file):

Previously, marksvc wrote…

Whoa, this is getting me confused. We are going to show a form. And to do it, we take the form data and compose a form to solicit data. I was getting confused about whether this method was being called before opening the approve dialog, and preparing its form data, or after closing the approve dialog, and processing its form data.

I gather that this.formData is the non-admin user's form data, and we are preparing a form for an admin user to fill out, from the non-admin user's form data. Can we make this more clear in our naming?

I don't think we need to specify that the user's request form data is even "form data" here, so we might write something like

const onboardingRequestData = this.formData;

I know this whole file is about an onboarding request. But I was getting confused in the interplay between form processing.

An onboarding request consists of an initial submission, most of which is the data the user submitted in the form. It also has lots of metadata and comments. The form data is just a part of the onboarding request. I've updated the variable name.

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/onboarding-request-detail/onboarding-request-detail.component.ts line 495 at r2 (raw file):

Previously, marksvc wrote…

So if the user's onboarding request form had a back translation project, and that back translation project has a TranslateConfig where the projectType is BackTranslation, then we tell the Serval admin that draftgen is already enabled. Even if the back translation project has preTranslate=false. Can you shed light on what is happening and what is meant?

This would have made more sense to me if the || were &&. Or even if just

draftingAlreadyEnabled: btConfig.preTranslate

The logic is correct. I've refactored it to a helper function to make it clearer. Back translation projects get to generate drafts even if not approved.

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/onboarding-request-detail/onboarding-request-detail.component.ts line 502 at r2 (raw file):

Previously, marksvc wrote…

Hmm. Now, I didn't see this discussed in the JIRA issue. So here we are saying that if the user's draft generation onboarding request has a back translation specified, and the user did not specify the back translation as a drafting source, then we will add all the drafting source options (most likely the single drafting source option) into the training sources. So If the user listed a back translation, then we include their drafting source as a training source.

If the user did specify the back translation as a drafting source, we don't add the drafting sources to the training sources.
If the user's request does not have a Back Translation specified, we don't add the drafting sources to the training sources.

Can you explain what the bigger picture is here? I looked on the help site about this but didn't see something that explained.

A user isn't going to list a back translation as a possible training source, but we do want to consider using it as such.

We're basically compiling all projects the user specified as possible training sources and avoiding duplicates.

This entire change is about mapping from the form to the selection options.

None of this will make much sense without familiarity with the onboarding form. Instead of writing a long explanation, can you look at that form first? If I still need to explain I'm happy to. It's mapping what the user provided to what options make logical sense.

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/onboarding-request-detail/onboarding-request-detail.component.ts line 515 at r2 (raw file):

Previously, marksvc wrote…

Hmm, okay, so even if the drafting source is included in the training sources list, we don't set that to the default but we always use A.

It's a consequence of what questions get asked in the form. We basically ask what you would use for drafting source, but allow three selections for possible training sources (though we don't use that term in the form)

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/onboarding-request-detail/onboarding-request-detail.component.ts line 515 at r2 (raw file):

Previously, marksvc wrote…

Devin mentions that if sourceProjectA was deleted after the onboarding form was submitted, we would end up getting an error on the backend when submitting the Serval Admin approval form. It might be useful to do a check during buildApproveDialogData as to whether each project ID still corresponds to something SF knows about.

I think it would probably be visible in the UI that something was wrong. Or a non-fatal error when it just can't set the sources.

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/onboarding-request-detail/onboarding-request-detail.component.spec.ts line 205 at r2 (raw file):

Previously, marksvc wrote…

Okay, that's confusing :). project01 and ptproject01 refer to different projects; not the same project. Can you use different identifiers that don't make them as easy to accidentally think they are the same?

Updated to role-based id names

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/onboarding-request-detail/approve-request-dialog/approve-request-dialog.component.html line 25 at r2 (raw file):

Previously, marksvc wrote…

You might consider using a different name for variable trainingSourceOrder (eg order) since that is also the name of a class method.

Done.

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/onboarding-request-detail/approve-request-dialog/approve-request-dialog.component.html line 45 at r2 (raw file):

Previously, marksvc wrote…

I wonder if this should be an if rather than an else if, so it shows up even if bt.draftingAlreadyEnabled===true. ?

I guess, if BT draftgen is already enabled, and there is a problem, we aren't really going to solve it with this form. But it might be helpful for the message to indicate the situation?

The onboarding request page already has this warning when relevant:
The language code specified in the back translation project (${backTranslationProjectIsoCode}) is equivalent to the project language code (${projectIsoCode}).

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/onboarding-request-detail/approve-request-dialog/approve-request-dialog.component.scss line 37 at r2 (raw file):

Previously, marksvc wrote…

For color and background-color, I encourage you to use something like one of the following, rather than hard-code colours.

color: var(--mat-sys-on-primary);
background-color: var(--mat-sys-primary);

color: var(--mat-sys-on-primary-container);
background-color: var(--mat-sys-primary-container);

color: var(--mat-sys-on-secondary);
background-color: var(--mat-sys-secondary);

color: var(--mat-sys-on-secondary-container);
background-color: var(--mat-sys-secondary-container);

There are also a number of background-color shades of grey you can use, such as

var(--mat-sys-surface-container-highest)
var(--mat-sys-surface-container-high)
var(--mat-sys-surface-container)
var(--mat-sys-surface-container-low)
var(--mat-sys-surface-container-lowest)

I find it helpful to reference this page: https://material.angular.dev/guide/theming-your-components

I like re-using colors, though I prefer using SF-specific colors like src/SIL.XForge.Scripture/ClientApp/src/_variables.scss when possible, rather than Material.

Do you have specific colors to suggest?

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/onboarding-request-detail/approve-request-dialog/approve-request-dialog.component.ts line 52 at r2 (raw file):

Previously, marksvc wrote…

I see that this isn't a problem for non-arrays, but it does make my type-usage hairs stand up :)
You might choose instead to write something like

 function trainingSourcesValidator(control: AbstractControl): ValidationErrors | null {
-  const value: string[] = control.value ?? [];
-  return value.length >= 1 && value.length <= 2 ? null : { trainingSourceCount: true };
+  const value: unknown = control.value;
+  if (!Array.isArray(value) || value.length < 1 || value.length > 2) return { trainingSourceCount: true };
+  else return null;
 }

Added guard.

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/onboarding-request-detail/approve-request-dialog/approve-request-dialog.component.ts line 60 at r2 (raw file):

Previously, marksvc wrote…

Just a reminder that you can write this line with the type-utils.ts helper, as

-      .filter((c): c is string => c != null && c !== '')
+      .filter(isPopulatedString)

Done.

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/onboarding-request-detail/approve-request-dialog/approve-request-dialog.component.ts line 70 at r2 (raw file):

Previously, marksvc wrote…

Just a reminder that you can write this line with another type-utils.ts helper, as

-    const allIds = [draftingSourceId, ...trainingSourceIds].filter((id): id is string => id != null);
+    const allIds = [draftingSourceId, ...trainingSourceIds].filter(isString);

Done.

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/onboarding-request-detail/approve-request-dialog/approve-request-dialog.component.ts line 94 at r2 (raw file):

Previously, marksvc wrote…

To help me when I come wonder what this class is in the future, can we write a comment on it that tips me off for what its purpose is and where its place is in the system? How about something like

Dialog for Serval admins to apply final configuration and approval to a draft generation onboarding request.

Done.

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/onboarding-request-detail/approve-request-dialog/approve-request-dialog.component.ts line 128 at r2 (raw file):

Previously, marksvc wrote…

I'm finding the control, the value, the meaning, and the application of "enable BT" to be frustrating on this form :-).
We're sort of overloading by combining "Is BT draft generation enabled?" and "Do you want to enable BT draft generation?", and going as far as setting this.enableBackTranslationDrafting control to true when there is no back translation (and then hiding the control and ignoring its value).

I did some thinking about this for a bit. Suppose we think of the control as describing whether back generation drafting is enabled, whether or not the admin can toggle its enablement. And we can separate the considerations of "Is BT draftgen enabled?" and "Should this dialog cause BT draftgen to be enabled?". With code like the following:

# approve-request-dialog.component.html
@@ -35,8 +35,8 @@
     </div>
     @if (data.backTranslation; as bt) {
       <h3>Back translation project</h3>
-      <mat-checkbox [formControl]="enableBackTranslationDrafting">
-        Enable draft generation for {{ bt.name }}
+      <mat-checkbox [formControl]="backTranslationDraftingEnabled">
+        Draft generation enabled for {{ bt.name }}
         <span class="language-code">{{ bt.languageCode }}</span>
       </mat-checkbox>
       <p class="hint">
# approve-request-dialog.component.ts
@@ -95,7 +95,7 @@ export class ApproveRequestDialogComponent {
   readonly draftingSource: FormControl<string>;
   readonly trainingSources: FormControl<string[]>;
   readonly form: FormGroup;
-  readonly enableBackTranslationDrafting: FormControl<boolean>;
+  readonly backTranslationDraftingEnabled: FormControl<boolean>;
   readonly backTranslationLanguageMatchesTarget: boolean;
   private readonly languageCodes: Map<string, string>;
   private readonly normalizedTargetLanguageCode: string;
@@ -125,15 +125,19 @@ export class ApproveRequestDialogComponent {
       { draftingSource: this.draftingSource, trainingSources: this.trainingSources },
       { validators: languageCodesValidator(this.languageCodes) }
     );
-    this.enableBackTranslationDrafting = new FormControl<boolean>(!this.backTranslationLanguageMatchesTarget, {
-      nonNullable: true
-    });
-    if (!this.canEnableBackTranslationDrafting) {
-      this.enableBackTranslationDrafting.disable();
+    this.backTranslationDraftingEnabled = new FormControl<boolean>(
+      this.data.backTranslation != null &&
+        (this.data.backTranslation.draftingAlreadyEnabled || !this.backTranslationLanguageMatchesTarget),
+      {
+        nonNullable: true
+      }
+    );
+    if (!this.canToggleBackTranslationDrafting) {
+      this.backTranslationDraftingEnabled.disable();
     }
   }
 
-  get canEnableBackTranslationDrafting(): boolean {
+  get canToggleBackTranslationDrafting(): boolean {
     return (
       this.data.backTranslation != null &&
       !this.data.backTranslation.draftingAlreadyEnabled &&
@@ -168,10 +172,12 @@ export class ApproveRequestDialogComponent {
 
   approve(): void {
     if (this.form.valid) {
+      const backTranslationDraftingNewlyEnabled: boolean =
+        this.canToggleBackTranslationDrafting && this.backTranslationDraftingEnabled.value;
       this.dialogRef.close({
         draftingSourceParatextId: this.draftingSource.value,
         trainingSourceParatextIds: this.trainingSources.value,
-        enableBackTranslationDrafting: this.canEnableBackTranslationDrafting && this.enableBackTranslationDrafting.value
+        enableBackTranslationDrafting: backTranslationDraftingNewlyEnabled
       });
     }
   }

This comment is non-blocking. If the overloaded meaning bothers you too, the above might be a solution.

You're right we're slightly overloading the form field, but I don't think your suggestion addresses that.

Current logic:

• Most of the time it's an editable field that reflects its name
• In a limited situation, it's set to show that it's actually already enabled and disable the field. In this situation, no action is taken; it's dead-end, and we need to remember not to mistakenly try to interpret it as needing to enable. The dead-end edge case needs to stay dead-end.

Proposed logic:

• Have a field that says backTranslationDraftingEnabled even when it isn't
• The name is wrong in the normal situation where the value does affect what we do

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/onboarding-request-detail/approve-request-dialog/approve-request-dialog.component.ts line 136 at r2 (raw file):

Previously, marksvc wrote…

I wonder if this would be better named canToggleBackTranslationDrafting. It's weird that the control might be checked and yet canEnable..is false. But, I get the idea, and canEnable.. and canToggle.. are considering different aspects of the situation.

I really think the current name reflects the logic well. In no case does this dialog let you toggle back translation drafting; it only lets you enable it by toggling the field.

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/onboarding-request-detail/approve-request-dialog/approve-request-dialog.component.ts line 144 at r2 (raw file):

Previously, marksvc wrote…

Okay. I am not thrilled that this is called sourcesMatchTargetLanguage and returns false if codes.size !== 1. Except I see that its warning message is shown below an error if any source language codes differ.

An alternate way to implement this could be

# approve-request-dialog/approve-request-dialog.component.html
     }
     @if (form.hasError("languageCodesDiffer")) {
       <app-notice type="error" mode="outline">All source projects must be in the same language.</app-notice>
-    }
-    @if (sourcesMatchTargetLanguage) {
+    } @else if (sourceMatchesTargetLanguage) {
       <app-notice type="warning" mode="outline">
-        Sources are in the same language as the target project. This may be a mistake.
+        One or more sources are in the same language as the target project. This may be a mistake.
       </app-notice>
     }
     @if (trainingSources.hasError("trainingSourceCount")) {

# approve-request-dialog/approve-request-dialog.component.ts
     );
   }
 
-  get sourcesMatchTargetLanguage(): boolean {
+  get sourceMatchesTargetLanguage(): boolean {
     const allIds = [this.draftingSource.value, ...this.trainingSources.value].filter(id => id !== '');
     const codes = uniqueNormalizedCodes(allIds, this.languageCodes);
-    if (codes.size !== 1) return false;
     return codes.has(this.normalizedTargetLanguageCode);
   }

This logic matches ConfirmSourcesComponent and is deliberate.

If there are multiple source languages the question of whether they match the target language is both nonsense and moot, because an error is shown. Having multiple source languages is not allowed, ever.

If there is a single source language, we then need to consider whether it's identical to the target. And if it is, warn, because this is a common mistake, but also a supported use-case.

The logic might look cleaner if the logic for the error and warning were in the same method.

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/onboarding-request-detail/approve-request-dialog/approve-request-dialog.component.spec.ts line 185 at r2 (raw file):

Previously, marksvc wrote…

Can we change this query selector so that (1) it is more obvious that it is correct, and (2) so it is more resilient to the form being rearranged?

Good point. Changed to a getter. Required test refactor. Also swapped to using ts-mockito instead of jasmine spy.

---

src/SIL.XForge.Scripture/Services/SFProjectService.cs line 1289 at r2 (raw file):

Previously, marksvc wrote…

You might consider modifying this to point a bit more to communicating that the project wasn't found in SF. So that might just be a change to

Source paratext project {paratextId} not found.

While you're at it, Paratext will look a bit nicer capitalized.

Done.

---

src/SIL.XForge.Scripture/Services/SFProjectService.cs line 1324 at r2 (raw file):

Previously, marksvc wrote…

You're syncing the Serval admin user role with this, is that correct and what you intend?
And even if that's a mistake and you're meaning to use the admin or translator with this line, can you explain why we are doing this? Mightn't something like this already have happened when the user was working on their onboarding request? (Unless we are concerned that it got stale by the time the Serval admin processed the request?)

Thank you for catching this. I originally tried to update the configure settings endpiont, but decided that was overly complicated and made a custom endpoint. I believe in the process this loop was ported over by mistake. Removed.

---

src/SIL.XForge.Scripture/Services/SFProjectService.cs line 2333 at r2 (raw file):

Previously, marksvc wrote…

Hmm. I guess that's here, too. Well, you might consider changing this one as well to

Source Paratext project {paratextId} not found.

Done.

---

test/SIL.XForge.Scripture.Tests/Services/SFProjectServiceTests.cs line 4048 at r2 (raw file):

Previously, marksvc wrote…

Can you explain a bit more about why this is being tested? Oh, maybe this is an artifact of the AI following a specific instruction or changing from one thing to another, but might not make sense on its own? I see the test title includes "FromDatabases".

It's verifying that the endpoing works even if you're not a member of the project. That it's not looking to Paratext for anything, only the database. It's a common mistake (and one I made) for us to make a Serval-admin feature that only works if you're actually a member of the project, and not notice because as devs we're always members of our test projects.

[marksvc]

I left a few things to talk about on a call.

@marksvc reviewed 7 files and all commit messages, made 9 comments, and resolved 13 discussions.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on Nateowami).

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/onboarding-request-detail/onboarding-request-detail.component.html line 80 at r2 (raw file):

Previously, Nateowami wrote…

This is not a rule that we have been following in SF (look around and I find more often we go the other way), and I don't think this is the right place to propose such a style change. If we want to make such a style decision it should be discussed, documented, and changes made systematically.

Okay. I might have characterized it as "We use Sentence case, but aren't very uniform and have many instances where we have not used it." Thank you.

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/onboarding-request-detail/onboarding-request-detail.component.spec.ts line 205 at r2 (raw file):

Previously, Nateowami wrote…

Updated to role-based id names

Oh, that's better; thank you.

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/onboarding-request-detail/approve-request-dialog/approve-request-dialog.component.scss line 37 at r2 (raw file):

Previously, Nateowami wrote…

I like re-using colors, though I prefer using SF-specific colors like src/SIL.XForge.Scripture/ClientApp/src/_variables.scss when possible, rather than Material.

Do you have specific colors to suggest?

Oh, interesting. I'll plan to ask you more about that.

The background-color you have here is somewhat similar to _default.scss neutral 94. Other than --mat--foo I don't think I have a good suggestion other than starting a _foo-theme.scss file.

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/onboarding-request-detail/approve-request-dialog/approve-request-dialog.component.ts line 128 at r2 (raw file):

Previously, Nateowami wrote…

You're right we're slightly overloading the form field, but I don't think your suggestion addresses that.

Current logic:

• Most of the time it's an editable field that reflects its name
• In a limited situation, it's set to show that it's actually already enabled and disable the field. In this situation, no action is taken; it's dead-end, and we need to remember not to mistakenly try to interpret it as needing to enable. The dead-end edge case needs to stay dead-end.

Proposed logic:

• Have a field that says backTranslationDraftingEnabled even when it isn't
• The name is wrong in the normal situation where the value does affect what we do

Hmm. Tricky. One of the things that bothers me in the current implementation is setting the enableBackTranslationDrafting control to true when there is no back translation. But, we don't need to figure out a prefect solution.

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/onboarding-request-detail/approve-request-dialog/approve-request-dialog.component.ts line 144 at r2 (raw file):

Previously, Nateowami wrote…

This logic matches ConfirmSourcesComponent and is deliberate.

If there are multiple source languages the question of whether they match the target language is both nonsense and moot, because an error is shown. Having multiple source languages is not allowed, ever.

If there is a single source language, we then need to consider whether it's identical to the target. And if it is, warn, because this is a common mistake, but also a supported use-case.

The logic might look cleaner if the logic for the error and warning were in the same method.

As I study this more, I see I am missing that codes is a set of languages codes without duplicates, and I don't have an issue with it now.

---

test/SIL.XForge.Scripture.Tests/Services/SFProjectServiceTests.cs line 4048 at r2 (raw file):

Previously, Nateowami wrote…

It's verifying that the endpoing works even if you're not a member of the project. That it's not looking to Paratext for anything, only the database. It's a common mistake (and one I made) for us to make a Serval-admin feature that only works if you're actually a member of the project, and not notice because as devs we're always members of our test projects.

Okay. Thank you for explaining.

---

src/SIL.XForge.Scripture/Services/SFProjectService.cs line 2331 at r3 (raw file):

            resources.SingleOrDefault(r => r.ParatextId == paratextId)
            ?? throw new DataNotFoundException(
                $"Specified source Paratext project {paratextId} not found in Scripture Forge."

I'm sorry. I was wrong about this one. The usages I'm seeing that call GetTranslateSourceAsync are passing a list of paratext projects and resources that are acquired from querying remote servers (eg ParatextService.GetProjectsAsync and SFInstallableDblResource.GetInstallableDblResources) . So not-found here means that the project was not in a list returned from a remote.

So, although it isn't the best error message to say that the project doesn't exist,, it's worse for the error to say that it wasn't found in Scripture Forge, because that's not relevant.

Let's revert this back to $"The source paratext project {paratextId} does not exist.". Sorry :)

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-utils.ts line 85 at r3 (raw file):

/**
 * Checks if drafting is enabled for the given translate configuration. It's enabled if it's been approved
 * (preTranslate is set to true), or if it's a backtranslation.

That is helpful; thank you.

[Nateowami]

@Nateowami made 3 comments and resolved 6 discussions.
Reviewable status: 11 of 15 files reviewed, 4 unresolved discussions (waiting on marksvc).

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/onboarding-request-detail/approve-request-dialog/approve-request-dialog.component.scss line 37 at r2 (raw file):

Previously, marksvc wrote…

Oh, interesting. I'll plan to ask you more about that.

The background-color you have here is somewhat similar to _default.scss neutral 94. Other than --mat--foo I don't think I have a good suggestion other than starting a _foo-theme.scss file.

What about background-color: rgba(colors.$info-color, 0.12);?

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/onboarding-request-detail/approve-request-dialog/approve-request-dialog.component.ts line 128 at r2 (raw file):

setting the enableBackTranslationDrafting control to true when there is no back translation

Oh, that's a good point. I could change that if you want; just set to false in that situation. Or set it to canEnableBackTranslationDrafting? Then it would be un-checked when drafting is already enabled, which might not be very intuitive.

---

src/SIL.XForge.Scripture/Services/SFProjectService.cs line 2331 at r3 (raw file):

Previously, marksvc wrote…

I'm sorry. I was wrong about this one. The usages I'm seeing that call GetTranslateSourceAsync are passing a list of paratext projects and resources that are acquired from querying remote servers (eg ParatextService.GetProjectsAsync and SFInstallableDblResource.GetInstallableDblResources) . So not-found here means that the project was not in a list returned from a remote.

So, although it isn't the best error message to say that the project doesn't exist,, it's worse for the error to say that it wasn't found in Scripture Forge, because that's not relevant.

Let's revert this back to $"The source paratext project {paratextId} does not exist.". Sorry :)

Good catch . Reverted.

[marksvc]

@marksvc made 2 comments.
Reviewable status: 11 of 15 files reviewed, 4 unresolved discussions (waiting on Nateowami).

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/onboarding-request-detail/approve-request-dialog/approve-request-dialog.component.scss line 37 at r2 (raw file):

Previously, Nateowami wrote…

What about background-color: rgba(colors.$info-color, 0.12);?

Good, that makes it centrally controlled.

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/onboarding-request-detail/approve-request-dialog/approve-request-dialog.component.ts line 128 at r2 (raw file):

Previously, Nateowami wrote…

setting the enableBackTranslationDrafting control to true when there is no back translation

Oh, that's a good point. I could change that if you want; just set to false in that situation. Or set it to canEnableBackTranslationDrafting? Then it would be un-checked when drafting is already enabled, which might not be very intuitive.

Yeah, that would help the situation some I think. I experimented with this again. A short solution is

# approve-request-dialog.component.ts
@@ -127,9 +127,13 @@ export class ApproveRequestDialogComponent {
       { draftingSource: this.draftingSource, trainingSources: this.trainingSources },
       { validators: languageCodesValidator(this.languageCodes) }
     );
-    this.enableBackTranslationDrafting = new FormControl<boolean>(!this.backTranslationLanguageMatchesTarget, {
-      nonNullable: true
-    });
+    this.enableBackTranslationDrafting = new FormControl<boolean>(
+      data.backTranslation != null &&
+        (!this.backTranslationLanguageMatchesTarget || data.backTranslation.draftingAlreadyEnabled),
+      {
+        nonNullable: true
+      }
+    );
     if (!this.canEnableBackTranslationDrafting) {
       this.enableBackTranslationDrafting.disable();
     }

Another option to consider is

# approve-request-dialog.component.ts
@@ -109,9 +109,9 @@ export class ApproveRequestDialogComponent {
     if (data.defaultTrainingSource == null) throw new Error('defaultTrainingSource is required');
 
     this.normalizedTargetLanguageCode = normalizeLanguageCodeToISO639_3(data.targetProject.languageCode);
-    this.backTranslationLanguageMatchesTarget =
+    this.btLanguageDiffersFromTargetLanguage =
       data.backTranslation != null &&
-      normalizeLanguageCodeToISO639_3(data.backTranslation.languageCode) === this.normalizedTargetLanguageCode;
+      normalizeLanguageCodeToISO639_3(data.backTranslation.languageCode) !== this.normalizedTargetLanguageCode;
     this.draftingSource = new FormControl<string>(data.draftingSourceOptions[0]?.paratextId ?? '', {
       validators: [Validators.required],
       nonNullable: true
@@ -127,9 +127,12 @@ export class ApproveRequestDialogComponent {
       { draftingSource: this.draftingSource, trainingSources: this.trainingSources },
       { validators: languageCodesValidator(this.languageCodes) }
     );
-    this.enableBackTranslationDrafting = new FormControl<boolean>(!this.backTranslationLanguageMatchesTarget, {
-      nonNullable: true
-    });
+    this.enableBackTranslationDrafting = new FormControl<boolean>(
+      this.btLanguageDiffersFromTargetLanguage || (data.backTranslation?.draftingAlreadyEnabled ?? false),
+      {
+        nonNullable: true
+      }
+    );
     if (!this.canEnableBackTranslationDrafting) {
       this.enableBackTranslationDrafting.disable();
     }
@@ -139,7 +142,7 @@ export class ApproveRequestDialogComponent {
     return (
       this.data.backTranslation != null &&
       !this.data.backTranslation.draftingAlreadyEnabled &&
-      !this.backTranslationLanguageMatchesTarget
+      this.btLanguageDiffersFromTargetLanguage
     );
   }
 
# approve-request-dialog.component.html
@@ -39,7 +39,7 @@
       <p class="hint">
         @if (bt.draftingAlreadyEnabled) {
           Draft generation is already enabled for this project.
-        } @else if (backTranslationLanguageMatchesTarget) {
+        } @else if (!btLanguageDiffersFromTargetLanguage) {
           Back translation must be a different language than the target project.
         } @else {
           Enabling will set {{ data.targetProject.name }} as the drafting and training source for the back translation.

[marksvc]

Thank you for your work on this and for bearing with all the discussion.

@marksvc reviewed 8 files and all commit messages, made 1 comment, and resolved 4 discussions.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on Nateowami).