[WIP] Add warning when sending message to announcement streams #693

imnitishng · 2020-06-17T19:03:50Z

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: #682

imnitishng · 2020-06-17T19:08:03Z

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

kaustubh-nair · 2020-06-17T20:21:29Z

@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

neiljp · 2020-06-17T21:55:43Z

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

imnitishng · 2020-06-20T17:08:47Z

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

neiljp

@imnitishng This looks to use the existing code more, but after a little manual testing it seems to get stuck. I've given a pointer as to why this is the case and how you might move the code around to fix it.

Note that you can place the 'fixes' line at the end of the commit too, and it will close the issue when the commit is merged :)

We don't seem to have the stream property you're using here in any test or fixtures yet, so if you wanted to work on that at the same time or afterwards you'd be welcome to do that. 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.

zulipterminal/ui_tools/boxes.py

imnitishng · 2020-06-21T05:11:06Z

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

kaustubh-nair · 2020-06-21T05:16:21Z

Ah, good job noticing that - I think using stream_post_policy is a better idea 👍
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 ?

imnitishng · 2020-06-25T05:56:53Z

maybe we'd have to use both?

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

kaustubh-nair · 2020-06-25T07:26:49Z

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

neiljp

@imnitishng This seems to work fine in my manual testing, in terms of limiting myself posting as a non-admin 👍

I'm happy to merge a simple version similar to this, but if we look for the next version features then we should consider how to structure and test it further. Let us know how you far you'd like to take this 👍

neiljp · 2020-06-26T06:47:09Z

zulipterminal/ui_tools/boxes.py

+                if (self.model.stream_dict[self.message.get('stream_id')].
+                        get('is_announcement_only', None) == 1
+                    or (self.model.initial_data.get('is_admin', None)
+                    or self.model.initial_data.get('is_owner', None)
+                    and self.model.stream_dict[self.message.get('stream_id')].
+                        get('stream_post_policy', None) == 2)):


is_announcement_only is still present in what will soon be 3.0, so we could stick with just it for now to begin with, since it works across the versions we plan to support.

If we support the new versions, we should likely have this logic in the model, in any case.

We'd ideally want tests against the two different server versions (or feature levels). While we added some background for this in #653, we don't have good examples of using this in a feature like this yet, so you're welcome to give it a go but you may wish to discuss on the stream about this first.

preetmishra

@imnitishng Thanks for working on this. I ran your changes locally. It seems to work well for the case when a message is directly being sent from the 'announce' stream. 👍

However, when the compose box is already open, in some other stream, and I am trying to send a message to the 'announce' stream, there is no feedback. We may want to have a footer update for this case as well.

neiljp · 2020-07-20T05:10:13Z

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

imnitishng · 2020-07-20T05:13:03Z

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

neiljp · 2020-11-23T04:00:58Z

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

This method acts as an sanitary check when a normal user is sending message to a stream where only organization admins are allowed to send. The method also updates the footer with appropriate warning.

This commit adds warning for when an unauthorised user tries to send a message to an admin only stream.

zulipbot · 2021-01-30T20:34:40Z

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 added the size: M [Automatic label added by zulipbot] label Jun 17, 2020

imnitishng force-pushed the restrict-announcment branch from 237cb26 to 935b5dd Compare June 20, 2020 17:02

zulipbot added size: L [Automatic label added by zulipbot] and removed size: M [Automatic label added by zulipbot] labels Jun 20, 2020

imnitishng changed the title ~~boxes: Add unauthorised write box for streams with user restrictions~~ boxes: Add warning when sending message to announcement streams Jun 20, 2020

neiljp reviewed Jun 21, 2020

View reviewed changes

zulipterminal/ui_tools/boxes.py Outdated Show resolved Hide resolved

imnitishng force-pushed the restrict-announcment branch from 935b5dd to 47a1f88 Compare June 21, 2020 04:19

imnitishng force-pushed the restrict-announcment branch from 47a1f88 to e022c05 Compare June 25, 2020 05:43

zulipbot added size: S [Automatic label added by zulipbot] and removed size: L [Automatic label added by zulipbot] labels Jun 25, 2020

imnitishng force-pushed the restrict-announcment branch from e022c05 to d233d08 Compare June 25, 2020 05:54

neiljp reviewed Jun 26, 2020

View reviewed changes

preetmishra reviewed Jul 2, 2020

View reviewed changes

neiljp added the issue requires user response Issue cannot be progressed without further information label Jul 20, 2020

neiljp changed the title ~~boxes: Add warning when sending message to announcement streams~~ [WIP] Add warning when sending message to announcement streams Nov 23, 2020

neiljp removed the issue requires user response Issue cannot be progressed without further information label Nov 23, 2020

imnitishng force-pushed the restrict-announcment branch from d233d08 to 84caa04 Compare December 16, 2020 21:47

imnitishng force-pushed the restrict-announcment branch from 84caa04 to cdc1b7b Compare January 19, 2021 15:56

zulipbot removed the size: S [Automatic label added by zulipbot] label Jan 19, 2021

zulipbot added the size: L [Automatic label added by zulipbot] label Jan 19, 2021

imnitishng added 2 commits January 19, 2021 21:49

helper.py: Add method unauthorised_warning to update footer

a7b99be

This method acts as an sanitary check when a normal user is sending message to a stream where only organization admins are allowed to send. The method also updates the footer with appropriate warning.

boxes.py: Add warning when user without permission sends message

9eebbee

This commit adds warning for when an unauthorised user tries to send a message to an admin only stream.

imnitishng force-pushed the restrict-announcment branch from cdc1b7b to 9eebbee Compare January 19, 2021 16:28

neiljp added this to the Release after next milestone Jan 28, 2021

Base automatically changed from master to main January 30, 2021 20:30

zulipbot added the has conflicts label Jan 30, 2021

neiljp force-pushed the main branch from f9f483a to 793e73d Compare December 15, 2021 21:30

neiljp modified the milestones: Next Release, Release after next Apr 12, 2022

Uh oh!

[WIP] Add warning when sending message to announcement streams #693

Are you sure you want to change the base?

[WIP] Add warning when sending message to announcement streams #693

Uh oh!

Conversation

imnitishng commented Jun 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

imnitishng commented Jun 17, 2020

Uh oh!

kaustubh-nair commented Jun 17, 2020

Uh oh!

neiljp commented Jun 17, 2020

Uh oh!

imnitishng commented Jun 20, 2020

Uh oh!

neiljp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

imnitishng commented Jun 21, 2020

Uh oh!

kaustubh-nair commented Jun 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

imnitishng commented Jun 25, 2020

Uh oh!

kaustubh-nair commented Jun 25, 2020

Uh oh!

neiljp left a comment

Choose a reason for hiding this comment

Uh oh!

neiljp Jun 26, 2020

Choose a reason for hiding this comment

Uh oh!

preetmishra left a comment

Choose a reason for hiding this comment

Uh oh!

neiljp commented Jul 20, 2020

Uh oh!

imnitishng commented Jul 20, 2020

Uh oh!

neiljp commented Nov 23, 2020

Uh oh!

zulipbot commented Jan 30, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

imnitishng commented Jun 17, 2020 •

edited

Loading

kaustubh-nair commented Jun 21, 2020 •

edited

Loading