zulip-terminal icon indicating copy to clipboard operation
zulip-terminal copied to clipboard

[WIP] Add warning when sending message to announcement streams

Open imnitishng opened this issue 4 years ago • 12 comments

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

imnitishng avatar Jun 17 '20 19:06 imnitishng

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 avatar Jun 17 '20 19:06 imnitishng

@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

kaustubh-nair avatar Jun 17 '20 20:06 kaustubh-nair

@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.

neiljp avatar Jun 17 '20 21:06 neiljp

Hi @kaustubh-nair @neiljp, thanks a lot for the help. I believe we're good to go now? Please have a look.

imnitishng avatar Jun 20 '20 17:06 imnitishng

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 Screenshot from 2020-06-21 10-34-46 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

imnitishng avatar Jun 21 '20 05:06 imnitishng

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 ?

kaustubh-nair avatar Jun 21 '20 05:06 kaustubh-nair

maybe we'd have to use both?

I have updated the PR keeping this in mind. Please have a look.

imnitishng avatar Jun 25 '20 05:06 imnitishng

I think this will be simpler once #653 is merged, so we can check directly based on server versions

kaustubh-nair avatar Jun 25 '20 07:06 kaustubh-nair

@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.

neiljp avatar Jul 20 '20 05:07 neiljp

Hi @neiljp, I am occupied as of now so someone else can take this up.

imnitishng avatar Jul 20 '20 05:07 imnitishng

@imnitishng I've added WIP here to indicate the status, though you're welcome to take this up again at some point of course :)

neiljp avatar Nov 23 '20 04:11 neiljp

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.

zulipbot avatar Jan 30 '21 20:01 zulipbot