diff options
| -rw-r--r-- | CLAUDE.md | 13 | ||||
| -rw-r--r-- | docs/superpowers/plans/2026-08-03-post-0.1.0-usability-closed.md | 163 | ||||
| -rw-r--r-- | docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md | 116 | ||||
| -rw-r--r-- | src/tagrules.cpp | 108 | ||||
| -rw-r--r-- | src/tagrules.h | 25 | ||||
| -rw-r--r-- | src/tagrulesdialog.cpp | 184 | ||||
| -rw-r--r-- | src/tagrulesdialog.h | 29 | ||||
| -rw-r--r-- | tests/test_tagrules.cpp | 376 |
8 files changed, 877 insertions, 137 deletions
@@ -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<TagRule> &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<TagRule> &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<QVBoxLayout *>(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 |