Restrict posting messages on streams having moderators/admins only #1149
Conversation
zulipbot
added
size: XL
There was a problem hiding this comment.
@mounilKshah Thanks for working on this! 👍
I've left some feedback inline, let me know if you have questions.
Once the code is moved and the logic is isolated, I'd highly recommend adding some tests for the new model function, as that will ensure we cover a lot (or all) of the combinations of stream settings, server levels (if necessary) and user roles.
neiljp
added
area: UI
zulipbot
added
size: L
mounilKshah
force-pushed
the
restrict-messages-to-announcement-streams
branch
3 times, most recently
from
84a52d1
to
62c2e77
Compare
There was a problem hiding this comment.
@mounilKshah This is looking cleaner already, including after that black issue!
Once you've extracted some variables this will read much simpler :)
Given the function behaves a little like a boolean, we could consider naming it with an is_ prefix or similar, and of course this doesn't need to refer to the compose box specifically now.
As per my earlier review, I'd recommend investigating tests of the model method now, or perhaps once you've pushed a version with these changes. The user roles should be already independent of server version via the function you're already calling, but a parametrised test will help show confidence in behavior.
zulipterminal/model.py
Outdated
| if self.stream_dict[stream_identifier].get( | ||
| "stream_post_policy" | ||
| ) == 2 or self.stream_dict[stream_identifier].get("is_announcement_only"): |
There was a problem hiding this comment.
As with the other file, extracting these two elements above the conditional will make each branch of the conditional much easier to read if you give these good names.
There was a problem hiding this comment.
You've done this, but note that you've replaced a .get with an index; this may or may not be a problem, but this will be clearer once you have test cases for different server versions.
zulipterminal/model.py
Outdated
| self.get_user_info(self.user_id).get("role") <= 200 | ||
| or self.initial_data.get("is_admin", True) | ||
| or self.initial_data.get("is_owner", True) |
There was a problem hiding this comment.
I'm guessing get_user_info could work here, but the type is wrong so mypy is complaining.
Do we need more than the get_user_info?
In principle we could track the role of the user who is running the app separately, but we already have the logic in get_user_info. That method does currently do a lot of work which we won't use here, however.
There was a problem hiding this comment.
I too thought of eliminating the use of the function by extracting value of the role variable alone. However, going through the changelogs, I noticed that role was introduced in Zulip 4.0 (ZFL 59) and the moderator role was added in Zulip 4.0 (ZFL 60), so using role alone can cause problems with respect to backwards compatibility. We would also have to use the is_admin and is_owner attributes to make sure it runs smoothly for earlier versions as well.
One way to do it would be following your suggestion about assigning the user information to a variable using just the first three lines of get_user_info. After assigning, we can verify whether the given user exists or not (as the mypy error is due to the Optional return type of get_user_info). I tried this on my local machine and mypy didn't throw any errors for it.
There was a problem hiding this comment.
I'm a little wary of duplicating the logic in this new method. That said, let's get it working and tested against some known server responses, and we can refactor next.
There was a problem hiding this comment.
Considering your suggestion regarding get_user_info doing a lot more than what we need here, I considered this logic as an alternative to using the get_user_info function within the new method.
zulipterminal/model.py
Outdated
| return "Only Admins, moderators and owners are allowd to type" | ||
| elif self.stream_dict[stream_identifier].get("stream_post_policy") == 3: | ||
| # checks whether the user is an admin/owner/moderator/full member | ||
| if self.get_user_info(self.user_id).get("role") <= 400: |
There was a problem hiding this comment.
This includes all users; a "full user" is subtly different and can be time-limited (eg. so new members cannot write to some streams). Check the zulip help documentation.
There was a problem hiding this comment.
Yes, that is the definition which had been suggested on the #api-documentation>stream_post_policy stream.
However, as suggested by Tim in the #api-documentation>stream_post_policy, I checked with the help center documentation and here they have mentioned that a full member is
Member: Has access to all public streams. Member is the default role for most users. Some organization settings allow an organization to restrict the permissions of new members; Members who do not have those restrictions are called full members.
So the definition is not limited to the new members who are members for the waiting_period_threshold. Hence, I kept a separate conditional statement for this as well.
There was a problem hiding this comment.
Ah, OK, so you know about it but you've left the waiting_period_threshold part unimplemented at this point?
There was a problem hiding this comment.
I hadn't actually thought about its implementation yet. However, I'll look into it.
On a side note, I couldn't find any references to waiting_period_threshold in the terminal repository nor in the (API and Developer) documentation. Can you please guide me on that?
There was a problem hiding this comment.
I see you posted about that; you could check the server code in the meantime.
|
@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. |
mounilKshah
force-pushed
the
restrict-messages-to-announcement-streams
branch
from
698deee
to
4799355
Compare
There was a problem hiding this comment.
@mounilKshah As per another comment, this looks a lot cleaner 👍
Now seems a good time to write some tests for the function added to model.py for different user/stream combinations, for different server versions. That will ensure that the behavior is as we expect without manually testing every combination.
zulipbot
added
size: XL
mounilKshah
force-pushed
the
restrict-messages-to-announcement-streams
branch
from
166f33f
to
594d96d
Compare
mounilKshah
force-pushed
the
restrict-messages-to-announcement-streams
branch
3 times, most recently
from
159e039
to
a6006b6
Compare
| case( | ||
| {"is_owner": False, "is_admin": False, "is_guest": False}, | ||
| {"is_announcement_only": False, "stream_post_policy": 3}, | ||
| id="is_member:members_only:Zulip3.0+:ZFL8", | ||
| ), |
There was a problem hiding this comment.
stream_post_policy == 3 appears to only be valid for full members, not all members. The distinction between these depends on how a server is configured, which I don't believe we detect right now.
| case( | ||
| {"is_admin": False, "is_owner": False, "is_guest": False, "role": 400}, | ||
| {"is_announcement_only": False, "stream_post_policy": 3}, | ||
| id="is_member:members_only:Zulip4.0+:ZFL59", | ||
| ), |
zulipterminal/model.py
Outdated
| if ( | ||
| stream_dict.get("is_announcement_only") | ||
| or stream_dict.get("stream_post_policy") == 2 | ||
| ): | ||
| # For admins and owners only (not using user role) |
There was a problem hiding this comment.
In future servers there will likely be only stream_post_policy, so maybe we should check for the role here?
Could we simplify the conditionals overall, since == 2 and is_announcement_only are the same?
There was a problem hiding this comment.
Actually, role falls under ZFL 59, whereas is_admin, is_guest and is_owner are under ZFL 8/10. Hence, I felt that we could do the checking without using role. However, I'll add role.
is_announcement_only got deprecated after ZFL 1 right? Hence, I decided to keep both so that it can function the same even after we stop support for v2.1.
mounilKshah
force-pushed
the
restrict-messages-to-announcement-streams
branch
3 times, most recently
from
3af81f6
to
55f992b
Compare
mounilKshah
force-pushed
the
restrict-messages-to-announcement-streams
branch
from
55f992b
to
845c283
Compare
| return None | ||
| else: | ||
| return STREAM_POST_POLICY[2] | ||
| elif stream.get("stream_post_policy") is not None: |
There was a problem hiding this comment.
@neiljp
In my opinion the condition can be changed to – "stream_post_policy" in stream
Apart from this, the rest of the function seems fine to me in the current condition.
mounilKshah
force-pushed
the
restrict-messages-to-announcement-streams
branch
from
845c283
to
224d830
Compare
zulipbot
removed
the
PR awaiting update
There was a problem hiding this comment.
@mounilKshah Thanks for getting back to this 👍
I'd suggest splitting the core function+test into an initial commit and then connecting the UI in one or more later commits - for now the second commit is completely removing some code you added in the first commit, which is not how we'd approach step-by-step commits in a PR branch. It also is easier to see an isolated function and the tests for it separately.
Regarding the new behavior, I tried some of this out and we can clarify in the stream, but broadly I'd suggest:
- we disallow simple replies and user mention replies (as you had before)
- we open the compose box for quoted replies (as you have now, to allow content quoting to another stream)
In any case I think we need to extend the time the warnings are shown?
I'm not sure if the filter in the comprehension you added in the second commit is for the reply processing, or a step towards further work? It limits autocomplete, but doesn't handle the case of c while in the stream, and also entering text manually doesn't stop compose either.
zulipterminal/model.py
Outdated
| if user_info.get("role") is None: | ||
| if user_info.get("is_owner"): | ||
| role = 100 | ||
| elif user_info.get("is_admin"): | ||
| role = 200 | ||
| elif user_info.get("is_guest"): | ||
| role = 600 | ||
| else: | ||
| role = 400 | ||
| else: | ||
| role = user_info["role"] |
There was a problem hiding this comment.
This is cleaner 👍
I think this could be combined with an extraction from get_user_info into a new method, since I believe the code is effectively the same? We discussed this previously, and I think we now have the same behavior?
This is possibly missing is_moderator, though that might be an edge case.
neiljp
added
PR awaiting update
mounilKshah
force-pushed
the
restrict-messages-to-announcement-streams
branch
from
224d830
to
8df3905
Compare
mounilKshah
added
PR needs review
This commit contains is_unauthorised_to_post_in_stream function which restricts unauthorised users from writing/replying on unauthorised streams. Two test functions have been defined under test_model to test is_unauthorised_to_post_in_stream function. Fixes zulip#682.
This commit alters _stream_box_autocomplete() to avoid showing any restricted stream(s) in autocomplete suggestions.
This commit implements is_unauthorised_to_post() which was added in the previous commit. Fixes zulip#682.
mounilKshah
force-pushed
the
restrict-messages-to-announcement-streams
branch
from
8df3905
to
afb5625
Compare
| ) | ||
| if is_unauthorised_warning is not None: | ||
| self.model.controller.view.set_footer_text(is_unauthorised_warning, "task:warning", 8) |
There was a problem hiding this comment.
Here you can use report_warning function in core.py
| self.message["stream_id"] | ||
| ) | ||
| if is_unauthorised_warning is not None: | ||
| self.model.controller.view.set_footer_text(is_unauthorised_warning, "task:warning", 8) |
There was a problem hiding this comment.
Here also you can use report_warning function in core.py
| self.message["stream_id"] | ||
| ) | ||
| if is_unauthorised_warning is not None: | ||
| self.model.controller.view.set_footer_text(is_unauthorised_warning, "task:warning", 8) |
There was a problem hiding this comment.
Also, when we open the compose box (current narrow is unrestricted_stream) by using c shortcut for new message and press ctrl+d to send a message, it shows error footer_text.
To keep the styling uniform, maybe we can use report_error instead of report_warning
|
@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
added
PR awaiting update
|
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 |
What does this PR do?
is_unauthorised_to_post()function to verify if a user is allowed to message in the given stream or notis_unauthorised_to_post()functionPartly fixes Restrict sending messages to announcement streams #682
Discussion on developer channel:
[#zulip-terminal> Restrict sending messages to streams #T1149 #T682
#api-documentation>stream_post_policy (API docs)
Tested?
Commit flow
Only one commit which contains all the changes made
is_unauthorised_to_post()inmodel.pyboxes.py, the compose box is opened only when the user is authorised to send a message to the streamtest_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:
Use cases is still pending:
Example
Before implementing the 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
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.