fix: append a copy of every sent message to the sent folder - #455
Merged
Merged
Conversation
Signed-off-by: Arne K. (TRC-Loop) <me@arne.sh>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #451.
Sent mail was delivered and no copy was ever kept. Not on the server, not locally. The report traced it exactly right: the append step is wired through
psmtp.WithSentAppender, and the one place that builds the sender never passed it, soappendSentwas nil on every send and the step returned immediately.Transmitthen returned nil, the outbox marked the message sent, and the compose window said it had worked.The only trace was a
Debugline, and file logging is off by default, so a total loss of sent copies looked exactly like normal operation.Confirmed before changing anything:
WithSentAppenderhad one caller in the whole repository,cmd/smtptest, a debug tool. Nothing ininternal/desktopever called it.The fix
accountTransmitter.Transmitnow passes an appender that opens an imap session for the account and callsAppendToSent, whichinternal/imap/append.goalready implemented correctly, special-use\\Sentlookup and name fallbacks included. It was simply never reached.AppendToSentjoins themailClientseam, which is why it was awkward to wire before: there was no way to get a client for an account through the interface the bindings use, and going around it would have put the send path outside the seam every other binding is tested through.Three decisions worth stating:
syncMu. Every other imap session in the package does, so a send during a sync waits its turn instead of opening a competing login.Tests
Two, driving the real transmit path against an in-process submission server, with the account pinning the server's certificate through the trust added in #453.
Testing the helper alone would have been worthless here: the helper was fine, the wiring was missing, and a helper test would have passed for the entire time the feature was dead. So the assertion is on
Transmitas the outbox worker calls it. I checked it fails for the right reason by reverting the one-line wiring:appended 0 messages to Sent, want 1.The second test covers the case where the append fails: the message has already been delivered, so reporting failure would have the outbox retry and send it twice. It stays a warning.
Not done here
The report also asks for a visible signal when the copy cannot be written. It is a
Warntoday and nothing surfaces in the interface. That is a real gap and worth its own issue, but it is a question about how to show it rather than part of this fix.