SF-3713 Allow importing notes that have note tag changes

[pmachapman]

This PR allows notes to be imported form Paratext that the most recent comment is a note tag change.

---

---

This change 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 2 additional findings.

[codecov]

Codecov Report

❌ Patch coverage is 50.00000% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.32%. Comparing base (bdd850d) to head (5ed2f57).
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
...stions-dialog/import-questions-dialog.component.ts	50.00%	1 Missing and 2 partials ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #3717   +/-   ##
=======================================
  Coverage   81.32%   81.32%           
=======================================
  Files         620      620           
  Lines       39023    39026    +3     
  Branches     6360     6342   -18     
=======================================
+ Hits        31735    31738    +3     
- Misses       6305     6317   +12     
+ Partials      983      971   -12     

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

[Nateowami]

My first reaction is that I'm surprised the notes aren't pre-processed on the back end, so that all the specifics of how Paratext handles notes is kept on the back end. To me, the lack of proper encapsulation of the Paratext note format seems like the root cause of this bug, and of SF-3714, and possibly others. Maybe we should try to rework the processing of notes server-side before they're sent to the client?

I was also surprised to see the front end loads all notes, not just those of the note tag the user selects.

@Nateowami made 1 comment.
Reviewable status: 0 of 2 files reviewed, all discussions resolved.

[pmachapman]

My first reaction is that I'm surprised the notes aren't pre-processed on the back end, so that all the specifics of how Paratext handles notes is kept on the back end.

@Nateowami They are processed in the backend, and transformed (i.e. were PT has multiple note elements for the same note, we have a note document with comments in it), but some of the peculiarities of Paratext notes are maintained (such as changing note tag) so they can be rendered on the frontend in a similar way to Paratext, maintaining the correct note history.

Maybe we should try to rework the processing of notes server-side before they're sent to the client?

I am not sure what we would rework specifically, unless we were to show notes in a completely different way to Paratext? We perform processing of note content from XML to HTML, and combine notes in the same thread into one note document with multiple comments, all while maintaining 1:1 import/export compatibility with Paratext's notes. If we transformed or stripped the data too much, we would be unable to maintain this level of compatibility on sync.

I was also surprised to see the front end loads all notes, not just those of the note tag the user selects.

Yes, as comments in notes can change the note tag, this is necessary.

[RaymondLuong3]

I wonder if we can add a qualification that only the very first comment with a given tag will be interpreted as the question. See my reasoning below.

@RaymondLuong3 reviewed 2 files and all commit messages, and made 2 comments.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on pmachapman).

---

src/SIL.XForge.Scripture/ClientApp/src/app/checking/import-questions-dialog/import-questions-dialog.component.ts line 624 at r1 (raw file):

        if (comment.tag == null || comment.tag.id !== tagId) {
          continue;
        }

It does seem like it would save us effort if we filter out which note tag we want when we retrieve notes from the server. I do see that we use the notes we retrieve from the server to get the list of note tags we can use, so we would need a different way to access those tags.
I wonder if it would be better to be explicit that only the first comment with a given tag is the question. Suppose somebody created a question and tagged that note to be imported. Then a second comment is added to the note thread. This would import the second comment instead of the real question. Any thoughts?

Code quote:

        if (comment.tag == null || comment.tag.id !== tagId) {
          continue;
        }

[pmachapman]

I wonder if we can add a qualification that only the very first comment with a given tag will be interpreted as the question.

Good idea. Done.

The XML is stripped, so I haven't needed to update the backend as I thought I might.

@pmachapman made 2 comments.
Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on RaymondLuong3).

---

src/SIL.XForge.Scripture/ClientApp/src/app/checking/import-questions-dialog/import-questions-dialog.component.ts line 624 at r1 (raw file):

Previously, RaymondLuong3 (Raymond Luong) wrote…

It does seem like it would save us effort if we filter out which note tag we want when we retrieve notes from the server. I do see that we use the notes we retrieve from the server to get the list of note tags we can use, so we would need a different way to access those tags.
I wonder if it would be better to be explicit that only the first comment with a given tag is the question. Suppose somebody created a question and tagged that note to be imported. Then a second comment is added to the note thread. This would import the second comment instead of the real question. Any thoughts?

Done.