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 group renaming #6699

Merged
merged 3 commits into from
Mar 29, 2025
Merged

Fix group renaming #6699

merged 3 commits into from
Mar 29, 2025

Conversation

link2xt
Copy link
Collaborator

@link2xt link2xt commented Mar 22, 2025

Fixes #6697

Based on #6711

@link2xt link2xt requested review from iequidoo and Hocuri March 22, 2025 15:31
Base automatically changed from link2xt/python-test-vcard to main March 23, 2025 15:45
@link2xt link2xt force-pushed the link2xt/fix-group-renaming branch from 2f0ac1d to 704729f Compare March 23, 2025 15:59
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.

I don't really understand how the fix fixes the test, since the test doesn't even have multiple devices that would send sync messages. Maybe the comment could be improved to explain this better, maybe it's fine.

src/chat.rs Outdated
Comment on lines 4218 to 4220
// If the change is received from the sync message,
// do not update timestamp. It may be long after
// the group is promoted.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it would be good to explain this more extensively, because it took me quite a long time to understand it. E.g.:

            // If the change is received from sync message
            // do not update timestamp. It may be long after
            // the group is promoted.
            // (note that `sync == Nosync` would mean
            //that the change comes from a sync message)

@link2xt
Copy link
Collaborator Author

link2xt commented Mar 24, 2025

I don't really understand how the fix fixes the test, since the test doesn't even have multiple devices that would send sync messages.

The bug is not about sync messages. Alice is sending old timestamp so the group name is not updated for Bob.

@iequidoo
Copy link
Collaborator

The reason of the bug is that MimeFactory::from_msg() loads the chat and then the old (not updated) timestamp is used, so i think the right fix should be just moving this block:

    if msg.param.get_cmd() == SystemMessage::GroupNameChanged {
        msg.chat_id
            .update_timestamp(context, Param::GroupNameTimestamp, msg.timestamp_sort)
            .await?;
    }

above. Using time() may lead to different timestamps in the Date and Chat-Group-Name-Timestamp headers which doesn't look good. And the time() return value isn't aimed for sending out, it's not smeared timestamp.

@link2xt link2xt force-pushed the link2xt/fix-group-renaming branch from 704729f to b4217fc Compare March 28, 2025 22:37
@link2xt link2xt changed the base branch from main to link2xt/test-vcard-contacts March 28, 2025 22:37
@iequidoo
Copy link
Collaborator

Still, why don't just move the block updating GroupNameTimestamp above? This way we save one db query and also that block goes after

    if recipients.is_empty() {
        // may happen eg. for groups with only SELF and bcc_self disabled                                                                                                                                            
        info!(
            context,
            "Message {} has no recipient, skipping smtp-send.", msg.id
        );
        msg.param.set_int(Param::GuaranteeE2ee, 1);
        msg.update_param(context).await?;
        msg.id.set_delivered(context).await?;
        msg.state = MessageState::OutDelivered;
        return Ok(Vec::new());
    }

which doesn't look good becuse GroupNameTimestamp isn't updated for self-only groups if bcc_self is disabled.

@link2xt link2xt force-pushed the link2xt/fix-group-renaming branch from b4217fc to 41df6be Compare March 29, 2025 00:10
@link2xt
Copy link
Collaborator Author

link2xt commented Mar 29, 2025

Still, why don't just move the block updating GroupNameTimestamp above? This way we save one db query and also that block goes after

I have moved it in a separate commit to avoid short-circuit in case of no recipients, but don't see how can it fix the original bug. The problem is with the chat param, not message param, and nothing updates the Chat saved inside the mime parser which is then used when rendering.

@link2xt link2xt force-pushed the link2xt/fix-group-renaming branch from 41df6be to 194ccfd Compare March 29, 2025 01:16
@link2xt link2xt force-pushed the link2xt/test-vcard-contacts branch from 7daba40 to dc2e4df Compare March 29, 2025 15:23
@link2xt link2xt force-pushed the link2xt/fix-group-renaming branch from 194ccfd to 99a6756 Compare March 29, 2025 15:23
Base automatically changed from link2xt/test-vcard-contacts to main March 29, 2025 16:27
@link2xt link2xt merged commit 99a6756 into main Mar 29, 2025
45 of 46 checks passed
@link2xt link2xt deleted the link2xt/fix-group-renaming branch March 29, 2025 16:27
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.

Chan-Group-Name-Timestamp header contains the wrong timestamp when changing group name
3 participants