[NewEntry] If duplicate, add note to existing

[imnasnainaec]

The "Note" part of #1584

---

This change is 

[johnthagen]

Hit #1579 while testing this issue.

[johnthagen]

---

src/components/DataEntry/DataEntryTable/DataEntryTable.tsx, line 167 at r1 (raw file):

      return;
    }
    // TODO: add the audio even if the word is a duplicate

The frontend does not update the existing field in the Data Entry if a note is added this way. This may be an edge case that is not super important if this is complex to fix.

In the following screenshot, I added another x/y entry with a note. I got the alert that it exists in the database, and the Note is added in the backend, but the frontend UI doesn't show that the Note exists (i.e. the note icon doesn't change and its hover text doesn't display the note).

[johnthagen]

---

src/components/DataEntry/DataEntryTable/DataEntryTable.tsx, line 167 at r1 (raw file):

Previously, johnthagen wrote…

The frontend does not update the existing field in the Data Entry if a note is added this way. This may be an edge case that is not super important if this is complex to fix.

In the following screenshot, I added another x/y entry with a note. I got the alert that it exists in the database, and the Note is added in the backend, but the frontend UI doesn't show that the Note exists (i.e. the note icon doesn't change and its hover text doesn't display the note).

In review entries, I do see the note added (confirming that the backend is doing the correct thing in the background).

[imnasnainaec]

src/components/DataEntry/DataEntryTable/DataEntryTable.tsx, line 167 at r1 (raw file):

      return;
    }
    // TODO: add the audio even if the word is a duplicate

The frontend does not update the existing field in the Data Entry if a note is added this way. This may be an edge case that is not super important if this is complex to fix.

In the following screenshot, I added another x/y entry with a note. I got the alert that it exists in the database, and the Note is added in the backend, but the frontend UI doesn't show that the Note exists (i.e. the note icon doesn't change and its hover text doesn't display the note).

That may fall in the realm of #1586

[johnthagen]

---

Backend/Models/Word.cs, line 247 at r1 (raw file):

        /// <summary> Add another note to the existing note. </summary>
        public void Add(Note note)

Perhaps a more explicit name would be Append?

[johnthagen]

---

Backend/Models/Word.cs, line 261 at r1 (raw file):

                    if (Language != note.Language)
                    {
                        Text += String.Format("[{0}] ", note.Language);

This can be simplified to:

Text += $"[{note.Language}] ";

[johnthagen]

---

Backend/Models/Word.cs, line 261 at r1 (raw file):

Previously, johnthagen wrote…

This can be simplified to:

Text += $"[{note.Language}] ";

I would actually simplify the entire else to:

                    var langTag = Language != note.Language ? "" : $"[{note.Language}] " ;
                    Text += $"; {langTag}{note.Text}";

[johnthagen]

---

Backend/Models/Word.cs, line 261 at r1 (raw file):

Previously, johnthagen wrote…

I would actually simplify the entire else to:

                    var langTag = Language != note.Language ? "" : $"[{note.Language}] " ;
                    Text += $"; {langTag}{note.Text}";

To refactor the whole function to reduce nesting:

        public void Add(Note note)
        {
            if (note.IsBlank() || Equals(note))
            {
                return;
            }

            if (IsBlank())
            {
                Language = note.Language;
                Text = note.Text;
            }
            else
            {
                var langTag = Language != note.Language ? "" : $"[{note.Language}] ";
                Text += $"; {langTag}{note.Text}";
            }
        }

[johnthagen]

---

Backend/Services/WordService.cs, line 1 at r1 (raw file):

using System;

Unused import.

[johnthagen]

---

Backend/Services/WordService.cs, line 186 at r1 (raw file):

                            matchingVern.EditedBy = matchingVern.EditedBy.Distinct().ToList();

                            // Add note

This comment is not very useful as it duplicates the same information that exists in the next line. Could it be improved or removed?

[johnthagen]

---

Backend.Tests/Models/WordTests.cs, line 128 at r1 (raw file):

            var oldNote = note.Clone();
            var newNote = note.Clone();
            var newText = "sameLangNewText";

Can be const

[codecov-commenter]

Codecov Report

Merging #1615 (82053e7) into master (e29f156) will increase coverage by 27.63%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           master    #1615       +/-   ##
===========================================
+ Coverage   52.37%   80.00%   +27.63%     
===========================================
  Files         272       37      -235     
  Lines        8321     3456     -4865     
  Branches      607        0      -607     
===========================================
- Hits         4358     2765     -1593     
+ Misses       3476      691     -2785     
+ Partials      487        0      -487     

Flag	Coverage Δ
backend	80.00% <100.00%> (-0.21%)	⬇️
frontend	?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
Backend/Models/Word.cs	98.49% <100.00%> (+0.06%)	⬆️
Backend/Services/WordService.cs	76.66% <100.00%> (+0.19%)	⬆️
Backend/Helper/DuplicateFinder.cs	87.64% <0.00%> (-5.62%)	⬇️
...onents/DataEntry/DataEntryTable/DataEntryTable.tsx
src/api/api/user-edit-api.ts
...c/goals/MergeDupGoal/MergeDupStep/MergeDupsTree.ts
src/resources/dictionaries/index.ts
src/components/UserSettings/AvatarUpload.tsx
...ries/ReviewEntriesComponent/ReviewEntriesTable.tsx
src/components/AppBar/ProjectNameButton.tsx
... and 228 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e29f156...82053e7. Read the comment docs.

[johnthagen]

---

Backend.Tests/Models/WordTests.cs, line 132 at r1 (raw file):

            oldNote.Add(newNote);
            var expectedNote = note.Clone();
            expectedNote.Text += "; " + newText;

Simplify to

expectedNote.Text += $"; {newText}";

[johnthagen]

---

Backend.Tests/Models/WordTests.cs, line 146 at r1 (raw file):

            oldNote.Add(newNote);
            var expectedNote = note.Clone();
            expectedNote.Text += "; [" + newLanguage + "] " + newNote.Text;

Simplify to

expectedNote.Text += $"; [{newLanguage}] {newNote.Text}";

[johnthagen]

---

Backend/Services/WordService.cs, line 187 at r1 (raw file):

                            // Add note
                            matchingVern.Note.Add(word.Note);

Can we add a test(s) to WordServiceTests.cs to ensure that the Service itself actually adds the Note as expected (i.e. test WordIsUnique)?

Another thing that feels a little odd semantically is that WordIsUnique seems like a read-only checking function, but in reality it is now (or maybe it was before as well?) mutating the Word that is being checked.

This seems like too much going on in one function. We should probably at least change the name to make it obvious that mutation is happening, or (ideally) pull the mutation logic out into a separate function that runs after uniqueness is checked (e.g. UpdateUniqueWord etc.)

[johnthagen]

---

src/components/DataEntry/DataEntryTable/DataEntryTable.tsx, line 167 at r1 (raw file):

Previously, imnasnainaec (D. Ror.) wrote…

That may fall in the realm of #1586

Agreed. I added a note to the issue with this condition.

[johnthagen]

Reviewed 4 of 4 files at r1.
Reviewable status: 2 of 4 files reviewed, 8 unresolved discussions (waiting on @imnasnainaec and @johnthagen)

[johnthagen]

---

Backend/Models/Word.cs, line 247 at r1 (raw file):

Previously, johnthagen wrote…

Perhaps a more explicit name would be Append?

Done.

[johnthagen]

---

Backend/Models/Word.cs, line 261 at r1 (raw file):

Previously, johnthagen wrote…

To refactor the whole function to reduce nesting:

        public void Add(Note note)
        {
            if (note.IsBlank() || Equals(note))
            {
                return;
            }

            if (IsBlank())
            {
                Language = note.Language;
                Text = note.Text;
            }
            else
            {
                var langTag = Language != note.Language ? "" : $"[{note.Language}] ";
                Text += $"; {langTag}{note.Text}";
            }
        }

Done.

[johnthagen]

---

Backend/Services/WordService.cs, line 1 at r1 (raw file):

Previously, johnthagen wrote…

Unused import.

Done.

[johnthagen]

---

Backend/Services/WordService.cs, line 186 at r1 (raw file):

Previously, johnthagen wrote…

This comment is not very useful as it duplicates the same information that exists in the next line. Could it be improved or removed?

Done.

[johnthagen]

---

Backend.Tests/Models/WordTests.cs, line 128 at r1 (raw file):

Previously, johnthagen wrote…

Can be const

Done.

[johnthagen]

---

Backend.Tests/Models/WordTests.cs, line 132 at r1 (raw file):

Previously, johnthagen wrote…

Simplify to

expectedNote.Text += $"; {newText}";

Done.

[johnthagen]

---

Backend.Tests/Models/WordTests.cs, line 146 at r1 (raw file):

Previously, johnthagen wrote…

Simplify to

expectedNote.Text += $"; [{newLanguage}] {newNote.Text}";

Done.

[imnasnainaec]

Reviewable status: 1 of 4 files reviewed, 1 unresolved discussion (waiting on @johnthagen)

---

Backend/Services/WordService.cs, line 187 at r1 (raw file):

Previously, johnthagen wrote…

Can we add a test(s) to WordServiceTests.cs to ensure that the Service itself actually adds the Note as expected (i.e. test WordIsUnique)?

Another thing that feels a little odd semantically is that WordIsUnique seems like a read-only checking function, but in reality it is now (or maybe it was before as well?) mutating the Word that is being checked.

This seems like too much going on in one function. We should probably at least change the name to make it obvious that mutation is happening, or (ideally) pull the mutation logic out into a separate function that runs after uniqueness is checked (e.g. UpdateUniqueWord etc.)

Improved comments. Since the "Audio" side of #1584 will require a refactor of WordIsUnique (to return more than a boolean, at the very least), splitting the identification of a duplicate and the mutation of the word being duplicated into two functions could be added to that task.

[johnthagen]

---

Backend/Services/WordService.cs, line 187 at r1 (raw file):

Previously, imnasnainaec (D. Ror.) wrote…

Improved comments. Since the "Audio" side of #1584 will require a refactor of WordIsUnique (to return more than a boolean, at the very least), splitting the identification of a duplicate and the mutation of the word being duplicated into two functions could be added to that task.

Sounds good.

I added a comment to #1584 about this thread.

[johnthagen]

---

Backend/Services/WordService.cs, line 184 at r4 (raw file):

                            foundDuplicateSense = true;

                            oldSense.SemanticDomains.AddRange(newSense.SemanticDomains);

@imnasnainaec Could we still add a test for WordServices that ensures the that the Note is appended by the service?

[johnthagen]

Reviewed 3 of 3 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @imnasnainaec)

[imnasnainaec]

Reviewable status: 4 of 5 files reviewed, 1 unresolved discussion (waiting on @imnasnainaec and @johnthagen)

---

Backend/Services/WordService.cs, line 184 at r4 (raw file):

Previously, johnthagen wrote…

@imnasnainaec Could we still add a test for WordServices that ensures the that the Note is appended by the service?

Done.

[johnthagen]

Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @imnasnainaec)