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

bugfix: messages: Fix crash when editing message #1468

Merged
merged 2 commits into from
Feb 9, 2024
Merged

bugfix: messages: Fix crash when editing message #1468

merged 2 commits into from
Feb 9, 2024

Conversation

Young-Lord
Copy link
Contributor

What does this PR do, and why?

When realm_message_content_edit_limit_seconds is None, which means no time limit for editing message, ZT crashes with TypeError.
This happens in Zulip cloud server.
This PR fix it.

External discussion & connections

  • Discussed in #zulip-terminal in topic
  • Fully fixes [bug] crash when editing message #1467
  • Partially fixes issue #
  • Builds upon previous unmerged work in PR #
  • Is a follow-up to work in PR #
  • Requires merge of PR #
  • Merge will enable work on #

How did you test this?

  • Manually - Behavioral changes
  • Manually - Visual changes
  • Adapting existing automated tests
  • Adding automated tests for new behavior (or missing tests)
  • Existing automated tests should already cover this (only a refactor of tested code)

Self-review checklist for each commit

  • It is a minimal coherent idea
  • It has a commit summary following the documented style (title & body)
  • It has a commit summary describing the motivation and reasoning for the change
  • It individually passes linting and tests
  • It contains test additions for any new behavior
  • It flows clearly from a previous branch commit, and/or prepares for the next commit

@zulipbot zulipbot added the size: S [Automatic label added by zulipbot] label Feb 7, 2024
@neiljp
Copy link
Collaborator

neiljp commented Feb 8, 2024

@Young-Lord Thanks for finding the bug and providing this fix! :)

I can add a test case for this situation before merging, unless you wish to do so?

@neiljp neiljp added the bug Something isn't working label Feb 8, 2024
@Young-Lord
Copy link
Contributor Author

I don't know how to add a test case, so maybe you can do that.

@neiljp
Copy link
Collaborator

neiljp commented Feb 8, 2024

No problem :)

Young-Lord and others added 2 commits February 8, 2024 18:32
When `realm_message_content_edit_limit_seconds` is None, which now means
no time limit for editing messages, ZT crashes with a `TypeError`.

This was diagnosed in Zulip cloud, but may be triggered for any version
of the server from feature level 138, which was released first in Zulip
server 6.0. Prior to this feature level, no limit on editing messages
was represented by a numerical value of zero; a direct numerical
comparison was therefore possible, but now causes an error.

Fixes #1467.
Confirmed as failing, if prior to the contributed bugfix in the previous
commit.
@zulipbot zulipbot added size: M [Automatic label added by zulipbot] and removed size: S [Automatic label added by zulipbot] labels Feb 9, 2024
@neiljp neiljp added this to the Next Release milestone Feb 9, 2024
@neiljp
Copy link
Collaborator

neiljp commented Feb 9, 2024

@Young-Lord Thanks again for finding and fixing! I slightly updated your commit text, and added a commit which covers testing of the new behavior, and am merging now 🎉

@neiljp neiljp merged commit ba5e27a into zulip:main Feb 9, 2024
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api migrations area: message editing bug Something isn't working size: M [Automatic label added by zulipbot]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[bug] crash when editing message
3 participants