SF-3709 Hide resolved note threads when importing questions from PT

[RaymondLuong3]

This PR updates the behaviour for importing community checking questions. Previously all threads for a particular tag were included, but resolved noted threads are hidden in PT and so it makes sense to exclude resolved threads and not allow users to choose them for importing questions.
This also fixes incorrect value assigned to the activeOnly parameter in CommentManager.FindThreads()

---

This change is 

---

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.85%. Comparing base (5e6d0c0) to head (b9719c7).
⚠️ Report is 5 commits behind head on master.
✅ All tests successful. No failed tests found.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3684      +/-   ##
==========================================
+ Coverage   81.83%   81.85%   +0.01%     
==========================================
  Files         619      619              
  Lines       38622    38614       -8     
  Branches     6317     6314       -3     
==========================================
+ Hits        31605    31606       +1     
+ Misses       6043     6036       -7     
+ Partials      974      972       -2     

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

[Nateowami]

@Nateowami reviewed 1 file and all commit messages, and made 1 comment.
Reviewable status: 1 of 2 files reviewed, 1 unresolved discussion (waiting on @RaymondLuong3).

---

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

        CommentTags? commentTags,
        Func<CommentThread, bool>? predicate = null,
        bool activeThreadsOnly = true

I spent a good while on this line and didn't come to a single clear conclusion, but here are findings regarding this line are in order:

1. It looks like you just changed the default behavior of this method (inverting what the default value is for the last arg). That means every use of this method potentially needs to be updated, and it doesn't look like that happened.
2. OK, finding all references and then excluding tests and interfaces shows ParatextService.GetNoteThreads is the only thing that calls it, which is a little surprising. And the only caller of that method is ParatextController.GetNotesAsync. So maybe it's OK? It's kind of odd that ParatextService.GetNotes doesn't call ParatextDataHelper.GetNotes, but ParatextService.GetNoteThreads does. It also feels wrong that we have a method that takes a predicate and a boolean flag, and only one consumer of that method, which doesn't use either.
3. The interface IParatextDataHelper still has the final property named includeInactiveThreads

I'm not completely certain I have 1 and 2 correct, but 3 definitely should be changed. If my analysis is correct though, we can probably just remove the predicate and final boolean flag and simplify this a lot. And then the method name could be updated to clearly indicate whether only active notes are going to be returned.

[RaymondLuong3]

@RaymondLuong3 made 1 comment.
Reviewable status: 0 of 4 files reviewed, 1 unresolved discussion (waiting on @Nateowami).

---

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

Previously, Nateowami wrote…

I spent a good while on this line and didn't come to a single clear conclusion, but here are findings regarding this line are in order:

1. It looks like you just changed the default behavior of this method (inverting what the default value is for the last arg). That means every use of this method potentially needs to be updated, and it doesn't look like that happened.
2. OK, finding all references and then excluding tests and interfaces shows ParatextService.GetNoteThreads is the only thing that calls it, which is a little surprising. And the only caller of that method is ParatextController.GetNotesAsync. So maybe it's OK? It's kind of odd that ParatextService.GetNotes doesn't call ParatextDataHelper.GetNotes, but ParatextService.GetNoteThreads does. It also feels wrong that we have a method that takes a predicate and a boolean flag, and only one consumer of that method, which doesn't use either.
3. The interface IParatextDataHelper still has the final property named includeInactiveThreads

I'm not completely certain I have 1 and 2 correct, but 3 definitely should be changed. If my analysis is correct though, we can probably just remove the predicate and final boolean flag and simplify this a lot. And then the method name could be updated to clearly indicate whether only active notes are going to be returned.

Great points. Thanks for the thoughtful review. I agree, knowing that there is currently no use case for specifying a predicate or a desire for inactive notes and there likely will not be in the future, I've simplified this based on your suggestions.

[Nateowami]

@Nateowami reviewed 3 files and all commit messages, made 5 comments, and resolved 1 discussion.
Reviewable status: 3 of 4 files reviewed, 3 unresolved discussions (waiting on @RaymondLuong3).

---

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

Previously, RaymondLuong3 (Raymond Luong) wrote…

Great points. Thanks for the thoughtful review. I agree, knowing that there is currently no use case for specifying a predicate or a desire for inactive notes and there likely will not be in the future, I've simplified this based on your suggestions.

Looks good.

---

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

    }

    public IReadOnlyList<ParatextNote> GetNotes(CommentManager? commentManager, CommentTags? commentTags)

Why is the comment manager optional? Why are we passing in commentTags? I realize you didn't write this method, but it sure feels like the implementation and usage is mismatched.

---

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

        // Only return note threads that are not resolved and active
        IEnumerable<CommentThread> threads = commentManager.FindThreads(
            t => t.Status != NoteStatus.Resolved,

Isn't this redundant with activeOnly: true?

---

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

        commentTags.InitializeTagList([5]);
        TestEnvironment.AddComment(scrText, "thread-04", "RUT 1:4", "Note to resolve", "5");
        TestEnvironment.AddComment(scrText, "thread-04", "RUT 1:4", "This is resolved", "5", resolved: true);

I did not realize C# had named arguments.

---

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

            string? tagValue,
            bool deleted = false,
            bool resolved = false

Maybe it would make more sense to pass a NoteStatus here, rather than a boolean flag? It's more obvious what it does when calling it, even without a named argument, and more flexible.

[Nateowami]

@Nateowami made 1 comment.
Reviewable status: 3 of 4 files reviewed, 3 unresolved discussions (waiting on @RaymondLuong3).

---

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

Previously, Nateowami wrote…

Why is the comment manager optional? Why are we passing in commentTags? I realize you didn't write this method, but it sure feels like the implementation and usage is mismatched.

It made me wonder if it might be copied from elsewhere, but I wasn't able to find where that might be. It was added quite recently.

[RaymondLuong3]

@RaymondLuong3 made 3 comments.
Reviewable status: 3 of 4 files reviewed, 3 unresolved discussions (waiting on Nateowami).

---

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

Previously, Nateowami wrote…

It made me wonder if it might be copied from elsewhere, but I wasn't able to find where that might be. It was added quite recently.

Ok, in my investigation into the Paratext.Data code I don't see a reason that comment manager or comment tags might be null because it would explicitly create them if one does not already exist. I have removed the nullable type from the signatures on the interface and the implementing class.

---

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

Previously, Nateowami wrote…

Isn't this redundant with activeOnly: true?

activeOnly means threads that have a non-deleted comment. Theoretically a thread can consist of only deleted comments, but that is unusual. This filters out those threads. A resolved thread is always active, but an unresolved thread may not be active.

---

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

Previously, Nateowami wrote…

Maybe it would make more sense to pass a NoteStatus here, rather than a boolean flag? It's more obvious what it does when calling it, even without a named argument, and more flexible.

I get a complaint that I cannot cast from NoteStatus to Enum<NoteStatus>. I do not fully understand it, so I left this as is.

[devin-ai-integration]

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no potential bugs to report.

View in Devin Review to see 3 additional findings.

[Nateowami]

@Nateowami reviewed 3 files and all commit messages, and resolved 3 discussions.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on RaymondLuong3).