From b08923df88de7ba03135234aaf3c602f23e49e03 Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Fri, 14 Aug 2026 11:01:58 +0200 Subject: fix(rules): stop a rule with a spaced name from vanishing on save A rule named "justeat orders" in the field labelled Name was written to rules.json correctly and then dropped by every reader, because load() required ^[a-z0-9][a-z0-9-]*$ and the save path validated nothing. The rule stayed in the file, invisible in the dialog, never applied by the post-new hook, and the next save from the dialog would have deleted it outright. The asymmetry was the defect, not the pattern. TagRules::validate() is now the single predicate: the dialog refuses to save against it, and load() uses it to repair rather than drop, so a rule that fails is visible and fixable instead of silently discarded. - The typed name is sanitised into an id when the field is committed, so the field shows what will reach the file. uniqueId() suffixes a collision, since sanitising is many-to-one and can manufacture the duplicate that load() then drops. - An already-legal id is never rewritten, including one like "a---b" that sanitising would otherwise collapse. Rewriting valid ids would churn a file mailctl also reads. - A bad id loads repaired, with the warning kept: what is on disk is not what the hook runs until the file is saved back. Deliberately not mirrored into mailrules.py. The hook tags real mail unattended every ten minutes, where silently renaming an id is worse than dropping the rule; the file converges as soon as the dialog saves. No format change, so no version bump and no two-repo commitment. The load warning was not missing: it had been showing "1 rule could not be read and was skipped" on every open, in the same font and colour as the intro prose two lines above it, and read as more explanation. It is now a red banner beside Save, with an icon and a dismiss button, and it says the rules need attention rather than that they were skipped, which is no longer true. Dismissal is per-appearance only; a persistent one would re-hide the problem that went unnoticed for a session. Both new dialog tests were confirmed to fail with the sanitiser reverted, and the banner's styling, position and dismissal each fail under mutation. 20 of 20 suites green, 34 tests in test_tagrules. Closes item 83. Co-Authored-By: Claude Opus 5 --- CLAUDE.md | 13 + .../2026-08-03-post-0.1.0-usability-closed.md | 163 +++++++++ .../plans/2026-08-03-post-0.1.0-usability.md | 116 +------ src/tagrules.cpp | 108 +++++- src/tagrules.h | 25 ++ src/tagrulesdialog.cpp | 184 +++++++++- src/tagrulesdialog.h | 29 ++ tests/test_tagrules.cpp | 376 ++++++++++++++++++++- 8 files changed, 877 insertions(+), 137 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 10eb843..b66c3b2 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -254,6 +254,19 @@ a failure or a `-1` count fails against correct code. This was recorded in building the rules. Assert on the positional contract, never on a provoked failure. +**A writer that does not validate what its reader requires loses data +silently.** `TagRules::save()` wrote any id and `load()` required +`^[a-z0-9][a-z0-9-]*$`, so a rule named `justeat orders` in a field labelled +**Name** was written correctly, dropped on every read, invisible in the dialog, +still occupying the file, and never applied by the hook. The next save from the +dialog would have deleted it outright. `TagRules::validate()` is now the single +predicate both sides use; a bad id loads REPAIRED rather than dropped, so the +rule can be seen and fixed. Two lessons beyond the fix. The load warning already +existed and was correct and useless, because the rule it named could not be +reached, and a warning the user cannot act on teaches them to ignore warnings. +And the repair belongs in the editor, not in `mailrules.py`: the hook tags real +mail unattended, where a silent rename is worse than a drop. + **Rule counts must count MESSAGES.** `requestCounts` counts threads, which is right for the placeholder pane because a click there produces thread rows. A rule tags messages, so a thread count understates every rule matching part of a diff --git a/docs/superpowers/plans/2026-08-03-post-0.1.0-usability-closed.md b/docs/superpowers/plans/2026-08-03-post-0.1.0-usability-closed.md index 4ea175a..c8ffb53 100644 --- a/docs/superpowers/plans/2026-08-03-post-0.1.0-usability-closed.md +++ b/docs/superpowers/plans/2026-08-03-post-0.1.0-usability-closed.md @@ -1171,6 +1171,40 @@ delivered per-message inspection at size S, touching only `htmlbuilder.cpp` and neither fragile area, but gives no message rows in the list and no reply-tree indentation. The user chose the full model deliberately. +## 23. No way to save a search query from the UI + +**Observed (user, 2026-08-04):** saved queries live in the config file only. +There is no way to keep a query you have just written without editing +`qtmaildir.conf` by hand. Underneath it, every saved query becomes a button +(`src/mainwindow.cpp:557`), so the query row grows without bound and cannot tell +Inbox from a one-off search. + +**Specified 2026-08-13. Read +`specs/2026-08-13-saved-queries-design.md` instead of planning from here.** + +The three things that decide whether this can be picked up: + +- **Saved queries move out of `[queries]` into + `~/.config/qtmaildir/queries.json`**, gaining an order, a `pinned` flag and a + per-query account scope. The INI cannot express order at all: `childKeys()` + returns keys alphabetically and `src/config.cpp:401` already records that a + hand-rolled parser would be needed to change it. +- **It stays a single-repo change.** The format takes the shape of `rules.json`, + a versioned document preserving unknown fields, but none of its + two-implementation machinery, because queries have one reader. +- **`startup_query`'s fallback changes meaning**, from alphabetically-first to + first-in-the-user's-order. User-visible, so this is a minor bump and wants a + changelog line. The README documents `[queries]` in three places, one of which + explains the alphabetical ordering this removes. + +**Relation to item 10.** Item 10 is postponed, but its second half proposed +exactly this: "saved queries that carry their own account scope, so one action +gets there". The account scope in the save dialog answers it as a side effect. +Do not reopen item 10 to do it: the user postponed it and asked that the +remaining work not be proposed unprompted. + +**Item 81 depends on this** and is deliberately not part of it. + ## 24. No right-click actions on the thread list **Observed (user, 2026-08-04):** "right click actions on the list (left pane)". @@ -4532,3 +4566,132 @@ the event loop has run, so the test needs `processEvents` after selecting a rule or it measures one row's height twice. **Size: XS.** +## 82. A saved query cannot be edited, unpinned or deleted from the UI + +**Observed (user, 2026-08-13):** hand-testing item 23. The user saved a query, +then asked how to unpin it, and there is no answer that does not involve either +a text editor or retyping the whole query. + +**Cause:** item 23 specified saving and nothing else, and that is exactly what +shipped. `SaveQueryDialog` opens on the contents of the query BAR, not on a +stored entry, so the only route to changing one field of an existing query is to +reconstruct the whole query, name it identically, and let +`MainWindow::saveCurrentQuery()` replace it by name. There is no delete at any +price: nothing in the UI removes an entry from `queries.json`. + +This is a defect rather than a missing enhancement. An action that creates +something the UI cannot then edit or remove is incomplete, and the user hit it +within minutes of the first hand test. + +**Approach.** A context menu on a saved-query button and on each **More +queries** entry, offering Edit, Unpin (or Pin) and Delete. + +- **Edit** opens `SaveQueryDialog` prefilled from the STORED entry rather than + from the query bar. The dialog already carries every field it needs; what it + lacks is a constructor that takes a `SavedQuery`. +- **Unpin** is a one-field write and does not need the dialog at all. +- **Delete** removes the entry and rewrites the file. + +**Constraints.** + +- `saveCurrentQuery()` already merges an existing entry's `unknown` fields over + the dialog's fresh value, and every one of these paths must do the same or a + field written by a later build is dropped by an edit here. +- Renaming through Edit is a rename, not a second entry: match on the name the + dialog was OPENED with, not the one it returns, or renaming silently creates a + duplicate and leaves the original behind. +- Delete is destructive and the file is user config, so it is one of the few + places in this application that wants a confirmation. The no-confirmation rule + in CLAUDE.md is about tag mutations, which are undoable through the undo + stack; this is not on that stack and cannot be undone. +- A test must exercise every route the way item 75's did not: the dialog's + Cancel goes through `done(int)` and never sends a `QCloseEvent`. + +**Size: S**, and it should land before the saved-query work is called done. + +**Done 2026-08-13.** A context menu on each button and each menu entry, with +Edit, Move to menu / Show as a button, and Delete. Every path goes through one +`replaceSavedQuery()`, matched on the name the dialog was OPENED with, so a +rename replaces rather than duplicating, and merging the stored entry's unknown +fields in one place rather than three. + +Two things the approach above did not anticipate. A GENERATED entry has no +query to edit, so the dialog shows its composed query read-only rather than +offering a field that changes nothing, and carries `generated` and `flat` +through an edit rather than letting it decay into a plain entry holding a +snapshot. And the overwrite notice had to learn to ignore the entry being +edited: warning that "Inbox" already exists while editing Inbox is noise. + +It also exposed a defect that predated it. `rebuildSavedQueryRow()` called +`deleteLater()` on the old row, which defers destruction to the event loop, so +the stale row went on answering `findChild()` and every lookup after a rebuild +saw the state from before the edit. It was already reachable from the save path. +Fixed by reparenting the row out immediately. + +The unknown-fields test initially passed against the merge being deleted: it +drove UNPIN, which copies the stored entry and therefore carries `unknown` +along by itself. It now goes through the edit path with a replacement that has +none, which is what the dialog actually returns. + + +## 83. A rule named with spaces is written to the file and dropped by every reader + +**Observed (user, 2026-08-14):** a rule filled in from the dialog, with the +count checked against the preview, was gone on reopening the window. Repeated +attempts under different names lost it every time. Closing and restarting the +application did not bring it back. + +**Cause (verified against the live file).** The rule was on disk, exactly once, +with its query, tags and note intact. Its `id` was `justeat orders`, and +`TagRules::load()` required `^[a-z0-9][a-z0-9-]*$`, so it was discarded on every +read while continuing to occupy the file. + +The fault was the asymmetry, not the pattern. The save path took +`m_id->text().trimmed()` verbatim and wrote it; the load path validated and +dropped. A rule could therefore be written correctly and never come back, and +nothing in between reported it. `mailrules.py` enforces the same pattern, so +the rule was not tagging mail either: it had been inert since the day it was +written. + +Two aggravating properties, both worse than the drop itself. The dropped rule +was invisible in the dialog while still present in the file, so the next save +from the dialog would have deleted it permanently, and the field is labelled +**Name**, which invites prose. + +**The warning was not missing.** `showWarnings()` already surfaced "1 rule could +not be read and was skipped" on every open. It was correct, it was ignored, and +it was a dead end: the rule it named could not be reached, so there was nothing +to do about it. A warning the user cannot act on trains them to stop reading +warnings. + +**Fixed 2026-08-14, three parts.** + +- **The name is sanitised into an id when the field is committed**, not on save, + so what the field shows is what reaches the file. `TagRules::sanitiseId()` + lowercases, collapses every run of anything else to one dash and trims the + ends; `uniqueId()` adds a numeric suffix, because sanitising is many-to-one + and manufactures the duplicate that `load()` then drops. +- **`TagRules::validate()` is the one predicate**, run before the write and + reported through the warning label rather than a modal, since `saveForTest()` + drives this path directly and a `QMessageBox` there would hang the suite. +- **A bad id now loads repaired rather than dropped**, so the rule is visible + and fixable. The warning still fires, because what is on disk is not what the + hook runs until the file is saved back. + +**Deliberately not mirrored into `mailrules.py`.** The hook applies rules to +real mail every ten minutes with nobody watching its output; silently renaming +an id there would tag mail under a rule the file does not contain. Dropping is +the correct failure for the hook and repairing is the correct one for the +editor, and the file converges as soon as the dialog saves. No format change and +no version bump, so this stayed a single-repo fix. + +**An already-legal id is never rewritten**, including one like `a---b` that +sanitising would otherwise collapse. Rewriting valid ids would churn a file +mailctl also reads and show a diff the user never made. A test asserting the +collapse was written first and was wrong; the code was right. + +**Verification.** Both new dialog tests were confirmed to fail with the +sanitiser reverted, one on the field contents and one on the save being refused. +32 tests in `test_tagrules`, 20 of 20 suites green. The user's own rule was +repaired in place to `justeat-orders` and verified to load through +`mailrules.py`, 18 rules and no warnings. diff --git a/docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md b/docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md index 5e97337..e60b88c 100644 --- a/docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md +++ b/docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md @@ -87,7 +87,7 @@ taking that too literally. | 20 | Thread view does not match the user's mental model | presentation | L | **done** 2026-08-10, as the card list; see 53 | | 21 | Default shortcuts are not sensible enough | discoverability | S | open | | 22 | Translatability audit and i18n wiring | correctness | M | open | -| 23 | No way to save a search query from the UI | workflow | M | open, specified 2026-08-13; see `specs/2026-08-13-saved-queries-design.md`. Saved queries move to `queries.json`, carrying order, `pinned` and account scope | +| 23 | No way to save a search query from the UI | workflow | M | **done** 2026-08-13, shipped in 0.18.0; see `specs/2026-08-13-saved-queries-design.md` | | 24 | No right-click actions on the thread list | discoverability | S | **done** | | 25 | No select-all, and bulk actions are undiscoverable | workflow | S | **done** | | 26 | No way to add or remove an arbitrary tag from the UI | workflow | S | **done** | @@ -139,14 +139,15 @@ taking that too literally. | 72 | No khard/khal integration | workflow | ? | open, unspecified; the user places it after send, so v2 at the earliest | | 73 | This backlog is past four thousand lines | maintenance | S | **done** 2026-08-13; 5056 lines to 578, closed sections moved to `2026-08-03-post-0.1.0-usability-closed.md` | | 74 | "Searching..." keeps claiming a query is running while rows are already arriving | feedback | XS | open; cause measured 2026-08-11, the delay itself is the cold page cache and is not fixable here | -| 75 | The tagging rules window forgets its size and its column widths | persistence | S | **done** 2026-08-13 on `rule-builder`, unreleased. The window-kind question is left open, see the closed-items file | -| 76 | Every field in the rules dialog is free text, so a rule is easy to get wrong | workflow | M | **done** 2026-08-13 on `rule-builder`, unreleased. See `specs/2026-08-13-rule-builder-design.md` | -| 77 | No way to see what a rule would collect, in the thread list | workflow | S | **done** 2026-08-13 on `rule-builder`, unreleased | +| 75 | The tagging rules window forgets its size and its column widths | persistence | S | **done** 2026-08-13, shipped in 0.17.0. The window-kind question is left open, see the closed-items file | +| 76 | Every field in the rules dialog is free text, so a rule is easy to get wrong | workflow | M | **done** 2026-08-13, shipped in 0.17.0. See `specs/2026-08-13-rule-builder-design.md` | +| 77 | No way to see what a rule would collect, in the thread list | workflow | S | **done** 2026-08-13, shipped in 0.17.0 | | 78 | No way to build a rule from something visible in a message | workflow | M | open; wants 76 first, so the created rule lands in a form that can hold it | -| 80 | A rule with many conditions squeezes the rule list to one visible row | defect | XS | **done** 2026-08-13 on `rule-builder`, unreleased. Follows item 76 | +| 80 | A rule with many conditions squeezes the rule list to one visible row | defect | XS | **done** 2026-08-13, shipped in 0.17.0. Follows item 76 | | 79 | Opening the rules dialog and saving destroys the first rule | defect | XS | **fixed on `rule-builder`** 2026-08-13, unreleased. Shipped in 0.16.0; damaged one real rule, repaired by hand | -| 81 | No way to turn a saved query into a tagging rule | workflow | S | open; depends on 23, which builds the dialog, and is excluded from its spec on purpose. Writes to the shared rules file, so it spans this repo and `mailctl` | -| 82 | A saved query cannot be edited, unpinned or deleted from the UI | defect | S | **done** 2026-08-13 on `saved-queries`, unreleased. Right-click offers Edit, Pin/Unpin and Delete | +| 81 | No way to turn a saved query into a tagging rule | workflow | S | open; 23 has shipped, so the dialog it depends on exists. Writes to the shared rules file, so it spans this repo and `mailctl` | +| 82 | A saved query cannot be edited, unpinned or deleted from the UI | defect | S | **done** 2026-08-13, shipped in 0.18.0. Right-click offers Edit, Pin/Unpin and Delete | +| 83 | A rule named with spaces is written to the file and dropped by every reader | defect | S | **done** 2026-08-14, unreleased. The name is sanitised into an id, save validates, a bad id loads for repair | Sizes are rough: XS under an hour, S a sitting, M a session. @@ -210,40 +211,6 @@ literal, the descriptions are prose. **Verification:** run `lupdate` and read the generated `.ts`. A string that does not appear there is not translatable, whatever the source looks like. -## 23. No way to save a search query from the UI - -**Observed (user, 2026-08-04):** saved queries live in the config file only. -There is no way to keep a query you have just written without editing -`qtmaildir.conf` by hand. Underneath it, every saved query becomes a button -(`src/mainwindow.cpp:557`), so the query row grows without bound and cannot tell -Inbox from a one-off search. - -**Specified 2026-08-13. Read -`specs/2026-08-13-saved-queries-design.md` instead of planning from here.** - -The three things that decide whether this can be picked up: - -- **Saved queries move out of `[queries]` into - `~/.config/qtmaildir/queries.json`**, gaining an order, a `pinned` flag and a - per-query account scope. The INI cannot express order at all: `childKeys()` - returns keys alphabetically and `src/config.cpp:401` already records that a - hand-rolled parser would be needed to change it. -- **It stays a single-repo change.** The format takes the shape of `rules.json`, - a versioned document preserving unknown fields, but none of its - two-implementation machinery, because queries have one reader. -- **`startup_query`'s fallback changes meaning**, from alphabetically-first to - first-in-the-user's-order. User-visible, so this is a minor bump and wants a - changelog line. The README documents `[queries]` in three places, one of which - explains the alphabetical ordering this removes. - -**Relation to item 10.** Item 10 is postponed, but its second half proposed -exactly this: "saved queries that carry their own account scope, so one action -gets there". The account scope in the save dialog answers it as a side effect. -Do not reopen item 10 to do it: the user postponed it and asked that the -remaining work not be proposed unprompted. - -**Item 81 depends on this** and is deliberately not part of it. - ## 36. `test_mainwindow` cannot reach the worker **Observed:** twice in one session (0.8.0), a defect could not be given a @@ -575,73 +542,6 @@ failing silently. **Size: S** on top of 23, and not meaningful before it. -## 82. A saved query cannot be edited, unpinned or deleted from the UI - -**Observed (user, 2026-08-13):** hand-testing item 23. The user saved a query, -then asked how to unpin it, and there is no answer that does not involve either -a text editor or retyping the whole query. - -**Cause:** item 23 specified saving and nothing else, and that is exactly what -shipped. `SaveQueryDialog` opens on the contents of the query BAR, not on a -stored entry, so the only route to changing one field of an existing query is to -reconstruct the whole query, name it identically, and let -`MainWindow::saveCurrentQuery()` replace it by name. There is no delete at any -price: nothing in the UI removes an entry from `queries.json`. - -This is a defect rather than a missing enhancement. An action that creates -something the UI cannot then edit or remove is incomplete, and the user hit it -within minutes of the first hand test. - -**Approach.** A context menu on a saved-query button and on each **More -queries** entry, offering Edit, Unpin (or Pin) and Delete. - -- **Edit** opens `SaveQueryDialog` prefilled from the STORED entry rather than - from the query bar. The dialog already carries every field it needs; what it - lacks is a constructor that takes a `SavedQuery`. -- **Unpin** is a one-field write and does not need the dialog at all. -- **Delete** removes the entry and rewrites the file. - -**Constraints.** - -- `saveCurrentQuery()` already merges an existing entry's `unknown` fields over - the dialog's fresh value, and every one of these paths must do the same or a - field written by a later build is dropped by an edit here. -- Renaming through Edit is a rename, not a second entry: match on the name the - dialog was OPENED with, not the one it returns, or renaming silently creates a - duplicate and leaves the original behind. -- Delete is destructive and the file is user config, so it is one of the few - places in this application that wants a confirmation. The no-confirmation rule - in CLAUDE.md is about tag mutations, which are undoable through the undo - stack; this is not on that stack and cannot be undone. -- A test must exercise every route the way item 75's did not: the dialog's - Cancel goes through `done(int)` and never sends a `QCloseEvent`. - -**Size: S**, and it should land before the saved-query work is called done. - -**Done 2026-08-13.** A context menu on each button and each menu entry, with -Edit, Move to menu / Show as a button, and Delete. Every path goes through one -`replaceSavedQuery()`, matched on the name the dialog was OPENED with, so a -rename replaces rather than duplicating, and merging the stored entry's unknown -fields in one place rather than three. - -Two things the approach above did not anticipate. A GENERATED entry has no -query to edit, so the dialog shows its composed query read-only rather than -offering a field that changes nothing, and carries `generated` and `flat` -through an edit rather than letting it decay into a plain entry holding a -snapshot. And the overwrite notice had to learn to ignore the entry being -edited: warning that "Inbox" already exists while editing Inbox is noise. - -It also exposed a defect that predated it. `rebuildSavedQueryRow()` called -`deleteLater()` on the old row, which defers destruction to the event loop, so -the stale row went on answering `findChild()` and every lookup after a rebuild -saw the state from before the edit. It was already reachable from the save path. -Fixed by reparenting the row out immediately. - -The unknown-fields test initially passed against the merge being deleted: it -drove UNPIN, which copies the stored entry and therefore carries `unknown` -along by itself. It now goes through the edit path with a replacement that has -none, which is what the dialog actually returns. - ## Deferred, unsized, or split out Items noted while triaging but not part of the original list. Same numbering diff --git a/src/tagrules.cpp b/src/tagrules.cpp index 39cc529..0049278 100644 --- a/src/tagrules.cpp +++ b/src/tagrules.cpp @@ -58,8 +58,96 @@ QStringList stringsOf(const QJsonValue &value) return out; } +/// The id pattern, in one place. mailrules.py enforces the same one; see +/// "Changing the shared rule format" in CLAUDE.md before touching it. +const QRegularExpression &idPattern() +{ + static const QRegularExpression pattern( + QStringLiteral("^[a-z0-9][a-z0-9-]*$")); + return pattern; +} + } // namespace +bool TagRules::isValidId(const QString &id) +{ + return idPattern().match(id).hasMatch(); +} + +QString TagRules::sanitiseId(const QString &name) +{ + // Untouched when already legal: sanitising every id on load would rewrite + // a good file and show mailctl a diff the user never made. + if (isValidId(name)) + return name; + + QString out; + out.reserve(name.size()); + for (const QChar ch : name.toLower()) { + if ((ch >= QLatin1Char('a') && ch <= QLatin1Char('z')) + || (ch >= QLatin1Char('0') && ch <= QLatin1Char('9'))) { + out.append(ch); + } else if (!out.isEmpty() && !out.endsWith(QLatin1Char('-'))) { + // One dash per run of anything else, so "Notify: PayPal!" does not + // become "notify--paypal-". + out.append(QLatin1Char('-')); + } + } + while (out.endsWith(QLatin1Char('-'))) + out.chop(1); + + // Empty rather than a manufactured id. The caller knows what the rule is + // and can name it; this function inventing "rule-1" would hide the fact + // that nothing of the typed name survived. + return out; +} + +QString TagRules::uniqueId(const QString &name, const QStringList &taken) +{ + QString base = sanitiseId(name); + if (base.isEmpty()) + base = QStringLiteral("rule"); + if (!taken.contains(base)) + return base; + + // Starts at 2: the unsuffixed id is the first, so "-2" reads as the second + // rule of that name rather than as an index. + for (int suffix = 2;; ++suffix) { + const QString candidate = + base + QLatin1Char('-') + QString::number(suffix); + if (!taken.contains(candidate)) + return candidate; + } +} + +QStringList TagRules::validate(const QList &rules) +{ + QStringList problems; + QStringList seen; + for (const TagRule &rule : rules) { + const QString where = + rule.id.isEmpty() ? QObject::tr("(unnamed)") : rule.id; + + if (!isValidId(rule.id)) { + problems.append( + QObject::tr("'%1': the name must be lowercase letters, digits " + "and dashes").arg(where)); + } else if (seen.contains(rule.id)) { + problems.append( + QObject::tr("'%1': another rule already has this name") + .arg(where)); + } + seen.append(rule.id); + + if (rule.query.trimmed().isEmpty()) + problems.append(QObject::tr("'%1': no query").arg(where)); + if (rule.add.isEmpty() && rule.remove.isEmpty()) + problems.append(QObject::tr("'%1': adds and removes nothing") + .arg(where)); + } + return problems; +} + QString TagRules::defaultPath() { // Not QStandardPaths::ConfigLocation: that appends the organization and @@ -121,9 +209,12 @@ void TagRules::load(const QString &path) } // An id is a handle: a UI selects on it and a diff tracks it. - static const QRegularExpression idPattern( - QStringLiteral("^[a-z0-9][a-z0-9-]*$")); - + // + // A bad id is REPAIRED rather than dropped. Dropping it made the rule + // invisible in the dialog while it still occupied the file, so the next + // save deleted it outright: the user wrote a rule, saw it vanish, wrote it + // again, and lost it again. The warning still fires, because what is on + // disk is not what the hook runs until the file is saved back. QStringList seen; const QJsonArray array = root.value(QStringLiteral("rules")).toArray(); for (int index = 0; index < array.size(); ++index) { @@ -132,12 +223,13 @@ void TagRules::load(const QString &path) TagRule rule; rule.id = object.value(QStringLiteral("id")).toString(); - if (!idPattern.match(rule.id).hasMatch()) { + if (!isValidId(rule.id)) { + const QString repaired = uniqueId(rule.id, seen); m_warnings.append( - QObject::tr("%1: id '%2' is missing or not lowercase letters, " - "digits and dashes; dropped") - .arg(where, rule.id)); - continue; + QObject::tr("%1: name '%2' is not lowercase letters, digits " + "and dashes; loaded as '%3'. Save to keep it.") + .arg(where, rule.id, repaired)); + rule.id = repaired; } if (seen.contains(rule.id)) { diff --git a/src/tagrules.h b/src/tagrules.h index 8f75b38..4d3f0ab 100644 --- a/src/tagrules.h +++ b/src/tagrules.h @@ -75,6 +75,31 @@ public: QStringList warnings() const { return m_warnings; } + /// The one predicate. load() drops or repairs by it, the dialog refuses to + /// save against it, and mailrules.py enforces the same pattern in the + /// companion repo. Anything failing this is invisible to the post-new hook. + static bool isValidId(const QString &id); + + /// A typed name reduced to a legal id: lowercased, every run of anything + /// else collapsed to one dash, dashes trimmed off both ends. + /// + /// Returns an EMPTY string when nothing legal survives ("!!!"), because an + /// empty id is not writable and the caller must decide the fallback rather + /// than have one invented here. Already-legal ids pass through untouched, + /// so loading a good file never rewrites it. + static QString sanitiseId(const QString &name); + + /// sanitiseId plus a numeric suffix when the result is already taken. + /// Sanitising is many-to-one, so it manufactures duplicates that load() + /// would then drop; this is what stops the second rule becoming the first. + static QString uniqueId(const QString &name, const QStringList &taken); + + /// Every reason these rules would not survive a reload, one string each, + /// empty when they all would. Written for the save path: the defect this + /// answers is that save() wrote anything and load() validated, so a rule + /// could reach the file and never come back. + static QStringList validate(const QList &rules); + /// No file yet, as distinct from a file that would not load. A fresh /// install is not an error and must not be reported as one. bool missing() const { return m_missing; } diff --git a/src/tagrulesdialog.cpp b/src/tagrulesdialog.cpp index 4fca971..226a564 100644 --- a/src/tagrulesdialog.cpp +++ b/src/tagrulesdialog.cpp @@ -97,11 +97,6 @@ TagRulesDialog::TagRulesDialog(QWidget *parent) intro->setWordWrap(true); layout->addWidget(intro); - m_warningLabel = new QLabel(this); - m_warningLabel->setWordWrap(true); - m_warningLabel->setVisible(false); - layout->addWidget(m_warningLabel); - m_list = new QTreeWidget(this); m_list->setColumnCount(ColumnCount + 1); m_list->setHeaderLabels({ tr("On"), tr("Stage"), tr("Rule"), tr("Tags"), @@ -242,6 +237,62 @@ TagRulesDialog::TagRulesDialog(QWidget *parent) buttons->addWidget(refreshButton); layout->addLayout(buttons); + // Beside Save, not under the intro. It sat below the header in the same + // font and colour as the prose around it, so it read as more explanatory + // text: the user missed "1 rule could not be read and was skipped" on + // every open of this dialog while hunting the rule it was telling them + // about. Down here it is next to the button whose outcome it reports, and + // it is the only red thing in the window. + // + // A palette role would be theme-correct and is not usable: the warning has + // to stand out against BOTH a light and a dark desktop, and no role means + // "alarming" in both. The colours are therefore literal, chosen to pass + // contrast either way, which is the same reasoning the tag chips use. + // The label and its dismiss button share a banner widget, so hiding the + // warning takes the button with it. Visibility is the banner's; the label + // itself stays visible inside it and setWarning() is still the one route. + m_warningBanner = new QWidget(this); + m_warningBanner->setVisible(false); + m_warningBanner->setStyleSheet(QStringLiteral( + "QWidget { background-color: #b3261e; border-radius: 4px; }")); + + auto *warningRow = new QHBoxLayout(m_warningBanner); + warningRow->setContentsMargins(8, 6, 6, 6); + + m_warningLabel = new QLabel(m_warningBanner); + m_warningLabel->setWordWrap(true); + // Plain text: these strings interpolate ids and queries read from the + // file, and a query holding '<' would otherwise be eaten as markup. + m_warningLabel->setTextFormat(Qt::PlainText); + m_warningLabel->setStyleSheet(QStringLiteral( + "QLabel { color: #ffffff; font-weight: bold; background: transparent; }")); + warningRow->addWidget(m_warningLabel, 1); + + m_warningClose = new QPushButton(QStringLiteral("✕"), m_warningBanner); + m_warningClose->setToolTip(tr("Dismiss")); + m_warningClose->setFlat(true); + m_warningClose->setCursor(Qt::ArrowCursor); + m_warningClose->setFixedSize(22, 22); + // Focus would put a highlight ring on the banner and let Space dismiss a + // warning the user is only tabbing past. + m_warningClose->setFocusPolicy(Qt::NoFocus); + m_warningClose->setStyleSheet(QStringLiteral( + "QPushButton { color: #ffffff; background: transparent; border: none;" + " font-weight: bold; }" + "QPushButton:hover { background-color: rgba(255, 255, 255, 60);" + " border-radius: 11px; }")); + warningRow->addWidget(m_warningClose, 0, Qt::AlignTop); + + // Dismissed for THIS appearance only, never persistently. The message it + // most often carries is that the file on disk is not what the hook runs, + // and a stored "do not show again" would re-hide exactly the problem that + // went unnoticed for a whole session. The next warning shows it again, + // including on the next open with the file still unrepaired. + connect(m_warningClose, &QPushButton::clicked, + this, [this] { setWarning(QString()); }); + + layout->addWidget(m_warningBanner); + auto *box = new QDialogButtonBox(QDialogButtonBox::Save | QDialogButtonBox::Cancel, this); @@ -417,17 +468,35 @@ void TagRulesDialog::reloadListForTest() reloadList(); } +void TagRulesDialog::setWarning(const QString &text) +{ + if (text.isEmpty()) { + m_warningLabel->clear(); + m_warningBanner->setVisible(false); + return; + } + // One route in, so the icon and the styling cannot drift apart between the + // load path and the save refusal. The glyph is part of the string rather + // than a second widget: it has to survive word wrap without leaving an + // icon stranded beside an empty line. + m_warningLabel->setText(QStringLiteral("⚠ ") + text); + m_warningBanner->setVisible(true); +} + void TagRulesDialog::showWarnings() { const QStringList warnings = m_rules.warnings(); if (warnings.isEmpty()) { - m_warningLabel->setVisible(false); + setWarning(QString()); return; } - m_warningLabel->setText( - tr("%n rule(s) could not be read and were skipped: %1", "", - warnings.size()).arg(warnings.join(QStringLiteral("; ")))); - m_warningLabel->setVisible(true); + // Not "skipped" any more: a rule with a bad name is loaded repaired, and + // saying it was skipped would send the user looking for something that is + // in front of them. What is true of every warning here is that the file on + // disk is not yet what the hook will run. + setWarning(tr("%n rule(s) in the file need attention: %1. Save to write " + "them back.", "", warnings.size()) + .arg(warnings.join(QStringLiteral("; ")))); } void TagRulesDialog::fillItem(QTreeWidgetItem *item, const TagRule &rule) const @@ -541,7 +610,24 @@ void TagRulesDialog::applyEditsToCurrentRule() return; TagRule &rule = m_working[index]; - rule.id = m_id->text().trimmed(); + + // Sanitised as it is committed, not on save, so what the field shows is + // what reaches the file. The field is labelled "Name" and a person types + // "Justeat orders" into it; an id with a space is written happily and then + // dropped by every reader, which is the defect this answers. + QStringList taken; + for (int other = 0; other < m_working.size(); ++other) { + if (other != index) + taken.append(m_working.at(other).id); + } + const QString typed = m_id->text().trimmed(); + rule.id = TagRules::isValidId(typed) ? typed + : TagRules::uniqueId(typed, taken); + if (rule.id != typed) { + const QSignalBlocker blocker(m_id); + m_id->setText(rule.id); + } + rule.stage = m_stage->value(); rule.enabled = m_enabled->isChecked(); rule.add = splitTags(m_add->text()); @@ -629,6 +715,20 @@ void TagRulesDialog::onSave() { applyEditsToCurrentRule(); + // Validated against the same predicate load() uses. Writing a rule that + // cannot be read back is what made a rule disappear: the file was correct, + // every reader dropped it, and nothing said so at the point of the write. + // Reported through the warning label rather than a modal, as the text-mode + // refusal already is: a QMessageBox inside onSave() would hang the suite, + // which drives this path directly through saveForTest(). + const QStringList problems = TagRules::validate(m_working); + if (!problems.isEmpty()) { + setWarning(tr("%n rule(s) cannot be saved as they are: %1", "", + problems.size()) + .arg(problems.join(QStringLiteral("; ")))); + return; + } + m_rules.setRules(m_working); if (!m_rules.save()) { QMessageBox::warning(this, tr("Tagging rules"), @@ -667,6 +767,59 @@ void TagRulesDialog::setTextModeForTest(bool on) m_textMode->setChecked(on); } +void TagRulesDialog::setNameForTest(const QString &name) +{ + m_id->setText(name); + // editingFinished is what leaving the field emits, and it is where the + // sanitiser hangs. setText() alone does not emit it. + emit m_id->editingFinished(); +} + +QString TagRulesDialog::nameLineForTest() const +{ + return m_id->text(); +} + +QString TagRulesDialog::warningStyleForTest() const +{ + // Both: the fill is the banner's and the text colour is the label's, so + // reading only one of them would miss half the styling. + return m_warningBanner->styleSheet() + m_warningLabel->styleSheet(); +} + +void TagRulesDialog::dismissWarningForTest() +{ + m_warningClose->click(); +} + +Qt::TextFormat TagRulesDialog::warningTextFormatForTest() const +{ + return m_warningLabel->textFormat(); +} + +bool TagRulesDialog::warningIsBelowTheRuleListForTest() const +{ + // By layout position rather than by coordinates: the offscreen platform + // does not lay a dialog out the way a real one is, so a y() comparison + // would assert about the platform. indexOf() on the shared parent layout + // is exact and true in both. + auto *parent = qobject_cast(layout()); + if (!parent) + return false; + return parent->indexOf(m_warningBanner) > parent->indexOf(m_splitter); +} + +int TagRulesDialog::ruleCountForTest() const +{ + return m_working.size(); +} + +void TagRulesDialog::setTagsForTest(const QString &tags) +{ + m_add->setText(tags); + emit m_add->editingFinished(); +} + bool TagRulesDialog::textModeToggleIsReachableForTest() const { // isVisibleTo rather than isVisible: nothing is isVisible() on a dialog @@ -681,8 +834,10 @@ QString TagRulesDialog::warningTextForTest() const // so it would report no warning whatever the label held. isVisibleTo() // answers the question actually being asked: would this be on screen if // the dialog were. - return m_warningLabel->isVisibleTo(this) ? m_warningLabel->text() - : QString(); + // The BANNER carries the visibility now: the label stays visible inside it + // and would report a dismissed warning as still showing. + return m_warningBanner->isVisibleTo(this) ? m_warningLabel->text() + : QString(); } void TagRulesDialog::selectRuleForTest(int index) @@ -922,10 +1077,9 @@ void TagRulesDialog::setTextMode(bool on) if (!parsed.parsed) { const QSignalBlocker block(m_textMode); m_textMode->setChecked(true); - m_warningLabel->setText( + setWarning( tr("This query is more than the builder can show, so it stays as " "text. It is still saved and applied normally.")); - m_warningLabel->setVisible(true); return; } diff --git a/src/tagrulesdialog.h b/src/tagrulesdialog.h index e8979fe..8ba656b 100644 --- a/src/tagrulesdialog.h +++ b/src/tagrulesdialog.h @@ -89,6 +89,28 @@ public: void setTextModeForTest(bool on); QString warningTextForTest() const; + /// The warning's appearance and place, asserted as widget properties. A + /// render probe cannot carry this: see "Rendering probes lie" in CLAUDE.md. + QString warningStyleForTest() const; + Qt::TextFormat warningTextFormatForTest() const; + bool warningIsBelowTheRuleListForTest() const; + + /// Clicks the warning's dismiss button, so the test drives the same signal + /// the user's click does rather than calling setWarning() behind it. + void dismissWarningForTest(); + + /// Types a name and commits it the way leaving the field does. The commit + /// is the point: the sanitiser runs on editingFinished, so a test that + /// only calls setText() asserts against a field nothing has processed. + void setNameForTest(const QString &name); + QString nameLineForTest() const; + + /// Adding and filling a rule the way the buttons do, so a test can drive + /// the whole journey the user takes rather than only its last step. + int ruleCountForTest() const; + void addRuleForTest() { onAddRule(); } + void setTagsForTest(const QString &tags); + /// Whether the text-mode toggle would be on screen. A toggle that hides /// itself when switched on is a one-way trip, and asserting only on the /// checked STATE passes against that, since the state is still readable @@ -169,6 +191,11 @@ private: void restoreUiState(); void saveUiState(); void showWarnings(); + + /// The only way the warning label is written. An empty string hides it. + /// One route in so the icon and the red styling cannot drift between the + /// load path, the save refusal and the text-mode notice. + void setWarning(const QString &text); int currentIndex() const; /// Writes one rule's summary onto its row. Shared by reloadList() and @@ -232,6 +259,8 @@ private: bool m_countColumnSized = false; QTreeWidget *m_list = nullptr; + QWidget *m_warningBanner = nullptr; + QPushButton *m_warningClose = nullptr; QLineEdit *m_id = nullptr; QLineEdit *m_add = nullptr; QLineEdit *m_remove = nullptr; diff --git a/tests/test_tagrules.cpp b/tests/test_tagrules.cpp index ed3a06d..cc28711 100644 --- a/tests/test_tagrules.cpp +++ b/tests/test_tagrules.cpp @@ -38,6 +38,15 @@ private slots: void aRuleLoadsWithEveryField(); void absentFieldsTakeTheirDefaults(); void aMalformedRuleIsDroppedWithAWarning(); + void aRuleWithABadIdLoadsForRepairRatherThanVanishing(); + void aTypedNameIsSanitisedIntoAnId(); + void aSanitisedNameThatCollidesGetsItsOwnId(); + void savingIsRefusedWhenARuleWouldNotLoadBack(); + void aRepairedIdSurvivesASaveAndReload(); + void theWarningReadsAsAWarningAndSitsBesideSave(); + void aDismissedWarningComesBackWhenThereIsSomethingNewToSay(); + void aNameTypedWithSpacesIsSanitisedInTheField(); + void aRuleAddedAndNamedInTheDialogSurvivesAReopen(); void unknownFieldsSurviveASave(); void stageOrderPutsAccountsFirst(); void aQueryWithQuotesRoundTrips(); @@ -128,16 +137,19 @@ void TestTagRules::absentFieldsTakeTheirDefaults() void TestTagRules::aMalformedRuleIsDroppedWithAWarning() { - // One bad rule must not cost the others. Four separate defects, and the - // good rule sits first so a parser that stops at the first problem is - // caught by the count rather than by an empty list. + // One bad rule must not cost the others. The good rule sits first so a + // parser that stops at the first problem is caught by the count rather + // than by an empty list. + // + // A rule with nothing to run is still dropped: no query and no tags are + // both unrepairable without inventing the user's intent. A rule whose only + // fault is its ID is NOT dropped any more, see the next test. const QString path = writeRules(R"({ "version": 1, "rules": [ {"id": "good", "add": ["x"], "query": "from:a@example.com"}, {"id": "no-query", "add": ["y"]}, - {"id": "no-tags", "query": "from:b@example.com"}, - {"id": "Bad Id", "add": ["z"], "query": "from:c@example.com"} + {"id": "no-tags", "query": "from:b@example.com"} ] })"); @@ -146,7 +158,164 @@ void TestTagRules::aMalformedRuleIsDroppedWithAWarning() QCOMPARE(rules.rules().size(), 1); QCOMPARE(rules.rules().first().id, QStringLiteral("good")); - QCOMPARE(rules.warnings().size(), 3); + QCOMPARE(rules.warnings().size(), 2); +} + +void TestTagRules::aRuleWithABadIdLoadsForRepairRatherThanVanishing() +{ + // The defect this whole change exists for. A rule saved with a space in + // its id was written to the file correctly, dropped on every load, and so + // was invisible in the dialog while still occupying the file. The next + // save from the dialog would then have deleted it for good. + // + // It now loads, carrying its repaired id, so the dialog can show it and + // the user can fix it. The warning still fires: the file on disk is not + // what the hook will run until it is saved back. + const QString path = writeRules(R"({ + "version": 1, + "rules": [ + {"id": "justeat orders", "add": ["promo"], + "query": "from:no-reply@order.example.com"} + ] + })"); + + TagRules rules; + rules.load(path); + + QCOMPARE(rules.rules().size(), 1); + QCOMPARE(rules.rules().first().id, QStringLiteral("justeat-orders")); + QCOMPARE(rules.rules().first().add, QStringList{ QStringLiteral("promo") }); + QCOMPARE(rules.warnings().size(), 1); + QVERIFY(rules.warnings().first().contains(QStringLiteral("justeat orders"))); +} + +void TestTagRules::aTypedNameIsSanitisedIntoAnId() +{ + // Spaces, capitals and punctuation are what a person types into a field + // labelled "Name". Each case here is one the user is likely to produce, + // and every result has to satisfy ^[a-z0-9][a-z0-9-]*$ or the hook drops + // it. + QCOMPARE(TagRules::sanitiseId(QStringLiteral("justeat orders")), + QStringLiteral("justeat-orders")); + QCOMPARE(TagRules::sanitiseId(QStringLiteral("JustEat Orders")), + QStringLiteral("justeat-orders")); + QCOMPARE(TagRules::sanitiseId(QStringLiteral("Notify: PayPal!")), + QStringLiteral("notify-paypal")); + QCOMPARE(TagRules::sanitiseId(QStringLiteral(" spaced out ")), + QStringLiteral("spaced-out")); + QCOMPARE(TagRules::sanitiseId(QStringLiteral("-leading-dash")), + QStringLiteral("leading-dash")); + + // A run of dashes is collapsed only when the name needed sanitising at + // all: "a---b" already satisfies the pattern and is left exactly as it is, + // because rewriting legal ids would churn the file mailctl also reads. + QCOMPARE(TagRules::sanitiseId(QStringLiteral("a---b")), + QStringLiteral("a---b")); + QCOMPARE(TagRules::sanitiseId(QStringLiteral("a - - b")), + QStringLiteral("a-b")); + QCOMPARE(TagRules::sanitiseId(QStringLiteral("mailing-list/SBo")), + QStringLiteral("mailing-list-sbo")); + + // An id may not START with a dash or a digit-less symbol run, and a name + // made only of punctuation sanitises to nothing. Empty is not a legal id, + // so the caller has to supply a fallback rather than writing one out. + QCOMPARE(TagRules::sanitiseId(QStringLiteral("!!!")), QString()); + QCOMPARE(TagRules::sanitiseId(QString()), QString()); + + // Already valid ids pass through untouched, or every load would rewrite + // the file and show mailctl a diff the user never made. + QCOMPARE(TagRules::sanitiseId(QStringLiteral("notify-github")), + QStringLiteral("notify-github")); +} + +void TestTagRules::aSanitisedNameThatCollidesGetsItsOwnId() +{ + // Sanitising maps many names onto one id, so it can manufacture the exact + // duplicate that load() drops. "Justeat Orders" and "justeat orders" both + // reduce to justeat-orders; the second must not silently become the first. + const QStringList taken{ QStringLiteral("justeat-orders"), + QStringLiteral("justeat-orders-2") }; + + QCOMPARE(TagRules::uniqueId(QStringLiteral("Justeat Orders"), taken), + QStringLiteral("justeat-orders-3")); + + // No collision means no suffix. + QCOMPARE(TagRules::uniqueId(QStringLiteral("promo"), taken), + QStringLiteral("promo")); + + // A name that sanitises to nothing still has to produce a legal id. + const QString fallback = TagRules::uniqueId(QStringLiteral("!!!"), taken); + QVERIFY(!fallback.isEmpty()); + QVERIFY(TagRules::isValidId(fallback)); +} + +void TestTagRules::savingIsRefusedWhenARuleWouldNotLoadBack() +{ + // The asymmetry that caused the bug: save wrote anything, load validated. + // validate() is the one predicate both sides now use, so a rule that + // would not survive a reload is reported BEFORE it reaches the file. + TagRule good; + good.id = QStringLiteral("good"); + good.query = QStringLiteral("from:a@example.com"); + good.add = { QStringLiteral("x") }; + + TagRule noQuery; + noQuery.id = QStringLiteral("no-query"); + noQuery.add = { QStringLiteral("y") }; + + TagRule noTags; + noTags.id = QStringLiteral("no-tags"); + noTags.query = QStringLiteral("from:b@example.com"); + + TagRule badId; + badId.id = QStringLiteral("Bad Id"); + badId.query = QStringLiteral("from:c@example.com"); + badId.add = { QStringLiteral("z") }; + + QVERIFY(TagRules::validate({ good }).isEmpty()); + + const QStringList problems = + TagRules::validate({ good, noQuery, noTags, badId }); + QCOMPARE(problems.size(), 3); + QVERIFY(problems.join(QChar(' ')).contains(QStringLiteral("no-query"))); + QVERIFY(problems.join(QChar(' ')).contains(QStringLiteral("no-tags"))); + QVERIFY(problems.join(QChar(' ')).contains(QStringLiteral("Bad Id"))); + + // A duplicate id survives a save and is then dropped on load, so it is a + // save-time problem too even though each rule is fine on its own. + TagRule twin = good; + QCOMPARE(TagRules::validate({ good, twin }).size(), 1); +} + +void TestTagRules::aRepairedIdSurvivesASaveAndReload() +{ + // End to end, and the assertion that matters to the user: the rule they + // could not keep is still there after the round trip, with its tags, its + // query and its note intact. + const QString path = writeRules(R"({ + "version": 1, + "rules": [ + {"id": "justeat orders", "add": ["promo"], "stage": 50, + "note": "kept", "query": "from:no-reply@order.example.com"} + ] + })"); + + TagRules loaded; + loaded.load(path); + QCOMPARE(loaded.rules().size(), 1); + QVERIFY(loaded.save(path)); + + TagRules reread; + reread.load(path); + QCOMPARE(reread.rules().size(), 1); + QCOMPARE(reread.rules().first().id, QStringLiteral("justeat-orders")); + QCOMPARE(reread.rules().first().note, QStringLiteral("kept")); + QCOMPARE(reread.rules().first().query, + QStringLiteral("from:no-reply@order.example.com")); + + // Repaired on the way in, so the second read has nothing left to complain + // about. A warning that never clears trains the user to ignore it. + QVERIFY(reread.warnings().isEmpty()); } void TestTagRules::unknownFieldsSurviveASave() @@ -560,6 +729,201 @@ void TestTagRules::leavingTextModeIsRefusedWhenTheQueryCannotBeShownAsRows() "a stale refusal must not outlive the query that caused it"); } +void TestTagRules::theWarningReadsAsAWarningAndSitsBesideSave() +{ + // The label was correct and unread: same font and colour as the intro + // prose two lines above it, so it looked like more explanation. The user + // opened this dialog repeatedly, with the warning showing every time, + // while hunting the rule it was telling them about. + // + // Asserted on the widget's own properties, not on a render. CLAUDE.md + // records why a pixel probe cannot carry this: counting lit pixels cannot + // tell one colour from another reliably, and viewport()->render() returns + // blank often enough that a probe reporting "no red anywhere" says more + // about the probe than the code. + QTemporaryDir configHome; + QVERIFY(configHome.isValid()); + qputenv("XDG_CONFIG_HOME", configHome.path().toUtf8()); + QVERIFY(QDir().mkpath(configHome.filePath(QStringLiteral("mailrules")))); + + const QString stored = configHome.filePath( + QStringLiteral("mailrules/rules.json")); + QFile out(stored); + QVERIFY(out.open(QIODevice::WriteOnly)); + // A rule that warns on load, so the label is populated by opening alone. + out.write(R"({ + "version": 1, + "rules": [ + {"id": "justeat orders", "query": "from:no-reply@order.example.com", + "add": ["promo"], "stage": 50, "enabled": true} + ] + })"); + out.close(); + + TagRulesDialog dialog; + + // The guard. Everything below asserts about a warning that is showing, and + // all of it would pass vacuously against a label that never appears. + QVERIFY2(!dialog.warningTextForTest().isEmpty(), + "a repaired rule must warn, or this test proves nothing"); + + QVERIFY2(dialog.warningTextForTest().contains(QStringLiteral("justeat")), + "the warning must name the rule it is about"); + + const QString style = dialog.warningStyleForTest(); + QVERIFY2(style.contains(QStringLiteral("background-color")), + "a warning that is not filled reads as ordinary prose"); + QVERIFY2(style.contains(QStringLiteral("bold")), "and it must be bold"); + + // Below the rule list, next to the button whose outcome it reports. The + // intro sits at the top, so comparing against it pins the move: this + // assertion fails if the label drifts back under the header. + QVERIFY2(dialog.warningIsBelowTheRuleListForTest(), + "the warning belongs beside Save, not under the intro text"); + + // Plain text, because the strings interpolate ids and queries read from + // the file. A query holding '<' would otherwise be swallowed as markup. + QCOMPARE(dialog.warningTextFormatForTest(), Qt::PlainText); +} + +void TestTagRules::aDismissedWarningComesBackWhenThereIsSomethingNewToSay() +{ + // Dismissal is per-appearance. The warning most often says the file is not + // yet what the hook runs, so a persistent "do not show again" would rehide + // the exact problem that went unnoticed for a session. Closing it clears + // this one; the next thing worth saying shows it again. + QTemporaryDir configHome; + QVERIFY(configHome.isValid()); + qputenv("XDG_CONFIG_HOME", configHome.path().toUtf8()); + QVERIFY(QDir().mkpath(configHome.filePath(QStringLiteral("mailrules")))); + + const QString stored = configHome.filePath( + QStringLiteral("mailrules/rules.json")); + QFile out(stored); + QVERIFY(out.open(QIODevice::WriteOnly)); + out.write(R"({ + "version": 1, + "rules": [ + {"id": "justeat orders", "query": "from:no-reply@order.example.com", + "add": ["promo"], "stage": 50, "enabled": true} + ] + })"); + out.close(); + + TagRulesDialog dialog; + QVERIFY2(!dialog.warningTextForTest().isEmpty(), + "the repaired rule must warn, or the dismissal proves nothing"); + + dialog.dismissWarningForTest(); + QVERIFY2(dialog.warningTextForTest().isEmpty(), + "the X must actually clear the warning"); + + // Something new to say: a rule that cannot be saved. The dismissal must + // not have latched the banner shut. + dialog.setNameForTest(QStringLiteral("second")); + dialog.addRuleForTest(); + dialog.setTagsForTest(QString()); + dialog.setTextModeForTest(true); + dialog.setQueryTextForTest(QString()); + dialog.saveForTest(); + + QVERIFY2(!dialog.warningTextForTest().isEmpty(), + "a refusal after a dismissal must still be shown"); +} + +void TestTagRules::aNameTypedWithSpacesIsSanitisedInTheField() +{ + // The field is labelled "Name", so a person types prose into it. What the + // field SHOWS after the edit is committed is the assertion: sanitising + // silently on save would leave the user looking at a name that is not the + // one being written. + QTemporaryDir configHome; + QVERIFY(configHome.isValid()); + qputenv("XDG_CONFIG_HOME", configHome.path().toUtf8()); + QVERIFY(QDir().mkpath(configHome.filePath(QStringLiteral("mailrules")))); + + const QString stored = configHome.filePath( + QStringLiteral("mailrules/rules.json")); + QFile out(stored); + QVERIFY(out.open(QIODevice::WriteOnly)); + out.write(R"({ + "version": 1, + "rules": [ + {"id": "vendor", "query": "from:vendor.example.org", + "add": ["vendor"], "stage": 50, "enabled": true} + ] + })"); + out.close(); + + TagRulesDialog dialog; + dialog.setNameForTest(QStringLiteral("Justeat orders")); + QCOMPARE(dialog.nameLineForTest(), QStringLiteral("justeat-orders")); + + dialog.saveForTest(); + + TagRules reloaded; + reloaded.load(stored); + QCOMPARE(reloaded.rules().size(), 1); + QCOMPARE(reloaded.rules().first().id, QStringLiteral("justeat-orders")); + QVERIFY2(reloaded.warnings().isEmpty(), + "a rule saved from the dialog must load back without complaint"); +} + +void TestTagRules::aRuleAddedAndNamedInTheDialogSurvivesAReopen() +{ + // The user's session, end to end: add a rule, name it in prose, fill in + // the query and tags, save, reopen. Before the fix the rule was written to + // the file with a space in its id and dropped by every reader, so the + // dialog came back without it and the file kept a rule nothing would run. + QTemporaryDir configHome; + QVERIFY(configHome.isValid()); + qputenv("XDG_CONFIG_HOME", configHome.path().toUtf8()); + QVERIFY(QDir().mkpath(configHome.filePath(QStringLiteral("mailrules")))); + + const QString stored = configHome.filePath( + QStringLiteral("mailrules/rules.json")); + QFile out(stored); + QVERIFY(out.open(QIODevice::WriteOnly)); + out.write(R"({ + "version": 1, + "rules": [ + {"id": "vendor", "query": "from:vendor.example.org", + "add": ["vendor"], "stage": 50, "enabled": true} + ] + })"); + out.close(); + + { + TagRulesDialog dialog; + QCOMPARE(dialog.ruleCountForTest(), 1); + + dialog.addRuleForTest(); + QCOMPARE(dialog.ruleCountForTest(), 2); + + dialog.setNameForTest(QStringLiteral("Justeat orders")); + dialog.setTextModeForTest(true); + dialog.setQueryTextForTest( + QStringLiteral("from:no-reply@order.example.com")); + dialog.setTagsForTest(QStringLiteral("promo, notify/justeat")); + dialog.saveForTest(); + + QVERIFY2(dialog.warningTextForTest().isEmpty(), + "a complete rule must not be refused"); + } + + TagRules reloaded; + reloaded.load(stored); + QCOMPARE(reloaded.rules().size(), 2); + + const TagRule added = reloaded.rules().at(1); + QCOMPARE(added.id, QStringLiteral("justeat-orders")); + QCOMPARE(added.query, + QStringLiteral("from:no-reply@order.example.com")); + QCOMPARE(added.add, (QStringList{ QStringLiteral("promo"), + QStringLiteral("notify/justeat") })); + QVERIFY(reloaded.warnings().isEmpty()); +} + void TestTagRules::aFolderRowUsesTheDropdownAndKeepsItsSuffix() { // A path: without its suffix matches nothing and notmuch says nothing -- cgit v1.2.3