Skip to content
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

fix: only show draft in message info if self is in chat #6234

Closed
wants to merge 3 commits into from

Conversation

Septias
Copy link
Collaborator

@Septias Septias commented Nov 20, 2024

This PR adds a check to get_info so that draft is only added if self is part of the group. It also implements a check that stops setting a draft if self is not part of the chat. The function does not fail but only emit an error in this case.

close #6229

This PR adds a check to `get_info` so that `draft` is only added if self
is part of the group determined by `can_send`.

close #6229
src/chat.rs Outdated
@@ -1839,7 +1840,7 @@ impl Chat {
/// deltachat, and the data returned is still subject to change.
pub async fn get_info(&self, context: &Context) -> Result<ChatInfo> {
let draft = match self.id.get_draft(context).await? {
Some(message) => message.text,
Some(message) if self.can_send(context).await? => message.text,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

With this line you can not set a draft while we are still waiting for securejoin and if chat is still in contact request phase. Also you can not set it if protection status is broken. Maybe this is too restrictive and we should only check is_contact_in_chat here aswell.

@Septias Septias marked this pull request as draft November 20, 2024 11:58
@Septias
Copy link
Collaborator Author

Septias commented Nov 22, 2024

Maybe disabling draft setting when one is not in a chat is a bit much and should rather be enforced by UI, I don't know how they use our internal API, and disabling it might be too restrictive. That's why we return Ok() but emit a warning and not more.

let draft = match self.id.get_draft(context).await? {
Some(message) => message.text,
Some(message) if self_in_chat => message.text,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

if we don't check that drafts are only set when one is part of the chat, the PR reduces to this line only and its test

@Septias Septias marked this pull request as ready for review November 23, 2024 09:21
Copy link
Collaborator

@link2xt link2xt left a comment

Choose a reason for hiding this comment

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

Original issue is about the chatlist summary, but in the PR "summary" appears only in the test name and it seems nothing is changed to hide the draft from the summary.

.sql
.query_get_value("SELECT type FROM chats WHERE id=?;", (self,))
.await
.map(|res| res.ok_or(anyhow!("Can't get type for char")))
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should use .context(), otherwise you lose underlying error.
Something like .await.context("Failed to get chat type")?.context("Chat does not exist")

Copy link
Collaborator

Choose a reason for hiding this comment

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

typo: s/char/chat/

async fn get_chat_type(self, context: &Context) -> Result<Chattype> {
context
.sql
.query_get_value("SELECT type FROM chats WHERE id=?;", (self,))
Copy link
Collaborator

Choose a reason for hiding this comment

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

; is not needed

@@ -1838,8 +1862,9 @@ impl Chat {
/// This is somewhat experimental, even more so than the rest of
/// deltachat, and the data returned is still subject to change.
pub async fn get_info(&self, context: &Context) -> Result<ChatInfo> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this function called by any UI? Seems it has been added in #1068
If it is not used after 5 years of being experimental, looks like it should be deprecated.
Chatlist::get_summary that is actually used by the UI does not seem to call it.

@Hocuri
Copy link
Collaborator

Hocuri commented Nov 23, 2024

This very small bug created a lot of discussions without any fix (which I wouldn't have expected when I brought it up), so, doing some issue gardening 🌱, let's move it to project resurrection.

@Hocuri Hocuri closed this Nov 23, 2024
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.

Hide the draft from summary if cannot send to chat
3 participants