fix: harden candidate appends and cover the message-row sender roles

4 files changed, 93 insertions, 26 deletions

diff --git a/README.md b/README.md
index 91561f5..70df5ca 100644
--- a/README.md
+++ b/README.md
@@ -391,6 +391,30 @@ name shown in your own language (`In arrivo`). The English one is the safer
 choice: it is the filter's identity rather than its label, so a config written
 that way keeps working whatever `LANG` is set to.
 
+### `~/.config/qtmaildir/business-senders`
+
+Addresses that should read as businesses rather than people, one per line.
+A card's avatar takes its pattern from this: a listed address gets the
+two-tone fill, anything presenting a display name gets the identicon.
+
+    # a comment, and the form the application itself writes
+    # noreply@cofidis.it (47 messages)
+    billing@example.org
+    @newsletter.example.com
+
+An entry is either an exact address or a whole domain written `@example.com`.
+Comments and blank lines are ignored.
+
+After each sync the application appends addresses that look like bulk mail,
+**always commented out**, so nothing changes appearance until you uncomment
+it. Anything already in the file, commented or not, is never proposed again:
+commenting a line out is therefore the permanent way to reject it, while
+deleting it lets that sender be proposed again if they write to you.
+
+The first scan, when the file does not exist or holds no active entry, covers
+the whole database so the list is useful straight away. Afterwards it covers
+the last week's mail.
+
 ### Sent mail
 
 A **Sent** button appears beside the saved queries once at least one account
@@ -429,30 +453,6 @@ Sent mail is presented differently from the rest, because it reads differently:
 This applies only to the Sent button. The same query typed into the bar by hand
 behaves like any other query, threads and all.
 
-### `~/.config/qtmaildir/business-senders`
-
-Addresses that should read as businesses rather than people, one per line.
-A card's avatar takes its pattern from this: a listed address gets the
-two-tone fill, anything presenting a display name gets the identicon.
-
-    # a comment, and the form the application itself writes
-    # noreply@cofidis.it (47 messages)
-    billing@example.org
-    @newsletter.example.com
-
-An entry is either an exact address or a whole domain written `@example.com`.
-Comments and blank lines are ignored.
-
-After each sync the application appends addresses that look like bulk mail,
-**always commented out**, so nothing changes appearance until you uncomment
-it. Anything already in the file, commented or not, is never proposed again:
-commenting a line out is therefore the permanent way to reject it, while
-deleting it lets that sender be proposed again if they write to you.
-
-The first scan, when the file does not exist or holds no active entry, covers
-the whole database so the list is useful straight away. Afterwards it covers
-the last week's mail.
-
 ## The query bar
 
 The bar at the top takes a notmuch query and shows the matching threads.
diff --git a/src/businesssenders.cpp b/src/businesssenders.cpp
index 06f4d43..ab9394d 100644
--- a/src/businesssenders.cpp
+++ b/src/businesssenders.cpp
@@ -102,10 +102,17 @@ void appendCandidates(const QString &path, const QHash<QString, int> &counts)
     // from parse() above, which deliberately drops comments: here a comment is
     // exactly what must be remembered.
     QSet<QString> mentioned;
+    bool needsLeadingNewline = false;
     QFile existing(path);
     if (existing.open(QIODevice::ReadOnly | QIODevice::Text)) {
-        const QStringList lines =
-            QString::fromUtf8(existing.readAll()).split(QLatin1Char('\n'));
+        const QString contents = QString::fromUtf8(existing.readAll());
+        // Hand-editing (the documented workflow) can leave the file without a
+        // trailing newline; appending then glues the first addition onto the
+        // last entry and silently breaks it. Write a newline before the
+        // additions in that case.
+        if (!contents.isEmpty() && !contents.endsWith(QLatin1Char('\n')))
+            needsLeadingNewline = true;
+        const QStringList lines = contents.split(QLatin1Char('\n'));
         for (const QString &raw : lines) {
             QString line = raw.trimmed();
             if (line.startsWith(QLatin1Char('#')))
@@ -140,6 +147,8 @@ void appendCandidates(const QString &path, const QHash<QString, int> &counts)
     if (!file.open(QIODevice::Append | QIODevice::Text))
         return;
     QTextStream out(&file);
+    if (needsLeadingNewline)
+        out << '\n';
     for (const QString &line : additions)
         out << line << '\n';
 }
diff --git a/tests/test_businesssenders.cpp b/tests/test_businesssenders.cpp
index bc6a5f6..fb26e28 100644
--- a/tests/test_businesssenders.cpp
+++ b/tests/test_businesssenders.cpp
@@ -34,6 +34,7 @@ private slots:
     void matchingIsCaseInsensitive();
     void anAbsentFileMatchesNothing();
     void candidatesAreAppendedCommentedOut();
+    void appendingDoesNotCorruptALineThatLacksATrailingNewline();
     void anAddressAlreadyPresentIsNeverReproposed();
     void onlyBulkLookingLocalPartsAreProposed();
     void theFirstRunScansEverything();
@@ -126,6 +127,36 @@ void TestBusinessSenders::candidatesAreAppendedCommentedOut()
                                        QStringLiteral("noreply@cofidis.it")));
 }
 
+void TestBusinessSenders::appendingDoesNotCorruptALineThatLacksATrailingNewline()
+{
+    // Hand-editing, the documented workflow, can leave the file without a
+    // trailing newline. Appending then glued the first candidate onto the last
+    // existing line, silently breaking the user's own active entry so it
+    // stopped matching. The guard writes a newline before the additions.
+    QTemporaryDir dir;
+    const QString path = dir.filePath(QStringLiteral("business-senders"));
+    QFile seed(path);
+    QVERIFY(seed.open(QIODevice::WriteOnly | QIODevice::Text));
+    seed.write("billing@example.org");   // deliberately no trailing newline
+    seed.close();
+
+    QHash<QString, int> counts;
+    counts.insert(QStringLiteral("noreply@a.org"), 3);
+    BusinessSenders::appendCandidates(path, counts);
+
+    // The original entry is intact and still matches.
+    const BusinessSenders::List list = BusinessSenders::load(path);
+    QVERIFY(BusinessSenders::contains(list,
+                                      QStringLiteral("billing@example.org")));
+
+    // ...and the candidate sits on its own commented line, not glued onto it.
+    QFile file(path);
+    QVERIFY(file.open(QIODevice::ReadOnly | QIODevice::Text));
+    const QString written = QString::fromUtf8(file.readAll());
+    QVERIFY(written.contains(QStringLiteral(
+        "billing@example.org\n# noreply@a.org (3 messages)")));
+}
+
 void TestBusinessSenders::anAddressAlreadyPresentIsNeverReproposed()
 {
     QTemporaryDir dir;
diff --git a/tests/test_threadlistmodel.cpp b/tests/test_threadlistmodel.cpp
index 25c89b0..5c10b6c 100644
--- a/tests/test_threadlistmodel.cpp
+++ b/tests/test_threadlistmodel.cpp
@@ -103,6 +103,7 @@ private slots:
     void flatModeIsOffByDefaultAndReversible();
     void recipientsReplaceTheSenderWhenPresent();
     void aRowCarriesItsSenderAndAccountAddress();
+    void aMessageRowCarriesItsOwnSenderAndAddress();
 };
 
 static ThreadSummary makeThread(const QString &id, const QString &subject)
@@ -533,6 +534,32 @@ void TestThreadListModel::aRowCarriesItsSenderAndAccountAddress()
              QStringLiteral("John Doe"));
 }
 
+void TestThreadListModel::aMessageRowCarriesItsOwnSenderAndAddress()
+{
+    // Task 8 counterpart of aRowCarriesItsSenderAndAccountAddress: that test
+    // covers the thread-row branch, and a role added to one branch and not the
+    // other is silently absent with nothing to flag it. A selected reply's
+    // avatar reads these, so the row that actually answers must carry them.
+    ThreadListModel model;
+    model.appendBatch({ makeThread(QStringLiteral("t1"),
+                                   QStringLiteral("A subject")) });
+
+    MessageNode root = makeNode(QStringLiteral("m0@example.org"), 0);
+    MessageNode reply = makeNode(QStringLiteral("m1@example.org"), 1,
+                                 QStringLiteral("Bob <bob@example.org>"));
+    reply.senderAddress = QStringLiteral("bob@example.org");
+    model.setThreadMessages(QStringLiteral("t1"), { root, reply });
+
+    const QModelIndex replyIndex =
+        model.index(0, 0, model.index(0, 0, QModelIndex()));
+    QVERIFY(model.isMessageRow(replyIndex));
+
+    QCOMPARE(replyIndex.data(ThreadListModel::SenderAddressRole).toString(),
+             QStringLiteral("bob@example.org"));
+    QCOMPARE(replyIndex.data(ThreadListModel::SenderNameRole).toString(),
+             QStringLiteral("Bob <bob@example.org>"));
+}
+
 void TestThreadListModel::theReplyCountExcludesTheRootMessage()
 {
     ThreadListModel model;