fix: Make pre-messages w/o text want MDNs (#8004)

[iequidoo]

This fixes #8004 . Maybe it's even good that MDNs will
be sent for pre-messages only having placeholder text with the viewtype and size, at least this way
we notify the contact that we've noticed the message. Anyway, with adding message previews the
problem will be solved for the corresponding viewtypes.

[iequidoo]

I made a bug in the test -- bob doesn't accept the chat -- and this looks suspicious to me:

alice WARN: src/mimeparser.rs:435: decryption failed: decrypt_with_keys: missing key
alice INFO: src/receive_imf.rs:520: Receiving message "ffc78f9e-5ca0-4ab0-8379-8b9150fedfac@localhost", seen=false...
alice INFO: src/contact.rs:1094: Added contact id=1002 addr=bob@example.net.
alice INFO: src/receive_imf.rs:1442: Non-group message, no parent. num_recipients=1. from_id=Contact#1002. Chat assignment = OneOneChat.
alice INFO: src/receive_imf.rs:1771: No chat id for message (TRASH).
alice INFO: src/receive_imf.rs:2356: Message has 1 parts and is assigned to chat #Chat#Trash.
test tests::pre_messages::receiving::test_pre_msg_mdn ... FAILED

Why is bob's self-MDN being processed after the decryption failure? It should go to the trash just because of that and no message should be tried to be added.

[link2xt]

Can we instead have a new function shift_sent_msg_opt that does this and not a global config that is only used for tests? Otherwise we will have to add this config to all tests over time.

[iequidoo]

Otherwise we will have to add this config to all tests over time.

Why? Only to those where sending messages from head is needed to reproduce some regression. Anyway, do you mean adding a function that moves an smtp job from head to tail?

[link2xt]

I mean creating a function shift_sent_msg_opt that takes message from the beginning of the queue rather than the end, so we can use it in new tests.

[iequidoo]

The first i wanted to do is to create unqueue_sent_msg_opt() which does the same as pop_sent_msg_opt(), but takes the first queued message. But there are other functions calling pop_sent_msg_opt(), so we should forward some flag everywhere. E.g. in the new test it's send_msg() returning SentMessage. Note sure you mean the same, what does shift_ mean? Anyway, i'd avoid adding a flag everywhere, better we add a new Config key or some flag (maybe one-shot) to Context.

I also don't expect many tests needing this, so a config key doesn't look like a problem.

[iequidoo]

Also it appeared that not the sending order is a problem, but absence of message text, so i can drop this part at all. But as it's already done, maybe let it remain?

EDIT: The discussion here is a bit outdated because pop_sent_msg_ex() was added meanwhile, but we still need a way to revert the order for messages popped from the SMTP queue indirectly.

[iequidoo]

Removed this Config::PopSentMsgFromHead, chat::send_msg() can be used in tests directly, which doesn't pop any message from smtp, then sent messages can be popped by calling TestContext::pop_sent_msg_ex() which pops from head by default.

[link2xt]

I really don't like conditional compilation and generally changing the code just for a single test, like adding hooks, options and code paths that are only needed to make the tests pass. If every test adds this, we will end up with unmaintainable code because every time a change to such function is needed someone will have to make sure that the code added for the test does not break as well.

If the function is too large to be usable in a test, e.g. because it makes SMTP connections, it can be refactored first into smaller functions to make it possible to call relevant parts from the test.

[link2xt]

For a similar discussion, previously: #8019
Also we have pre_encrypt_mime_hook used only in a single test: #8271

[iequidoo]

I added a commit removing conditional compilation by passing different callbacks for MDN sending from the release and test code. Not sure this resolves your comment however, but the idea isn't that every test adds a new conditional compilation block or a new callback, but all tests pass the same callback to send_mdn() so that we only have two: the real and the test ("mock") ones. We anyway cannot test in Rust the same code that we really run.

At least now, with callbacks instead of conditional compilation, code isolation is better, e.g. the test code can't be affected by local vars.

[iequidoo]

One more way to refactor this is to add another set of smtp functions (namely, send_mdn_rfc724_mid() and send_mdn()) for tests and extract common code blocks into separate functions shared with the release code, but i afraid this will worsen readability and it'll be even difficult to name those common functions meaningfully.

[iequidoo]

Have made send_fn impl AsyncFnOnce and removed extra parameters from it, they are now bound to closures.

@link2xt is this callback approach enough? I'm not sure that breaking everything into smaller functions will improve maintainability

[link2xt]

To avoid complicating send_mdn_rfc724_mid with hooks and test code: it is enough to check that the message WantsMdn and smtp_mdns has an entry, as already checked above.

It indeed looks bad that send_mdn_rfc724_mid does SMTP sending itself rather than just queueing a message and interrupting SMTP loop and probably should be refactored, but it's better not to do any refactoring of this function in this PR.

[iequidoo]

Is it actually that bad (less readable?) that send_mdn_rfc724_mid() now accepts send_fn instead of smtp: &mut Smtp? It's not a test hook like we had before and have already removed. Rather a way to abstract parts of the code.

In this test it's ok to just check for presence of the smtp_mdns entry, fortunately. But what to do in tests actually needing to receive MDNs we send, like e.g. 67a75a4? If we never refactor this MDN sending pipeline, the only way to test receiving MDNs is to put them into test-data/ which is really harder to maintain. Testing receiving Detlta Chat -generated MDNs in Python tests isn't always helpful because of timings. Sometimes an MDN needs to be received at an exact moment.

...

It indeed looks bad that send_mdn_rfc724_mid does SMTP sending itself rather than just queueing a message and interrupting SMTP loop and probably should be refactored, but it's better not to do any refactoring of this function in this PR.

Agree, that would be a quite big refactoring for this PR. In the current state the PR only moves a part of the code into AsyncFnOnce(&str, String, Vec<String>) -> Result<bool>, this is all the refactoring.

[iequidoo]

I've removed the refactoring of smtp and corresponding parts of the tests. It's unfortunate that MDNs can't be fully tested w/o it. Here's a branch for the removed refactoring, but i'm not going to create more discussions or PRs: https://github.com/chatmail/core/tree/iequidoo/queue_mdn

[link2xt]

I am not sure I understand this comment. "we can't tell which one was seen" means that we cannot tell if pre-message or fully downloaded message was seen?

Also not clear which part of receive_imf "see receive_imf" refers to.

[iequidoo]

"we can't tell which one was seen" means that we cannot tell if pre-message or fully downloaded message was seen?

Yes. I've already forgotten which part of receive_imf i meant and why, but i tried to improve the comment.

[link2xt]

I've removed the refactoring of smtp and corresponding parts of the tests. It's unfortunate that MDNs can't be fully tested w/o it. Here's a branch for the removed refactoring, but i'm not going to create more discussions or PRs: https://github.com/chatmail/core/tree/iequidoo/queue_mdn

I have looked at it, but still think we don't want to have code that converts smtp_mdns into smtp just for tests. The code related to smtp table handling will need to be changed for failover as we will need to store the messages differently to make it possible to update the From header. I'm currently looking into possibility of updating the Date header for #8112 as it will also make it possible to do failover and try sending the same message from different relays. Having more code depending on smtp will make it more difficult and storing MDNs in smtp table is not what happens in the actually running code.

[iequidoo]

I have looked at it, but still think we don't want to have code that converts smtp_mdns into smtp just for tests. [...]

Got it. Maybe then it should be not queue_mdn() but get_sent_mdn() -> SentMessage<'_> in test_utils module? However, this can be done later after #8112 and implementing SMTP failover.