Skip to content

Generate rfc724_mid when creating Message #6704

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

Septias
Copy link
Collaborator

@Septias Septias commented Mar 25, 2025

Set rfc724_mid in Message::new(), Message::new_text(), and Message::default() instead of when sending the message. This way the rfc724 mid can be read in the draft stage which makes it more consistent for bots. Tests had to be adjusted to create multiple messages to get unique mid, otherwise core would not send the messages out.

close #6621

@Septias Septias marked this pull request as ready for review March 25, 2025 17:55
@Septias Septias requested review from link2xt, iequidoo and Hocuri and removed request for link2xt March 26, 2025 12:40
Copy link
Collaborator

@Hocuri Hocuri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR introduces a new bug: When I send any webxdc in DC Android, it gets sent twice, i.e. the webxdc is duplicated. Edit: This bug is fixed now

Edit: BTW, would be nice to add a test in this PR

@Septias Septias force-pushed the sk/creat_mid_uprfront branch from 82877cf to c5658fb Compare March 27, 2025 17:51
Copy link
Collaborator

@iequidoo iequidoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, though i couldn't understand which test reproduces the reported bug

@Septias
Copy link
Collaborator Author

Septias commented Mar 27, 2025

@Hocuri built manually and tested on android

@iequidoo
Copy link
Collaborator

@Hocuri built manually and tested on android

It seems that it's not difficult to add a test that just compares the get_webxdc_self_addr() result for a draft and after sending. Or maybe even not a webxdc test, but some test checking that rfc724_mid doesn't change after sending

@Hocuri
Copy link
Collaborator

Hocuri commented Mar 27, 2025

Hm, right now, when I test it on my Android (draft poll webxdc -> create poll -> vote -> send), the message is only sent once, but it doesn't contain the vote (i.e. the behavior is the same as before this PR).

About the test, it's just good to have a regression test so that we don't introduce the same bug again in a future refactoring.

@Septias
Copy link
Collaborator Author

Septias commented Mar 28, 2025

By bug do you mean double sending messages or not setting the mid upfront? Because that is rather a feature not a bug

@Septias Septias force-pushed the sk/creat_mid_uprfront branch from a59bab7 to 351d7e6 Compare March 28, 2025 09:29
@Hocuri
Copy link
Collaborator

Hocuri commented Mar 28, 2025

If possible, it would be best if the test tests both that only one message is sent and that the rfc724_mid is set upfront. But I'd say it's not strictly necessary.

I tested again, this PR certainly doesn't fix the bug on Android - the behavior is the same as without the PR, the selfAddr in a drafted message is always 91f25.... Might be a DC Android bug, of course.

@Septias
Copy link
Collaborator Author

Septias commented Mar 31, 2025

Idk if it is only a display error because the new test successfully checks that the sending side only has one associated message in the chat. Maybe a display bug on android?

@Septias Septias force-pushed the sk/creat_mid_uprfront branch from 6b97991 to 564a569 Compare March 31, 2025 10:16
Copy link
Collaborator

@iequidoo iequidoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, but i didn't check Android. Desktop seems to work w/o regressions (checked using the poll app)

Copy link
Collaborator

@iequidoo iequidoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For some reason when i put this PR on top of 3efd94914 chore(release): prepare for 1.158.0, it breaks forwarding messages in Desktop -- messages are forwarded twice. But i need to wait some time until the second message arrives.

r10s
r10s previously requested changes Apr 1, 2025
Copy link
Contributor

@r10s r10s left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i did not follow closely, however, we should not merge this pr until we know the forwarding issue is clearly unrelated

and/or desktop is fixed

otherwise we upgrade to a worse bug :)

@Hocuri
Copy link
Collaborator

Hocuri commented Apr 2, 2025

@iequidoo I can confirm that the same bug exists on DC Android.

@Septias Septias force-pushed the sk/creat_mid_uprfront branch from 564a569 to afc625f Compare April 13, 2025 09:17
@iequidoo
Copy link
Collaborator

For some reason when i put this PR on top of 3efd94914 chore(release): prepare for 1.158.0, it breaks forwarding messages in Desktop -- messages are forwarded twice. But i need to wait some time until the second message arrives.

I checked the new PR version putting it on top of 1.159.1, looks working now. Will try to dismiss @r10s's "requested change" then

@iequidoo iequidoo dismissed r10s’s stale review April 14, 2025 05:47

The PR works for me now

@iequidoo iequidoo requested a review from Hocuri April 14, 2025 05:57
@r10s
Copy link
Contributor

r10s commented Apr 14, 2025

did someone actually try the PR at least on android and desktop, including doing things as stage drafts with images, files, sticker etc.?

so that we do not break the workflow of sending regular messages by fixing staging webxdc :)

@Septias
Copy link
Collaborator Author

Septias commented Apr 14, 2025

I'm still fixing the problem that test webxd does show the same self-addr for all webxdcs even though they should differ because of the 724mids. While doing it I can also verify that everything else works as expected.
Edit: It seems like the forwarded msg get different msgid on restart, but consecutive forwards then produce the same mid xD
Edit2: This problem seems to exist on main too, so I filed an issue: deltachat/deltachat-desktop#4993

create mid in default

remove comments
@Septias Septias force-pushed the sk/creat_mid_uprfront branch from 5fa9643 to 3833322 Compare April 14, 2025 11:01
@Septias
Copy link
Collaborator Author

Septias commented Apr 14, 2025

Sadly I can't build for android and desktop does not let you open the webxdc while in drafting stage so I can not test the workflow manually. But the test verifies that selfAddr is the same for draft and sent message so for bots (which the issue is about) it should be fine. @Hocuri can you test this on android?

@Hocuri
Copy link
Collaborator

Hocuri commented Apr 14, 2025

I tested this PR on Android. As far as I can see, this PR doesn't change the behavior of DC Android; all drafted webxdc's still have the same selfAddr (91f...), and participating in a poll while it is still in draft mode doesn't work. But this PR also doesn't introduce a regression anymore, so, if this PR fixes things for bots then it would be fine to merge it.

@link2xt
Copy link
Collaborator

link2xt commented Apr 14, 2025

We should not close #6621 however if the bug is not fixed.

@iequidoo
Copy link
Collaborator

iequidoo commented Apr 14, 2025

Now, if i revert fix: no double send, no Rust tests fail. I think it's important enough to be covered, but may be done in a follow-up PR as it's not the original bug

@iequidoo
Copy link
Collaborator

This tests fix: no double send, so i suggest to add it:

--- a/src/chat/chat_tests.rs
+++ b/src/chat/chat_tests.rs
@@ -2020,8 +2020,10 @@ async fn test_forward_basic() -> Result<()> {
     forward_msgs(&bob, &[msg.id], bob_chat.get_id()).await?;
 
     let forwarded_msg = bob.pop_sent_msg().await;
+    let msg_bob = Message::load_from_db(&bob, forwarded_msg.sender_msg_id).await?;
     assert_eq!(bob_chat.id.get_msg_cnt(&bob).await?, 2);
     let msg = alice.recv_msg(&forwarded_msg).await;
+    assert_eq!(msg.rfc724_mid(), msg_bob.rfc724_mid());
     assert_eq!(msg.get_text(), "Hi Bob");
     assert!(msg.is_forwarded());
     Ok(())

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

webxdc.selfAddr not consistent for draft messages vs sent message
5 participants