Conversation
| ) | ||
| } | ||
|
|
||
| pub(crate) async fn mdn( |
There was a problem hiding this comment.
Main change is the addition of this function that builds QueuedMail directly without ever creating MimeFactory structure. And all code that uses MimeFactory structure does not have to consider MDN case.
cedfd0d to
91245c4
Compare
c282e62 to
cc74979
Compare
| additional_msg_ids: Vec<String>, | ||
| ) -> Result<MimeFactory> { | ||
| let contact = Contact::get_by_id(context, from_id).await?; | ||
| let from_addr = context.get_primary_self_addr().await?; |
There was a problem hiding this comment.
This get_primary_self_addr() result was not used anywhere, but now it is not even called.
2cbb986 to
9e6a360
Compare
link2xt
changed the title
9e6a360 to
2af2b9a
Compare
Hocuri
left a comment
Hocuri
left a comment
There was a problem hiding this comment.
Partial review.
Looks good overall; I don't think this should go into a potential hotfix-release, but ofc we can merge it and then potentially backport a fix if we need one.
| } else { | ||
| recipients = vec![addr.clone()]; | ||
| QueuedEncryption::No | ||
| }; |
There was a problem hiding this comment.
I thought we don't send unencrypted MDNs anymore? In either case, I don't think we want them.
I would actually be surprised if they're still working at this point; fn mdn seems to forget to add the To and Date headers to unencrypted MDNs (I didn't check whether this problem was there before this PR). So, let's just remove them.
| let bcc_self = context.get_config_bool(Config::BccSelf).await?; | ||
|
|
||
| let date = chrono::DateTime::<chrono::Utc>::from_timestamp(timestamp, 0) | ||
| .unwrap() |
There was a problem hiding this comment.
Wouldn't it be better to use ? here rather than unwrap()?
I'm surprised that we don't have #![cfg_attr(not(test), forbid(clippy::unwrap_used))], is there a reason for it or did we just overlook it?
There was a problem hiding this comment.
Seems the last commit mentioning them is 0725fe3 and we probably did not figure out cfg_attr or maybe it did not even exist back then.
There was a problem hiding this comment.
Actually, turns out that we're using unwrap() in not so few places, I'm getting 24 errors when I enable the lint. Maybe it's fine not to enable the lint, I can't remember any problems with panics recently.
…ry::mdn() This factors out all MDN-related code that was previously inside `MimeFactory` into a `mimefactory::mdn` function that constructs `QueuedMail` directly, similarly to `mimefactory::keyupdate_message`. Removing MDN-related code from MimeFactory also means that MimeFactory only handles non-MDN messages now and we don't have to `match` on `MimeFactory.loaded` everywhere to access `msg` and `chat`. Unlike `MimeFactory::from_mdn`, `mimefactory::mdn` does not need to fill `MimeFactory.from_addr`. This removes one `get_primary_self_addr()` call. `mimefactory::mdn` also does not need to fill various fields that are never used for MDNs such as `past_members`.
434c32a to
ce94c0c
Compare
This branch has not been deployed
2 participants
This factors out all MDN-related code that was previously inside
MimeFactoryinto a
mimefactory::mdnfunction that constructsQueuedMaildirectly,similarly to
mimefactory::keyupdate_message.Removing MDN-related code from MimeFactory also means that MimeFactory only handles non-MDN messages now
and we don't have to
matchonMimeFactory.loadedeverywhere to accessmsgandchat.Unlike
MimeFactory::from_mdn,mimefactory::mdndoes not need to fillMimeFactory.from_addr.This removes one
get_primary_self_addr()call.mimefactory::mdnalso does not need to fillvarious fields that are never used for MDNs such as
past_members.Make sure to enable "hide whitespace" when reviewing on GitHub, there is some code that was inside
matchorif letforLoaded::Messagethat stays the same with just the branching around removed.