SF-3163 Store Draft Sources as Arrays

[pmachapman]

This PR migrates the previous draft source structure to a set of arrays in the draftConfig.

---

This change is 

[codecov]

Codecov Report

❌ Patch coverage is 98.31461% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.41%. Comparing base (93615cb) to head (ce57a40).
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
.../SIL.XForge.Scripture/Services/SFProjectService.cs	95.45%	0 Missing and 2 partials ⚠️
...XForge.Scripture/Services/MachineProjectService.cs	98.41%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3508      +/-   ##
==========================================
- Coverage   82.48%   82.41%   -0.08%     
==========================================
  Files         615      615              
  Lines       36914    36756     -158     
  Branches     6009     6021      +12     
==========================================
- Hits        30450    30291     -159     
+ Misses       5579     5565      -14     
- Partials      885      900      +15     

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

[Copilot]

Pull Request Overview

This PR migrates the draft source structure from individual boolean flags and single source objects to arrays of sources in the draftConfig. The migration consolidates alternate, additional training, and drafting sources into two arrays: draftingSources and trainingSources.

Key changes:

• Replaces individual source properties with draftingSources and trainingSources arrays
• Updates all backend services to work with the new array structure
• Provides migration logic to convert existing projects
• Updates frontend components to use the simplified array-based approach

Reviewed Changes

Copilot reviewed 29 out of 29 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
src/SIL.XForge.Scripture/Models/DraftConfig.cs	Replaces individual source properties with arrays
src/SIL.XForge.Scripture/Services/SFProjectService.cs	Updates project service to handle array-based sources
src/SIL.XForge.Scripture/Services/MachineProjectService.cs	Modifies machine project service for new source structure
src/RealtimeServer/scriptureforge/services/sf-project-migrations.ts	Adds migration from old structure to new arrays
Frontend TypeScript files	Updates components and services to use array-based sources
Test files	Updates all tests to reflect the new data structure

[Copilot]

Same issue as above - the logic for updatePermissions is incorrect. It should check if the current paratextId is NOT in sourceParatextIds, but it's checking if settings.SourceParatextId is not in the list. This should be !sourceParatextIds.Contains(paratextId).

Suggested change

	updatePermissions: !sourceParatextIds.Contains(settings.SourceParatextId),
	updatePermissions: !sourceParatextIds.Contains(paratextId),

[pmachapman]

Done.

[Copilot]

This assertion appears to be incorrect. The comment on line 2506 says 'Ensure that the new project does not exist', but the assertion checks Is.True which would mean the project DOES exist. This should likely be Is.False to match the comment's intention.

Suggested change

	Is.True
	Is.False

[Nateowami]

Once again this looks wrong to me for the same reason Copilot thinks it's wrong.

[pmachapman]

Done. The comment was wrong.

[Nateowami]

Excellent work. I'm impressed how quickly this was accomplished. I think there are still a number of tests, comments, localization strings, and possibly dead code that references the old model. You can find them by searching for(alternate|additional)( |_|-|)(drafting|training)[^D] (The [^D] is to filter out results for additionalTrainingData). A few results are for migrations, or other things that make sense to keep the old terminology.

@Nateowami reviewed 29 of 29 files at r1, all commit messages.
Reviewable status: all files reviewed, 14 unresolved discussions (waiting on @pmachapman)

---

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

 * @returns An object with three arrays: trainingSources, trainingTargets, and draftingSources
 */
export function projectToDraftSources(project: SFProjectProfile): DraftSourcesAsTranslateSourceArrays {

I'm pretty sure old clients calling this (running the old code) are going to choke on the new model as soon as they see it. Should we consider releasing just a change to this function that is effectively:

if (oldFormat) {
  // old logic 
} else {
  // new logic
}

...and releasing that a week before we release the rest of the changes? Either way, I think we should test what happens to newer clients when they see the new data format. I'm concerned because we have so many components and services related to drafting that would try to read the current config, that things would start breaking left and right.

---

src/RealtimeServer/scriptureforge/models/sf-project.ts line 25 at r1 (raw file):

  // Indexes for SFProjectService.IsSourceProject() in .NET
  [obj<SFProject>().pathStr(p => p.translateConfig.source!.projectRef), { sparse: true }],
  ['translateConfig.draftConfig.draftingSources.projectRef', { sparse: true }],

Is it not possible to use the type-safe method the lines above it use?

---

test/SIL.XForge.Scripture.Tests/Services/MachineProjectServiceTests.cs line 1467 at r1 (raw file):

        // SUT
        Assert.Throws<InvalidDataException>(() => env.Service.GetSourceLanguage(project, preTranslate: false));

Shouldn't this be pretranslate: true? Otherwise it won't even be reading the DraftingSources?

---

test/SIL.XForge.Scripture.Tests/Services/MachineProjectServiceTests.cs line 3108 at r1 (raw file):

                }

                if (options.TwoTrainingSources)

Looking through the rest of the code, it looks like this works, but only because anytime TwoTrainingSources is set to true, it also sets OneDraftingAndTrainingSource to true. Could the "two implies also one" logic be moved here as well, so it doesn't rely on logic elsewhere to behave in a logical manner?

For example:

if (one) add drafting source
if (one || two) add training source
if (two) add another training source

Alternatively the option could be changed to a number.

---

src/SIL.XForge.Scripture/Services/MachineProjectService.cs line 1731 at r1 (raw file):

        projects.AddRange(
            project
                .TranslateConfig.DraftConfig.DraftingSources.Union(project.TranslateConfig.DraftConfig.TrainingSources)

This isn't really a union is it, since it won't filter out duplicates? It seems like a different extension method would be more appropriate, so it doesn't imply it's filtering out duplicates, but it looks like Append isn't available, and maybe there aren't any other suitable methods either?

---

src/SIL.XForge.Scripture/Controllers/SFProjectsRpcController.cs line 196 at r1 (raw file):

                    { "UsersSeeEachOthersResponses", settings?.UsersSeeEachOthersResponses?.ToString() },
                    { "HideCommunityCheckingText", settings?.HideCommunityCheckingText?.ToString() },
                }

Don't we need to add the new properties here to be reported?

---

src/SIL.XForge.Scripture/Models/SFProjectSettings.cs line 19 at r1 (raw file):

    [Obsolete("For backwards compatibility with older frontend clients. Deprecated October 2025.")]
    public bool? AlternateSourceEnabled { get; set; }

I'm pretty sure new clients are going to immediately need to "speak" the new language? I'm not seeing what use-case leaving this property fixes.

It looks to me like maybe the real reason this property was kept is so you can check for it and detect that I client is trying to use the old API, and throw an exception?

---

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

            UpdateSetting(op, p => p.BiblicalTermsConfig.BiblicalTermsEnabled, settings.BiblicalTermsEnabled);
            UpdateSetting(op, p => p.TranslateConfig.Source, source, unsetSourceProject);
            if (trainingSources.Count > 0)

Does this mean this endpoint can't be used to clear the sources? We're not currently allowing users to clear sources, but that's more of a UI choice that probably shouldn't be reflected in the back-end logic.

---

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

            .QuerySnapshots<SFProject>()
            .Any(p =>
                p.TranslateConfig.Source != null && p.TranslateConfig.Source.ProjectRef == projectId

Can't optional chaining be used to remove the null check?

---

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

        await AddUserToSourceProjectAsync(conn, projectDoc.Data.TranslateConfig.Source, userDoc, shareKey);
        IEnumerable<TranslateSource> translateSources = projectDoc
            .Data.TranslateConfig.DraftConfig.DraftingSources.Union(

As mentioned elsewhere, Union feels like the wrong method for something that won't be de-duplicated, though maybe there isn't anything better available.

---

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

        SFProject project = env.GetProject(Project01);
        Assert.That(project.TranslateConfig.DraftConfig.TrainingSources[0].ProjectRef, Is.Not.Null);
        Assert.That(project.TranslateConfig.DraftConfig.TrainingSources[0].ParatextId, Is.EqualTo("changedId"));

I think you probably meant to reference the variable name instead of hard-coding a string? Otherwise I don't know what the purpose of defining a variable was.

[pmachapman]

I have cleaned up the terminology (mostly in tests or comments). Thank you for your review, and I am impressed Copilot picked up some real bugs (and incorrect comments)

Reviewable status: 15 of 33 files reviewed, 9 unresolved discussions (waiting on @Nateowami)

---

src/RealtimeServer/scriptureforge/models/sf-project.ts line 25 at r1 (raw file):

Previously, Nateowami wrote…

Is it not possible to use the type-safe method the lines above it use?

No, as draftingSources and trainingSources are arrays, and as pathStr converts the path to a string, it would embed the array [0] (or similar) in the index, which would be an invalid index for MongoDB. Although current pathStr is not used for arrays, overriding pathStr to not do this for arrays may break future expected behavior for other uses of it outside of index creation (i.e. filtering).

I place the blame on MongoDB for using a different syntax for an index of a field in an array than it uses for a filter for that same field.

---

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

Previously, Nateowami wrote…

I'm pretty sure old clients calling this (running the old code) are going to choke on the new model as soon as they see it. Should we consider releasing just a change to this function that is effectively:

if (oldFormat) {
  // old logic 
} else {
  // new logic
}

...and releasing that a week before we release the rest of the changes? Either way, I think we should test what happens to newer clients when they see the new data format. I'm concerned because we have so many components and services related to drafting that would try to read the current config, that things would start breaking left and right.

The choking will happen from the old version of this code not knowing about draftingSources and trainingSources, and so returning empty sources here as the old fields would have migrated to the new. This is also why I blocked their updates to settings explicitly in the backend, as I wanted them to not proceed until their client updates.

They only way around it would be to keep the old fields with their values being in sync with draftingSources and trainingSources, but that would cause all kinds of other headaches. If you think it is worth it for the potential pain, I can do it.

Another way would be something similar to the editingRequires feature I implemented in #3339, but that would have required us to already roll it out.

---

src/SIL.XForge.Scripture/Controllers/SFProjectsRpcController.cs line 196 at r1 (raw file):

Previously, Nateowami wrote…

Don't we need to add the new properties here to be reported?

Done. Yes we do - thank you!

---

src/SIL.XForge.Scripture/Models/SFProjectSettings.cs line 19 at r1 (raw file):

It looks to me like maybe the real reason this property was kept is so you can check for it and detect that I client is trying to use the old API, and throw an exception?

Yes. Please see my reply to your comment on projectToDraftSources.

---

src/SIL.XForge.Scripture/Services/MachineProjectService.cs line 1731 at r1 (raw file):

Previously, Nateowami wrote…

This isn't really a union is it, since it won't filter out duplicates? It seems like a different extension method would be more appropriate, so it doesn't imply it's filtering out duplicates, but it looks like Append isn't available, and maybe there aren't any other suitable methods either?

Done. You are right - I have changed to Concat as that is clearer.

---

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

Previously, Nateowami wrote…

Does this mean this endpoint can't be used to clear the sources? We're not currently allowing users to clear sources, but that's more of a UI choice that probably shouldn't be reflected in the back-end logic.

Done. I originally wasn't (because the UI doesn't), but based on your comment I've added support for clearing sources via an empty array.

---

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

Previously, Nateowami wrote…

Can't optional chaining be used to remove the null check?

No, as this is a MongoDB LINQ3 query, which does not support optional chaining.

---

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

Previously, Nateowami wrote…

As mentioned elsewhere, Union feels like the wrong method for something that won't be de-duplicated, though maybe there isn't anything better available.

Done. I have changed to a Concat for clarity.

---

test/SIL.XForge.Scripture.Tests/Services/MachineProjectServiceTests.cs line 1467 at r1 (raw file):

Previously, Nateowami wrote…

Shouldn't this be pretranslate: true? Otherwise it won't even be reading the DraftingSources?

Done. Yes - thank you!

---

test/SIL.XForge.Scripture.Tests/Services/MachineProjectServiceTests.cs line 3108 at r1 (raw file):

Previously, Nateowami wrote…

Looking through the rest of the code, it looks like this works, but only because anytime TwoTrainingSources is set to true, it also sets OneDraftingAndTrainingSource to true. Could the "two implies also one" logic be moved here as well, so it doesn't rely on logic elsewhere to behave in a logical manner?

For example:

if (one) add drafting source
if (one || two) add training source
if (two) add another training source

Alternatively the option could be changed to a number.

Done. I have changed the options to numbers as I think that is clearer.

---

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

Previously, Nateowami wrote…

I think you probably meant to reference the variable name instead of hard-coding a string? Otherwise I don't know what the purpose of defining a variable was.

Done.

[pmachapman]

Done.

[pmachapman]

Done. The comment was wrong.

[Nateowami]

@Nateowami reviewed 18 of 19 files at r2, 4 of 4 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @pmachapman)

---

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

Previously, pmachapman (Peter Chapman) wrote…

The choking will happen from the old version of this code not knowing about draftingSources and trainingSources, and so returning empty sources here as the old fields would have migrated to the new. This is also why I blocked their updates to settings explicitly in the backend, as I wanted them to not proceed until their client updates.

They only way around it would be to keep the old fields with their values being in sync with draftingSources and trainingSources, but that would cause all kinds of other headaches. If you think it is worth it for the potential pain, I can do it.

Another way would be something similar to the editingRequires feature I implemented in #3339, but that would have required us to already roll it out.

This is the only thing that really concerns me. I haven't figured out how to test it, but I think every single project that opens SF will mistakenly think no draft sources are set until the page reloads. I'm not sure how big of a deal it will be, but I think it could be pretty jarring for some users. What I'm thinking is that perhaps before merging this change we should ship the following at the top of the current function:

  const draftConfig = project.translateConfig.draftConfig;
  if (
    hasArrayProp(project.translateConfig.draftConfig, 'trainingSources') &&
    hasArrayProp(project.translateConfig.draftConfig, 'draftingSources')
  ) {
    const trainingSources: TranslateSource[] = [...project.translateConfig.draftConfig.trainingSources] as any[];
    const draftingSources: TranslateSource[] = [...project.translateConfig.draftConfig.draftingSources] as any[];
    const trainingTargets: SFProjectProfile[] = [project];
    return { trainingSources, trainingTargets, draftingSources };
  }

And then old clients would be prepared for the new data model.

What do you think?

[pmachapman]

Reviewable status: 29 of 34 files reviewed, 1 unresolved discussion (waiting on @Nateowami)

---

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

Previously, Nateowami wrote…

This is the only thing that really concerns me. I haven't figured out how to test it, but I think every single project that opens SF will mistakenly think no draft sources are set until the page reloads. I'm not sure how big of a deal it will be, but I think it could be pretty jarring for some users. What I'm thinking is that perhaps before merging this change we should ship the following at the top of the current function:

  const draftConfig = project.translateConfig.draftConfig;
  if (
    hasArrayProp(project.translateConfig.draftConfig, 'trainingSources') &&
    hasArrayProp(project.translateConfig.draftConfig, 'draftingSources')
  ) {
    const trainingSources: TranslateSource[] = [...project.translateConfig.draftConfig.trainingSources] as any[];
    const draftingSources: TranslateSource[] = [...project.translateConfig.draftConfig.draftingSources] as any[];
    const trainingTargets: SFProjectProfile[] = [project];
    return { trainingSources, trainingTargets, draftingSources };
  }

And then old clients would be prepared for the new data model.

What do you think?

Yes, that could work. It would delay this PR by one or two release cycles, but I think that is OK given this is not a super urgent PR.

I have created #3531 to do this.