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

events(fix): add mentions in the plain edit event when replacing an event with no initial mentions #1991

Merged
merged 1 commit into from
Jan 7, 2025

Conversation

bnjbvr
Copy link
Contributor

@bnjbvr bnjbvr commented Jan 7, 2025

When creating a replacement event, there are two places where we set mentions, if provided:

  • on the event content itself,
  • on the m.new_content field.

Mentions are fully included in the m.new_content variant, but for the event content itself, mentions that were already present were filtered out. Unfortunately, this code was no correct when the original event had no mentions at all: in this case, the mentions present in the event content itself would be empty, because of a lack of a special case in the code handling those mentions.

The new test introduced acts as a regression test; it fails on main at line 869, because mentions are set to None there.

Found while working on matrix-org/matrix-rust-sdk#4464.

Copy link
Contributor

@zecakeh zecakeh left a comment

Choose a reason for hiding this comment

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

Thanks! Please remove the (fix) in the commit message.

@bnjbvr bnjbvr force-pushed the bnjbvr/fix-adding-mentions-when-no-initial-mention branch from d135ea2 to 2bfe3e5 Compare January 7, 2025 14:42
@bnjbvr bnjbvr enabled auto-merge (rebase) January 7, 2025 14:42
@bnjbvr bnjbvr merged commit 5af5e77 into main Jan 7, 2025
21 checks passed
@bnjbvr bnjbvr deleted the bnjbvr/fix-adding-mentions-when-no-initial-mention branch January 7, 2025 15:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants