fix: emit ChatModified event when can_send changes during secure join
#7735
Conversation
join Also adds a test for the event (for securejoin group and secure join contact). Closes #7634
| } | ||
|
|
||
| #[tokio::test(flavor = "multi_thread", worker_threads = 2)] | ||
| async fn test_secure_join_contact_can_send_and_event() -> Result<()> { |
There was a problem hiding this comment.
There's already test_setup_contact_ex(), maybe add new checks there so that there's less code to maintain? It's not a new scenario anyway. And for SecureJoin to a group there's test_secure_join(). Or does awaiting for ChatModified somehow intersect with SecurejoinJoinerProgress so it's complicated to test both events?
There was a problem hiding this comment.
The used way of awaiting events drops all other events that are emitted. So when order of events is stable it could work to use the existing method. I thought a new test, which only tests for the event is cleaner and has less risk of breaking an existing test.
Also in this particular test there is this line
core/src/securejoin/securejoin_tests.rs
Line 139 in 659d21a
which already seemed to change can_send, because it already ingests alice's key.
maybe add new checks there so that there's less code to maintain?
Thought about it, but since that test is already hard to understand for me, I did not want to make it even more complex and more importantly I don't want to accidentally break an important test by editing it to test something less important.
There was a problem hiding this comment.
Overall i agree with adding new test code to not intersect with checks for EventType::SecurejoinInviterProgress.
which already seemed to change can_send, because it already ingests alice's key.
parse_msg() shouldn't change bob's context state this or another significant way because it's used in the tests. If it does, this should be fixed in a separate PR
| .await; | ||
|
|
||
| let bob_chat = bob.get_chat(&alice).await; | ||
| assert_eq!(bob_chat.can_send(&bob).await?, true); |
There was a problem hiding this comment.
NB: Bob already can send, but if vc-request-with-auth won't be delivered or reordered, Bob's message must be accepted and shown to Alice, but Bob shouldn't be verified yet. We need a separate test/PR for this, but it's better to be done as a separate SetupContactCase variant (testing for ChatModified isn't necessary in this case). Otherwise there's a risk that Alice just drops Bob's message to trash, this shalln't happen if we already allow to send messages.
|
|
||
| let sent = alice.pop_sent_msg().await; | ||
| let bob_chat = bob.get_chat(&alice).await; | ||
| assert_eq!(bob_chat.can_send(&bob).await?, false); |
There was a problem hiding this comment.
Why checking again here? Bob hasn't received any message from Alice since the previous check. I think that the previous check should just be moved here, it's better because it also checks the reason
| } | ||
|
|
||
| #[tokio::test(flavor = "multi_thread", worker_threads = 2)] | ||
| async fn test_secure_join_contact_can_send_and_event() -> Result<()> { |
There was a problem hiding this comment.
Overall i agree with adding new test code to not intersect with checks for EventType::SecurejoinInviterProgress.
which already seemed to change can_send, because it already ingests alice's key.
parse_msg() shouldn't change bob's context state this or another significant way because it's used in the tests. If it does, this should be fixed in a separate PR
| assert_eq!( | ||
| Chat::load_from_db(&bob, bob_chatid) | ||
| .await? | ||
| .can_send(&bob) |
There was a problem hiding this comment.
Better use why_cant_send() to check the reason
Also adds a test for the event (for securejoin group and secure join
contact).
Closes #7634