Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions deltachat-jsonrpc/src/api.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1106,9 +1106,6 @@ impl CommandApi {
/// because the word "channel" already appears a lot in the code,
/// which would make it hard to grep for it.
///
/// After creation, the chat contains no recipients and is in _unpromoted_ state;
/// see [`CommandApi::create_group_chat`] for more information on the unpromoted state.
///
/// Returns the created chat's id.
async fn create_broadcast(&self, account_id: u32, chat_name: String) -> Result<u32> {
let ctx = self.get_context(account_id).await?;
Expand Down
3 changes: 0 additions & 3 deletions deltachat-rpc-client/src/deltachat_rpc_client/account.py
Original file line number Diff line number Diff line change
Expand Up @@ -340,9 +340,6 @@ def create_broadcast(self, name: str) -> Chat:
because the word "channel" already appears a lot in the code,
which would make it hard to grep for it.
After creation, the chat contains no recipients and is in _unpromoted_ state;
see `create_group()` for more information on the unpromoted state.
Returns the created chat.
"""
return Chat(self, self._rpc.create_broadcast(self.id, name))
Expand Down
14 changes: 6 additions & 8 deletions src/chat.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1782,9 +1782,7 @@ impl Chat {
);
bail!("Cannot set message, contact for {} not found.", self.id);
}
} else if matches!(self.typ, Chattype::Group | Chattype::OutBroadcast)
&& self.param.get_int(Param::Unpromoted).unwrap_or_default() == 1
{
} else if self.param.get_int(Param::Unpromoted).unwrap_or_default() == 1 {
msg.param.set_int(Param::AttachChatAvatarAndDescription, 1);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Then maybe add ensure_and_debug_assert! here that this is a Group because below group-related params are set?

self.param
.remove(Param::Unpromoted)
Expand Down Expand Up @@ -3626,9 +3624,6 @@ pub(crate) async fn create_group_ex(
/// because the word "channel" already appears a lot in the code,
/// which would make it hard to grep for it.
///
/// After creation, the chat contains no recipients and is in _unpromoted_ state;
/// see [`create_group`] for more information on the unpromoted state.
///
/// Returns the created chat's id.
pub async fn create_broadcast(context: &Context, chat_name: String) -> Result<ChatId> {
let grpid = create_id();
Expand Down Expand Up @@ -3660,17 +3655,20 @@ pub(crate) async fn create_out_broadcast_ex(
|row| row.get(0),
)?;
ensure!(cnt == 0, "{cnt} chats exist with grpid {grpid}");
let mut params: Params = Params::new();
params.update_timestamp(Param::GroupNameTimestamp, time())?;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

It's not clear why this is needed for broadcasts, but not for groups. Why doesn't .set_i64(Param::GroupNameTimestamp, msg.timestamp_sort) in prepare_msg_raw() work, because prepare_msg_raw() isn't called when adding a new subscriber?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Because of && self.param.get_int(Param::Unpromoted).unwrap_or_default() == 1. Groups are unpromoted right after creating them, broadcast channels are immediately promoted.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Then the check in prepare_msg_raw() should be fixed, checking for Chattype::OutBroadcast is not necessary and confusing. And the documentation of create_broadcast() says:

/// After creation, the chat contains no recipients and is in _unpromoted_ state;

Should be fixed as well.

But the reason why for groups Param::GroupNameTimestamp is set when they get promoted is to not reveal the group creation time, as far as i remember. For broadcasts it also may make sense. Would be good to have the same logic for groups and broadcasts, maybe just set the timestamp to 1 on chat creation?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

And the documentation of create_broadcast() says:

/// After creation, the chat contains no recipients and is in _unpromoted_ state;

Should be fixed as well.

Good catch!

Then the check in prepare_msg_raw() should be fixed, checking for Chattype::OutBroadcast is not necessary and confusing.

Actually there is no need at all to check for the chat type there, I will just remove that check.

For broadcasts it also may make sense. Would be good to have the same logic for groups and broadcasts, maybe just set the timestamp to 1 on chat creation?

The logic is already different, and in separate functions, but either way of fixing this would be fine (it's a very small bug in the first place).


t.execute(
"INSERT INTO chats
(type, name, name_normalized, grpid, created_timestamp)
VALUES(?, ?, ?, ?, ?)",
(type, name, name_normalized, grpid, created_timestamp, param)
VALUES(?, ?, ?, ?, ?, ?)",
(
Chattype::OutBroadcast,
&chat_name,
normalize_text(&chat_name),
&grpid,
timestamp,
params.to_string(),
),
)?;
let chat_id = ChatId::new(t.last_insert_rowid().try_into()?);
Expand Down
21 changes: 18 additions & 3 deletions src/chat/chat_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ use crate::headerdef::HeaderDef;
use crate::imex::{ImexMode, has_backup, imex};
use crate::message::{Message, MessengerMessage, delete_msgs};
use crate::mimeparser::{self, MimeMessage};
use crate::qr::{Qr, check_qr};
use crate::receive_imf::receive_imf;
use crate::securejoin::{get_securejoin_qr, join_securejoin};
use crate::test_utils;
Expand Down Expand Up @@ -2922,10 +2923,24 @@ async fn test_broadcast_change_name() -> Result<()> {
let fiona = &tcm.fiona().await;

let broadcast_id = create_broadcast(alice, "Channel".to_string()).await?;
let qr = get_securejoin_qr(alice, Some(broadcast_id)).await.unwrap();
let mut qr = get_securejoin_qr(alice, Some(broadcast_id)).await.unwrap();
// Something goes wrong with the title, e.g. maybe it gets ellipsized
// Note that the title always comes at the end for human readability
qr += "+modified+title";

{
tcm.section("Alice invites Bob to her channel");
let Qr::AskJoinBroadcast { name, .. } = check_qr(bob, &qr).await? else {
panic!();
};
assert_eq!(name, "Channel modified title");

// The channel's name gets fixed after actually joining the channel:
let bob_chat_id = tcm.exec_securejoin_qr(bob, alice, &qr).await;
let bob_chat = Chat::load_from_db(bob, bob_chat_id).await?;
assert_eq!(bob_chat.name, "Channel");
}

tcm.section("Alice invites Bob to her channel");
tcm.exec_securejoin_qr(bob, alice, &qr).await;
tcm.section("Alice invites Fiona to her channel");
tcm.exec_securejoin_qr(fiona, alice, &qr).await;

Expand Down