zulip-terminal
zulip-terminal copied to clipboard
[WIP] Add warning when sending message to announcement streams
Normal users are not allowed to send messages in announcement streams, currently when an unauthorised user sends message to this stream, nothing happens. This commit changes this behaviour and adds a footer warning for the same.
Fixes: https://github.com/zulip/zulip-terminal/issues/682
Hi, maintainers. This is my first PR for the repository and terminal based applications with python is pretty new for me, so can I have a small review for this PR to point out the mistakes, because I believe this might not be the best way to implement this feature, also I have not written any tests for this. So there is a bit of work to do, I will complete it after the review. Thanks. :smile:
@imnitishng thanks for the PR!
You've got the sense of the issue correctly, and the solution does seem to work OK at a quick glance.
The way the error is displayed can be modified though - instead of adding the error to WriteBox
the message can be displayed in the footer using view.set_footer_text()
Also, WriteBox
should not open for this, i.e the user should not have the option to type a message at all.
If you want to discuss further, feel free to chat on https://chat.zulip.org/#narrow/stream/206-zulip-terminal
@imnitishng You seem to have the right broad idea here, but as @kaustubh-nair mentions, there is a pre-existing method we likely want to use here to set the text in the footer (for a short time). The check likely wants to be triggered upon any composing keypress.
Hi @kaustubh-nair @neiljp, thanks a lot for the help. I believe we're good to go now? Please have a look.
so if you wanted to work on that at the same time or afterwards you'd be welcome to do that.
Sure I would love to add some tests for this PR and other missed methods.
I've not checked which version of the server added the stream property we're using here, but we may need to think about that and check; we're currently focusing on targeting version 2.1 as the minimum server version.
I went through the changelogs of Zulip API and found that the attribute is_announcement_only
that we are using for this check is deprecated in Zulip 2.2
I think I should change the checking logic and use
stream_post_policy
instead, also add tests using the same attribute.
Please confirm. Thank You
Ah, good job noticing that - I think using stream_post_policy
is a better idea :+1:
I'd recommend holding off adding tests to WriteBox
/MessageBox
since it would conflict with some existing PRs (Like #675 )
Edit: Just noticed that stream_post_policy
would not work in 2.1, maybe we'd have to use both? @neiljp ?
maybe we'd have to use both?
I have updated the PR keeping this in mind. Please have a look.
I think this will be simpler once #653 is merged, so we can check directly based on server versions
@imnitishng Just checking with you if you plan to take this further in the near term, as this is something we'd like to integrate soon.
Hi @neiljp, I am occupied as of now so someone else can take this up.
@imnitishng I've added WIP here to indicate the status, though you're welcome to take this up again at some point of course :)
Heads up @imnitishng, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/main
branch and resolve your pull request's merge conflicts accordingly.