fix: Hide synced chat if we only know its visibility

[j-g00da]

Prevents showing "empty" chat on other devices,
when pinning a chat that did not complete securejoin yet.

Fixes: #7713

[j-g00da]

Tested with deltachat-desktop.

Maybe we should default to setting Blocked::Yes when creating new chats in sync_alter_chat, but there may be side effects hard to predict. That's why I only changed it in this single specific case (SyncId::ContactFingerprint with SyncAction::SetVisibility).

[j-g00da]

Difference (unrelated to the bug itself) from UX perspective:

Normally chat won't display on other devices until you send a first message or pin it (then it will display as a contact request, which IMO is confusing anyway, as we initiated the chat, not the other way around).

After this change the chat will be displayed on other devices only after first message is sent.

[Hocuri]

Maybe we should default to setting Blocked::Yes when creating new chats in sync_alter_chat, but there may be side effects hard to predict. That's why I only changed it in this single specific case (SyncId::ContactFingerprint with SyncAction::SetVisibility).

Do any tests fail when setting the default to Blocked::Yes?

Going through the SyncActions:

    Block,
    Unblock,
    Accept, // These three anyways set the blocked state of the chat, so that it doesn't matter what it was before

    SetVisibility(ChatVisibility),

    SetMuted(MuteDuration), // This one probably had the same bug as SetVisibility - muting a chat with an unfinished securejoin process probably created a weird chat on the second device

    CreateOutBroadcast {
        chat_name: String,
        secret: String,
    },
    CreateGroupEncrypted(String), // These two are anyways handled by a different part of the match statement

    Rename(String), // The function returns before `get_for_contact()` is called

    SetContacts(Vec<String>),
    SetPgpContacts(Vec<(String, String)>),
    SetDescription(String),  // These three are about groups and channels, not about 1:1 chats

    Delete, // The chat will be deleted, no need to worry about it

Not sure if all of my comments are understandable, but the result is that I think it's fine to change to Blocked::Yes unconditionally.

[Hocuri]

Also, could you add a test? You can look at test_blocked_bob_cant_create_11_chat_via_securejoin for how to test syncing and securejoin. The exec_securejoin_qr_multi_device() function (and some other functions in test_utils.rs) run the full securejoin process; in order to control which device receives which message, you can also pop the messages one by one, just like in test_deduplicate_member_added().

[j-g00da]

Reading the comment few lines higher:

                // Use `Request` so that even if the program crashes, the user doesn't have to look
                // into the blocked contacts.

This is a bit odd though and it feels like we should be safe setting it to just Blocked::Yes? (Also it's setting Blocked for a chat, not a contact, which is a different concept) cc: @iequidoo

[iequidoo]

This is a bit odd though and it feels like we should be safe setting it to just Blocked::Yes? (Also it's setting Blocked for a chat, not a contact, which is a different concept) cc: @iequidoo

The comment is indeed wrong, but Blocked::Yes would also look strange and i made an experiment, this:

                let chat_id = ChatIdBlocked::get_for_contact(self, contact_id, Blocked::Yes)
                    .await?
                    .id;
                ensure!(false);
                chat_id

creates a hidden chat on the other device. Maybe just remove the comment and leave Request so that the chat is at least visible if some error happens.

[Hocuri]

LGTM, thanks for fixing! Just some minor, and not too important, comments.