SF-3578 Add Import Questions from Paratext notes

[josephmyers]

The notes are fetched from the Paratext local copy, and the relevant pieces are parsed. The back end replaces the tag numbers with their user-displayed text (for readability).

The front end makes this call when the user clicks the Paratext Import option in the import dialog. It first gathers the available tags and displays them as a dropdown. Once a value is chosen, it filters the notes down to the selection, and feeds them to the same display used for the Transcelerator questions.

---

This change is 

[codecov]

Codecov Report

❌ Patch coverage is 67.71300% with 72 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.73%. Comparing base (47ee299) to head (1af2365).
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
...stions-dialog/import-questions-dialog.component.ts	70.09%	26 Missing and 6 partials ⚠️
...IL.XForge.Scripture/Services/ParatextDataHelper.cs	70.49%	9 Missing and 9 partials ⚠️
...XForge.Scripture/Controllers/ParatextController.cs	28.57%	13 Missing and 2 partials ⚠️
...c/SIL.XForge.Scripture/Services/ParatextService.cs	66.66%	2 Missing and 3 partials ⚠️
...ripture/ClientApp/src/app/core/paratext.service.ts	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3570      +/-   ##
==========================================
- Coverage   82.82%   82.73%   -0.09%     
==========================================
  Files         610      611       +1     
  Lines       37400    37600     +200     
  Branches     6151     6163      +12     
==========================================
+ Hits        30976    31108     +132     
- Misses       5478     5541      +63     
- Partials      946      951       +5     

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

[josephmyers]

One thing I intentionally haven't done here is to exclude resolved notes/questions, the reasons being that, if we do, there's no option for the user to bypass that and that, if they really don't want to see the item in the import list, they can just delete it in Paratext.

[pmachapman]

Some preliminary feedback on the .NET side of things.

@pmachapman reviewed 11 of 18 files at r1, 2 of 10 files at r2.
@pmachapman dismissed @github-advanced-security[bot] from 2 discussions.
Reviewable status: 13 of 22 files reviewed, 15 unresolved discussions (waiting on @josephmyers)

---

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

        @"\n\s*<",
        RegexOptions.CultureInvariant | RegexOptions.Compiled
    );

You can take advantage of .Net 8.0's Regular Expression source generator, which generates faster regex code under the hood than setting the compiled flag:

public static partial class NotesFormatter
{
    private const string NotesSchemaVersion = "1.1";

    /// <summary>
    /// The regular expression for finding whitespace before XML tags.
    /// </summary>
    /// <remarks>This is used by <see cref="ParseNotesToXElement"/>.</remarks>
    [GeneratedRegex(@"\n\s*<", RegexOptions.CultureInvariant)]
    private static partial Regex WhitespaceBeforeTagsRegex();

Code quote:

public static class NotesFormatter
{
    private const string NotesSchemaVersion = "1.1";

    /// <summary>
    /// The regular expression for finding whitespace before XML tags.
    /// </summary>
    /// <remarks>This is used by <see cref="ParseNotesToXElement"/>.</remarks>
    private static readonly Regex WhitespaceBeforeTagsRegex = new Regex(
        @"\n\s*<",
        RegexOptions.CultureInvariant | RegexOptions.Compiled
    );

---

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

    /// <param name="notesElement">The notes XML element.</param>
    /// <param name="commentTags">The Paratext comment tag collection.</param>
    public static void ReplaceTagIdentifiersWithCommentTags(XElement notesElement, CommentTags commentTags)

This function is only used in a unit test - should it be retained?

Code quote:

public static void ReplaceTagIdentifiersWithCommentTags(XElement notesElement, CommentTags commentTags)

---

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

        PropertyInfo[] properties = typeof(CommentTag).GetProperties(BindingFlags.Instance | BindingFlags.Public);
        foreach (PropertyInfo property in properties.Where(property => property.CanRead))

I'm not sure why you are iterating over every property in CommentTag? Did you need the properties marked with [XmlIgnore]? Although see also above the comment about whether we need ReplaceTagIdentifiersWithCommentTags.

Code quote:

        PropertyInfo[] properties = typeof(CommentTag).GetProperties(BindingFlags.Instance | BindingFlags.Public);
        foreach (PropertyInfo property in properties.Where(property => property.CanRead))

---

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

    public string Icon { get; set; } = string.Empty;
}

Up to you, but it looks like your model could take advantage of required and init, which might help keep use of your objects in future neat and tidy?

using System.Collections.Generic;

namespace SIL.XForge.Scripture.Models;

/// <summary>
/// Represents a Paratext note thread containing one or more comments for a scripture reference.
/// </summary>
public class ParatextNote
{
    public required string Id { get; init; }

    public required string VerseRef { get; init; }

    public required IReadOnlyList<ParatextNoteComment> Comments { get; init; }
}

/// <summary>
/// Represents a single comment that belongs to a Paratext note thread.
/// </summary>
public class ParatextNoteComment
{
    public required string VerseRef { get; init; }

    public required string Content { get; init; }

    public ParatextNoteTag? Tag { get; init; }
}

/// <summary>
/// Represents a Paratext comment tag that has been applied to a comment.
/// </summary>
public class ParatextNoteTag
{
    public int Id { get; init; }

    public required string Name { get; init; }

    public required string Icon { get; init; }
}

Totally optional though - just an idea I had while I was readying your code.

You'd probably also need to update the last line of NotesService.CreateNoteTag() to:

return new ParatextNoteTag { Id = tagId, Name = string.Empty, Icon = string.Empty };

Code quote:

using System;
using System.Collections.Generic;

namespace SIL.XForge.Scripture.Models;

/// <summary>
/// Represents a Paratext note thread containing one or more comments for a scripture reference.
/// </summary>
public class ParatextNote
{
    public string Id { get; set; } = string.Empty;

    public string VerseRef { get; set; } = string.Empty;

    public IReadOnlyList<ParatextNoteComment> Comments { get; set; } = Array.Empty<ParatextNoteComment>();
}

/// <summary>
/// Represents a single comment that belongs to a Paratext note thread.
/// </summary>
public class ParatextNoteComment
{
    public string VerseRef { get; set; } = string.Empty;

    public string Content { get; set; } = string.Empty;

    public ParatextNoteTag? Tag { get; set; }
}

/// <summary>
/// Represents a Paratext comment tag that has been applied to a comment.
/// </summary>
public class ParatextNoteTag
{
    public int Id { get; set; }

    public string Name { get; set; } = string.Empty;

    public string Icon { get; set; } = string.Empty;
}

---

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

/// Provides helpers for retrieving and mapping Paratext note threads into Scripture Forge models.
/// </summary>
public interface INotesService

Normally we try and avoid exposing Paratext classes in interfaces, but I see it is unavoidable here.

You could perhaps move this method to IParatextDataHelper?

Code quote:

public interface INotesService

---

test/SIL.XForge.Scripture.Tests/Services/NotesFormatterTests.cs line 416 at r3 (raw file):

        // SUT
        NotesFormatter.ReplaceTagIdentifiersWithCommentTags(notesElement, commentTags);

I think this is the only place this method is used?

Code quote:

NotesFormatter.ReplaceTagIdentifiersWithCommentTags(notesElement, commentTags);

---

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

/// Maps Paratext comment threads into lightweight Scripture Forge note models.
/// </summary>
public class NotesService : INotesService

I wonder whether the contents of this class should be moved to IParatextDataHelper, as it is a helper for the logic in ParatextData?

Alternatively I think naming this class to:

public class ParatextNotesService : IParatextNotesService

should help make clear what this class is doing.

Code quote:

public class NotesService : INotesService

---

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

{
    public IReadOnlyList<ParatextNote> GetNotes(
        CommentManager commentManager,

Since you perform a null check, you might as well define this argument as nullable:

CommentManager? commentManager,

Code quote:

CommentManager commentManager,

---

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

            if (comments.Count == 0)
                continue;

You can simplify this to:

List<ParatextNoteComment> comments =
[
    .. thread.ActiveComments.Select(comment => CreateNoteComment(comment, commentTags)),
];
if (comments.Count == 0)
    continue;

Code quote:

            IReadOnlyList<ParatextComment> activeComments = thread.ActiveComments.ToList();
            if (activeComments.Count == 0)
                continue;

            var comments = new List<ParatextNoteComment>();
            foreach (ParatextComment comment in activeComments)
            {
                comments.Add(CreateNoteComment(comment, commentTags));
            }

            if (comments.Count == 0)
                continue;

---

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

                {
                    Id = thread.Id ?? string.Empty,
                    VerseRef = verseRef ?? string.Empty,

This value is never null, as you coalesce it to an empty string below in CreateNoteComment.

Code quote:

VerseRef = verseRef ?? string.Empty,

---

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

    );
    string GetNotes(UserSecret userSecret, string paratextId, int bookNum);
    IReadOnlyList<ParatextNote> GetNoteThreads(UserSecret userSecret, string paratextId);

You should change the signature to be nullable, as you return null in the implementation:

IReadOnlyList<ParatextNote>? GetNoteThreads(UserSecret userSecret, string paratextId)

Code quote:

IReadOnlyList<ParatextNote> GetNoteThreads(UserSecret userSecret, string paratextId);

---

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

    IReadOnlyList<ParatextNote> GetNoteThreads(UserSecret userSecret, string paratextId);
    SyncMetricInfo PutNotes(UserSecret userSecret, string paratextId, XElement notesElement);
    CommentTags? GetCommentTags(UserSecret userSecret, string paratextId);

We should avoid exposing PT classes in interfaces, where possible. See comment below about making this function internal and avoiding the need to expose it in this interface.

Code quote:

CommentTags? GetCommentTags(UserSecret userSecret, string paratextId);

---

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

                LogMetric("Updating Paratext notes for questions");
                if (questionDocsByBook[text.BookNum].Count > 0)
                {

Why did you remove this? It will slow down syncs for projects without any questions (i.e. most projects)

Code quote:

                if (questionDocsByBook[text.BookNum].Count > 0)
                {

---

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

    /// <summary> Gets note threads from the Paratext project and maps them to Scripture Forge models. </summary>
    public IReadOnlyList<ParatextNote> GetNoteThreads(UserSecret userSecret, string paratextId)

You should change the signature to be nullable, as you return null:

public IReadOnlyList<ParatextNote>? GetNoteThreads(UserSecret userSecret, string paratextId)

Code quote:

public IReadOnlyList<ParatextNote> GetNoteThreads(UserSecret userSecret, string paratextId)

---

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

    }

    public CommentTags? GetCommentTags(UserSecret userSecret, string paratextId)

It seems this is public only for unit testing? You could make it internal instead.

Code quote:

public CommentTags? GetCommentTags(UserSecret userSecret, string paratextId)

[josephmyers]

Reviewable status: 3 of 25 files reviewed, 15 unresolved discussions (waiting on @pmachapman)

---

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

Previously, pmachapman (Peter Chapman) wrote…

Up to you, but it looks like your model could take advantage of required and init, which might help keep use of your objects in future neat and tidy?

using System.Collections.Generic;

namespace SIL.XForge.Scripture.Models;

/// <summary>
/// Represents a Paratext note thread containing one or more comments for a scripture reference.
/// </summary>
public class ParatextNote
{
    public required string Id { get; init; }

    public required string VerseRef { get; init; }

    public required IReadOnlyList<ParatextNoteComment> Comments { get; init; }
}

/// <summary>
/// Represents a single comment that belongs to a Paratext note thread.
/// </summary>
public class ParatextNoteComment
{
    public required string VerseRef { get; init; }

    public required string Content { get; init; }

    public ParatextNoteTag? Tag { get; init; }
}

/// <summary>
/// Represents a Paratext comment tag that has been applied to a comment.
/// </summary>
public class ParatextNoteTag
{
    public int Id { get; init; }

    public required string Name { get; init; }

    public required string Icon { get; init; }
}

Totally optional though - just an idea I had while I was readying your code.

You'd probably also need to update the last line of NotesService.CreateNoteTag() to:

return new ParatextNoteTag { Id = tagId, Name = string.Empty, Icon = string.Empty };

Done.

---

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

Previously, pmachapman (Peter Chapman) wrote…

You should change the signature to be nullable, as you return null in the implementation:

IReadOnlyList<ParatextNote>? GetNoteThreads(UserSecret userSecret, string paratextId)

Done.

---

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

Previously, pmachapman (Peter Chapman) wrote…

We should avoid exposing PT classes in interfaces, where possible. See comment below about making this function internal and avoiding the need to expose it in this interface.

Done.

---

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

Previously, pmachapman (Peter Chapman) wrote…

You can take advantage of .Net 8.0's Regular Expression source generator, which generates faster regex code under the hood than setting the compiled flag:

public static partial class NotesFormatter
{
    private const string NotesSchemaVersion = "1.1";

    /// <summary>
    /// The regular expression for finding whitespace before XML tags.
    /// </summary>
    /// <remarks>This is used by <see cref="ParseNotesToXElement"/>.</remarks>
    [GeneratedRegex(@"\n\s*<", RegexOptions.CultureInvariant)]
    private static partial Regex WhitespaceBeforeTagsRegex();

Done.

---

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

Previously, pmachapman (Peter Chapman) wrote…

This function is only used in a unit test - should it be retained?

Done. Nope!

---

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

Previously, pmachapman (Peter Chapman) wrote…

I'm not sure why you are iterating over every property in CommentTag? Did you need the properties marked with [XmlIgnore]? Although see also above the comment about whether we need ReplaceTagIdentifiersWithCommentTags.

Done.

---

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

Previously, pmachapman (Peter Chapman) wrote…

You should change the signature to be nullable, as you return null:

public IReadOnlyList<ParatextNote>? GetNoteThreads(UserSecret userSecret, string paratextId)

Done.

---

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

Previously, pmachapman (Peter Chapman) wrote…

It seems this is public only for unit testing? You could make it internal instead.

Done. Love it

---

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

Previously, pmachapman (Peter Chapman) wrote…

Why did you remove this? It will slow down syncs for projects without any questions (i.e. most projects)

This is critical, otherwise the only books that we poll for questions are the ones that already have questions in SF. Maybe there's a better way to do this?

---

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

Previously, pmachapman (Peter Chapman) wrote…

Normally we try and avoid exposing Paratext classes in interfaces, but I see it is unavoidable here.

You could perhaps move this method to IParatextDataHelper?

Done.

---

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

Previously, pmachapman (Peter Chapman) wrote…

I wonder whether the contents of this class should be moved to IParatextDataHelper, as it is a helper for the logic in ParatextData?

Alternatively I think naming this class to:

public class ParatextNotesService : IParatextNotesService

should help make clear what this class is doing.

Done.

---

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

Previously, pmachapman (Peter Chapman) wrote…

Since you perform a null check, you might as well define this argument as nullable:

CommentManager? commentManager,

Done.

---

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

Previously, pmachapman (Peter Chapman) wrote…

You can simplify this to:

List<ParatextNoteComment> comments =
[
    .. thread.ActiveComments.Select(comment => CreateNoteComment(comment, commentTags)),
];
if (comments.Count == 0)
    continue;

Done.

---

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

Previously, pmachapman (Peter Chapman) wrote…

This value is never null, as you coalesce it to an empty string below in CreateNoteComment.

Done.

---

test/SIL.XForge.Scripture.Tests/Services/NotesFormatterTests.cs line 416 at r3 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

I think this is the only place this method is used?

Done. I had implemented this earlier and then refactored.

[josephmyers]

@pmachapman I've split out the note id property and implemented your back end comments.

[pmachapman]

Looking good - just a couple of minor comments & discussion.

@pmachapman reviewed 1 of 18 files at r1, 1 of 10 files at r2, 20 of 20 files at r4, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @josephmyers)

---

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

Previously, josephmyers wrote…

This is critical, otherwise the only books that we poll for questions are the ones that already have questions in SF. Maybe there's a better way to do this?

From what I understand, UpdateParatextNotesAsync writes notes to Paratext, based on questions that are already in Scripture Forge (i.e. questions that are created manually, or via transcelerator, or via your new import questions feature). If questionDocsByBook[text.BookNum].Count is 0, then UpdateParatextNotesAsync won't do anything to the project, unless I am missing some other side effect or event that is happening in that function?

---

test/SIL.XForge.Scripture.Tests/Services/ParatextDataHelperTests.cs line 122 at r4 (raw file):

            // Ensure that the SLDR is initialized for LanguageID.Code to be retrieved correctly
            if (!Sldr.IsInitialized)
                Sldr.Initialize(true);

The Sldr is used in ScrText (and so MockScrText), so this should be initialized in your NotesTestEnvironment as well. Should these two test environments be combined?

Code quote:

            // Ensure that the SLDR is initialized for LanguageID.Code to be retrieved correctly
            if (!Sldr.IsInitialized)
                Sldr.Initialize(true);

---

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

    public DateTime DateCreated { get; set; }
    public string TransceleratorQuestionId { get; set; }
    public string ParatextNoteId { get; set; }

NIT: As these are nullable in the TypeScript model, can you please make these both nullable in C#? (TransceleratorQuestionId was added to Scripture Forge before support for nullable types was added to the C# project) i.e.

public string? TransceleratorQuestionId { get; set; }
public string? ParatextNoteId { get; set; }

Code quote:

    public string TransceleratorQuestionId { get; set; }
    public string ParatextNoteId { get; set; }

[josephmyers]

Reviewable status: 24 of 25 files reviewed, 2 unresolved discussions (waiting on @pmachapman)

---

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

Previously, pmachapman (Peter Chapman) wrote…

NIT: As these are nullable in the TypeScript model, can you please make these both nullable in C#? (TransceleratorQuestionId was added to Scripture Forge before support for nullable types was added to the C# project) i.e.

public string? TransceleratorQuestionId { get; set; }
public string? ParatextNoteId { get; set; }

Done.

---

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

Previously, pmachapman (Peter Chapman) wrote…

From what I understand, UpdateParatextNotesAsync writes notes to Paratext, based on questions that are already in Scripture Forge (i.e. questions that are created manually, or via transcelerator, or via your new import questions feature). If questionDocsByBook[text.BookNum].Count is 0, then UpdateParatextNotesAsync won't do anything to the project, unless I am missing some other side effect or event that is happening in that function?

Ok, good call. I went back and tested this specific change, and it appears to be functional without the change. Thanks for pushing me on this, there must've been some other problem that was causing books to get skipped back when I was developing this.

---

test/SIL.XForge.Scripture.Tests/Services/ParatextDataHelperTests.cs line 122 at r4 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

The Sldr is used in ScrText (and so MockScrText), so this should be initialized in your NotesTestEnvironment as well. Should these two test environments be combined?

Done. This may be a cop-out, but AI thinks they should be kept separate. (I get it, because the only duplication is that SLDR code and the object under test.) I'm not strongly opinionated either way.

[pmachapman]

@pmachapman reviewed 4 of 4 files at r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @josephmyers)