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

Restrict posting messages on streams having moderators/admins only

Open mounilKshah opened this issue 3 years ago • 4 comments

What does this PR do?

  • Added is_unauthorised_to_post() function to verify if a user is allowed to message in the given stream or not
  • If the user is not allowed, restrict compose box from opening
  • Print a warning in the footer
  • Test functions (can post and can-not post) for is_unauthorised_to_post() function Partly fixes #682

Discussion on developer channel: [#zulip-terminal> Restrict sending messages to streams #T1149 #T682 #api-documentation>stream_post_policy (API docs)

Tested?

  • [x] Manually
  • [x] Existing tests
  • [ ] Passed linting & tests (each commit)
  • [x] New tests added

Commit flow Only one commit which contains all the changes made

  • Creating is_unauthorised_to_post() in model.py
  • Using that function in boxes.py, the compose box is opened only when the user is authorised to send a message to the stream
  • Two new test functions added to test_model.py: -test_is_unauthorised_to_post_in_stream_can_post() and -test_is_unauthorised_to_post_in_stream_cannot_post()

Notes & Questions The current changes have covered the following cases:

  • Reply to a message in stream (r/enter)
  • Reply quoting the current message text (>)
  • Reply (in the same stream) mentioning the sender of the current message (@)

Use cases is still pending:

  • New message to a stream
  • Draft whose stream post policies have been changed
  • Full member

Example

Before implementing the changes

before_changes

Here, upon pressing enter/r for replying to the message in the announce stream, the compose box opens even though the user does not have permission to post on that stream

After implementing the changes

after_changes

Upon trying to reply to the message in the stream, no compose box pops up and a warning is displayed in the footer informing the user about the restrictions due to the stream post policy.

mounilKshah avatar Jan 29 '22 16:01 mounilKshah

@mounilKshah Just a note that generally it's fine (at least in Zulip) to force push a rebased version to a PR branch (where we also avoid merge commits). It's fine to show branch progress as you're coding as extra commits if it's something being explored, but otherwise it's better to show the current status of commits that you'd like to see merged. We can discuss on czo if you'd like to know more.

neiljp avatar Feb 03 '22 03:02 neiljp

@mounilKshah Just a note that generally it's fine (at least in Zulip) to force push a rebased version to a PR branch (where we also avoid merge commits). It's fine to show branch progress as you're coding as extra commits if it's something being explored, but otherwise it's better to show the current status of commits that you'd like to see merged. We can discuss on czo if you'd like to know more.

I am sorry for these extra commits. I will make sure to avoid any unnecessary forced pushes. I'll rebase them before making the next commit.

mounilKshah avatar Feb 04 '22 09:02 mounilKshah

@zulipbot remove "PR awaiting update" @zulipbot add "PR needs review"

mounilKshah avatar May 04 '22 06:05 mounilKshah

@mounilKshah I think you were planning to get this worked on more shortly, so I went through the code and conversation in the stream and left what I hope is a good summary of the current status in the stream topic, with some feedback and other ideas.

neiljp avatar Jun 21 '22 02:06 neiljp

Heads up @mounilKshah, we just merged some commits that conflict with the changes you 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 Feb 24 '23 21:02 zulipbot