settings: Allow "resolve topic" permissions to be managed independently. #29965

roanster007 · 2024-05-06T16:25:36Z

This commit separates the "resolve topic" permissions from the topic editing permissions, through the introduction of two settings - "allow_resolve_topic" to control whether to allow resolve topics in organization, and "resolve_topic_policy" to restrict the resolve topics permissions.

In addition, it also restricts resolve topic permissions to streams where users have posting permissions.

Fixes #21811, #21281

#api desing thread

Screenshots and screen captures:

Self-review checklist

Self-reviewed the changes for clarity and maintainability
(variable names, code reuse, readability, etc.).

Communicate decisions, questions, and potential concerns.

Explains differences from previous plans (e.g., issue description).
Highlights technical choices and bugs encountered.
Calls out remaining decisions and concerns.
Automated tests verify logic where appropriate.

Individual commits are ready for review (see commit discipline).

Each commit is a coherent idea.
Commit message(s) explain reasoning and motivation for changes.

Completed manual review and testing of the following:

Visual appearance of the changes.
Responsiveness and internationalization.
Strings and tooltips.
End-to-end functionality of buttons, interactions and flows.
Corner cases, error conditions, and easily imagined bugs.

zulipbot · 2024-05-06T16:25:43Z

Hello @zulip/server-settings members, this pull request was labeled with the "area: settings (admin/org)" label, so you may want to check it out!

timabbott · 2024-05-06T16:54:14Z

So I guess I should have made sure folks were aware of this previously, but all future permissions settings need to be done using groups (see #19525), and likely building on the approach in #29937.

Maybe you can chat with @sahil839, but I think you might be able to build this on top of #29937, which should merge in the next couple days.

roanster007 · 2024-05-09T11:02:30Z

web/src/server_events_dispatch.js

@@ -302,7 +303,7 @@ export function dispatch_normal_event(event) {
 gear_menu.rerender();
 }

- if (key === "edit_topic_policy") {
+ if (key === "edit_topic_policy" || key === "resolve_topic_group") {
 message_live_update.rerender_messages_view();
 }


so as to update the resolve topic icon in the message list.

roanster007 · 2024-05-09T11:04:37Z

web/src/user_groups.ts

+ if (setting_name === "resolve_topic_group") {
+ group_setting_config = {...group_setting_config, allow_nobody_group: false};
+ }
+


This is done to remove the "role:nobody" from dropdown options in case of resolve_topic_group since it is represented by checkbox.

roanster007 · 2024-05-09T11:07:25Z

web/src/settings_org.js

+ if (
+ realm.realm_resolve_topic_group === user_groups.get_user_group_from_name("role:nobody").id
+ ) {
+ update_resolve_topic_sub_settings(false);


When initial group of the resolve_topic_group is "role:nobody", we don't want to show it in dropdown option. Hence, we rather show the checkbox as unchecked and dropdown disabled with "role:members" (default).

I am not sure if there is need to show the option selected as members when the option is disabled. We can just show the option which was selected before the setting was disabled and I guess this is what we do for other similar settings. Maybe we can have a tooltip on hovering over it that "Resolving topics is not allowed in this organization".

And you can just call the code to disable the dropdown initially in build_page.

We can just show the option which was selected before the setting was disabled and I guess this is what we do for other similar settings.

The problem here is that we are using just one database field for both allow_resolve_topic and can_resolve_topics_group settings. So technically, if you uncheck the checkbox, the value of this field is role:nobody -- which removes the possibility of storing the last option of dropdown. Hence, I thought I would just simply show the default value (members).

roanster007 · 2024-05-09T11:09:02Z

web/src/settings_org.js

+ const is_checked = $(e.target).prop("checked");
+ update_resolve_topic_sub_settings(is_checked);
+ });
+


This is for disabling/enabling the dropdown depending on the allow_resolve_topic checkbox.

roanster007 · 2024-05-09T11:12:41Z

web/src/settings_org.js

+ property_name,
+ property_value,
+ );
+ } else {


When the permission is set to role:nobody using checkbox, when resolve_topic_group is updated, we set the dropdown value with updated one only if the value !== role:nobody.id. Else, we just disable dropdown.

roanster007 · 2024-05-09T11:32:03Z

web/src/settings_org.js

+ if (input_value === false) {
+ user_group = user_groups.get_user_group_from_name("role:nobody");
+ data.resolve_topic_group = user_group.id;
+ } else {


When submitting request, if allow_resolve_topic checkbox is checked, we send a request of resolve_topic_group = dropdown value. Else, we send request of resolve_topic_group = role:nobody.id.

roanster007 · 2024-05-09T11:37:45Z

zerver/views/message_edit.py

+def is_resolve_topic_request(
+ user_profile: UserProfile,
+ message_id: PathOnly[NonNegativeInt],
+ new_topic_name: OptionalTopic,


This is used for determining if the message edit request is a resolve topic request, and calling the corresponding resolve topic method in actions. Not sure if is_resolve_topic_request is a good name.

roanster007 · 2024-05-09T11:38:26Z

zerver/views/message_edit.py

+ ) and not new_topic_name.startswith(RESOLVED_TOPIC_PREFIX)
+
+ if topic_resolved or topic_unresolved:
+ return True


I believe the conditions are enough for determining if it is a resolve topic request?

roanster007 · 2024-05-11T13:40:13Z

addressed review:

I think can_resolve_topics_group would be a name that fits the pattern we've begun to use in other places (like can_remove_subscribers_group).

Renamed resolve_topic_group to can_resolve_topics_group.

roanster007 · 2024-05-18T09:50:15Z

Updated it to use the groups object rather than group ids.

roanster007 · 2024-05-18T13:17:40Z

zerver/views/realm.py

+ return setting_value["direct_subgroups"][0]
+
+ return AnonymousSettingGroupDict( # nocoverage
+ direct_members=setting_value["direct_members"],


No setting that returns AnonymousSettingGroupDict currently in views/realm. Hence, no coverage for now

sahil839

Posted some initial comments. Only reviewed backend changes for now, will review the remaining tommorrow.

sahil839 · 2024-05-20T13:45:39Z

zerver/views/realm.py

@@ -101,6 +132,9 @@ def update_realm(
 edit_topic_policy: Optional[int] = REQ(
 json_validator=check_int_in(Realm.EDIT_TOPIC_POLICY_TYPES), default=None
 ),
+ can_resolve_topics_group_id: Optional[Union[Dict[str, List[int]], int]] = REQ(


It would be better to just name this as can_resolve_topics_group now since it can also be a dict.

I see the docs mention that can_resolve_topics_group can only be set to a system group, so there is no advantage of this conversion and this should just be kept as int. I guess we would support setting it to any group(s) in future, so would be good to just confirm it once whether we want to support only system groups.

Ya, I just realised that there is an assertion that is preventing me to access the group setting if permission_configuration.require_system_group is True (if the setting value is not int), which in this case would hence raise error.

Would still be good to confirm if we just want to allow any group(s) in the API and we can just design the API according to that now instead of reworking it later. We can allow only system groups in the UI for now though or just allow all groups in the UI as well but only single group.

sahil839 · 2024-05-20T13:56:58Z

zerver/models/realms.py

+ allow_everyone_group=False,
+ default_group_name=SystemGroups.MEMBERS,
+ id_field_name="can_resolve_topics_group_id",
+ allowed_system_groups=[


We generally use this only if we want to allow a couple of groups and we cannot do that using allow_... parameters, which I think is not the case here and you can just keep this list empty.

ya makes sense! updated

sahil839 · 2024-05-20T18:05:48Z

zerver/actions/message_edit.py

+ raise JsonableError(_("Direct messages cannot be moved to channels."))
+
+ if not user_profile.can_resolve_topic():
+ raise JsonableError(_("You don't have permission to resolve topics."))


This duplicates some code from check_update_message. Maybe we can do some refactor to still have separate functions but reduce the duplication.

Only the message.is_stream_message() is duplicated. user_profile.can_resolve_topic() is a new method. So I guess it is fine?

But this also needs changes like below to chck permission to move between streams. Might be better to just have a function named check_resolve_topic which is called from check_update_message if the user is trying to resolve the topic.

the problem with calling it from check_update_message is that there are also lots of checks for the time limit for editing, but according to this issue - There should not be an option to time-limit topic resolution..

Another problem is it only checks the stream post policy if stream_id is None. For resolving the topics, we also need to check it here.

sahil839 · 2024-05-20T18:07:00Z

zerver/actions/message_edit.py

+ and raises a JsonableError if otherwise.
+ It returns the number changed.
+ """
+ message = access_message(user_profile, message_id, lock_message=True)


This is also called in is_resolve_topic_request, I guess we should try removing one of them.

sahil839 · 2024-05-20T18:07:58Z

zerver/views/realm.py

+ ]
+ ),
+ ]
+)


I was working on a PR for converting this to typed_endpoint. I will try to open the PR tommorrow, so that we do not need these changes.

roanster007 · 2024-05-22T22:04:51Z

@sahil839 addressed your reviews.

sahil839

Posted some comments.

Much of the frontend code would have been simpler if could just avoid the checkbox and keep the dropdown with "Nobody" option, but yeah having a checkbox to disable resolving seems good from UX perspective so it is fine for now.

sahil839 · 2024-05-23T09:16:05Z

web/src/message_list_view.js

+ }
+ const sub = stream_data.get_sub(stream_name);
+ group.user_can_resolve_topic =
+ stream_data.can_post_messages_in_stream(sub) && settings_data.user_can_resolve_topic();


Has this been discussed to allow only users who can post messages in stream to allow resolving topics?

yup, it is related to this issue - #21281

Yeah it is related, but #21811 mentions that the behavior should be discussed.

sahil839 · 2024-05-23T09:17:29Z

web/src/settings_components.ts

@@ -891,6 +909,9 @@ export function check_property_changed(
 case "field_data":
 proposed_val = get_input_element_value(elem, "field-data-setting");
 break;
+ case "realm_allow_resolve_topic":
+ proposed_val = get_input_element_value(elem, "checkbox");


I don't think this is required as we do not have this for other checkbox settings.

oh ya right, thanks for catching it. Must have left it when I updated the setting. updated it.

sahil839 · 2024-05-23T09:25:14Z

web/src/settings_org.js

+ unique_id_type: dropdown_widget.DataTypes.NUMBER,
+ });
+ settings_components.set_can_resolve_topics_group_widget(can_resolve_topics_group_widget);
+ can_resolve_topics_group_widget.setup();


We should look to somehow deduplicate this as a follow-up, else there will be lot of these when we would convert existing settings to be group based settings.

sure! do you want me to update in this rather?

sahil839 · 2024-05-23T09:35:02Z

zerver/actions/message_edit.py

+ raise JsonableError(_("Direct messages cannot be moved to channels."))
+
+ if not user_profile.can_resolve_topic():
+ raise JsonableError(_("You don't have permission to resolve topics."))


But this also needs changes like below to chck permission to move between streams. Might be better to just have a function named check_resolve_topic which is called from check_update_message if the user is trying to resolve the topic.

sahil839 · 2024-05-23T09:39:49Z

zerver/actions/message_edit.py

+ if not user_profile.can_move_messages_between_streams():
+ raise JsonableError(_("You don't have permission to move this message"))
+ new_stream = access_stream_by_id(user_profile, stream_id, require_active=True)[0]
+ check_stream_access_based_on_stream_post_policy(user_profile, new_stream)


I am not sure but should we check the stream posting permission in original stream as well as resolving topics is only allowed if you are allowed to post in the original stream.

ya it checks the permission in the original stream in the else statement below.

sahil839 · 2024-05-23T10:36:39Z

web/src/settings_components.ts

+ const group_id = can_resolve_topics_group_widget.value();
+ assert(typeof group_id === "number");
+ data.can_resolve_topics_group = group_id;
+ continue;


Can you just move this code to a function which will be called only if property_name === "allow_resolve_topic" || property_name === "can_resolve_topics_group" and the function should just return the value of setting by checking two values - it should return "nobody" group if the checkbox is unchecked and the group the ID from the dropdown widget if it is checked.

ya you're right. it makes it a lot cleaner. updated!

sahil839 · 2024-05-23T10:40:55Z

web/src/settings_org.js

+ if (
+ realm.realm_resolve_topic_group === user_groups.get_user_group_from_name("role:nobody").id
+ ) {
+ update_resolve_topic_sub_settings(false);


I am not sure if there is need to show the option selected as members when the option is disabled. We can just show the option which was selected before the setting was disabled and I guess this is what we do for other similar settings. Maybe we can have a tooltip on hovering over it that "Resolving topics is not allowed in this organization".

sahil839 · 2024-05-23T10:41:42Z

web/src/settings_org.js

+ if (
+ realm.realm_resolve_topic_group === user_groups.get_user_group_from_name("role:nobody").id
+ ) {
+ update_resolve_topic_sub_settings(false);


And you can just call the code to disable the dropdown initially in build_page.

sahil839 · 2024-05-23T10:56:36Z

web/src/settings_org.js

@@ -567,6 +585,10 @@ export function discard_property_element_changes(elem, for_realm_default_setting
 case "message_retention_days":
 set_message_retention_setting_dropdown(sub);
 break;
+ case "allow_resolve_topic":
+ settings_components.set_input_element_value($elem, property_value);
+ update_resolve_topic_sub_settings(property_value);


I think we do not need this. When this function will be called for "allow_resolve_topic" it will also be called for "realm_can_resolve_topics_group", so the update_resolve_topic... part will be handled. And the set_input_element_value will be called in "default" case. Can you check this once?

ya you're right. updated!

sahil839 · 2024-05-23T10:59:19Z

zerver/views/realm.py

@@ -101,6 +132,9 @@ def update_realm(
 edit_topic_policy: Optional[int] = REQ(
 json_validator=check_int_in(Realm.EDIT_TOPIC_POLICY_TYPES), default=None
 ),
+ can_resolve_topics_group_id: Optional[Union[Dict[str, List[int]], int]] = REQ(


Would still be good to confirm if we just want to allow any group(s) in the API and we can just design the API according to that now instead of reworking it later. We can allow only system groups in the UI for now though or just allow all groups in the UI as well but only single group.

roanster007 · 2024-05-24T13:14:55Z

addressed all the reviews. posted replies for this and this

This commit separates the "resolve topic" permissions from the topic editing permissions, through the introduction of setting - "can_resolve_topics_group" which is the groups id of the user group whose members can resolve topics. In addition, it also restricts resolve topic permissions to streams where users have posting permissions. Fixes zulip#21811

zulipbot added size: XL area: settings (admin/org) priority: high labels May 6, 2024

roanster007 force-pushed the iss-21811 branch from 6dfcb28 to f46da5a Compare May 6, 2024 16:27

roanster007 marked this pull request as draft May 6, 2024 16:36

roanster007 force-pushed the iss-21811 branch 5 times, most recently from 8cb9fd4 to e7c6889 Compare May 8, 2024 16:12

roanster007 marked this pull request as ready for review May 8, 2024 16:13

roanster007 force-pushed the iss-21811 branch from e7c6889 to 217a7c7 Compare May 8, 2024 16:20

roanster007 marked this pull request as draft May 8, 2024 19:22

roanster007 force-pushed the iss-21811 branch from 217a7c7 to 674dd07 Compare May 9, 2024 11:01

roanster007 commented May 9, 2024

View reviewed changes

roanster007 force-pushed the iss-21811 branch from 674dd07 to 20fb7d5 Compare May 9, 2024 11:35

roanster007 commented May 9, 2024

View reviewed changes

roanster007 force-pushed the iss-21811 branch from 20fb7d5 to 48af9d3 Compare May 9, 2024 11:52

roanster007 marked this pull request as ready for review May 9, 2024 12:03

roanster007 force-pushed the iss-21811 branch from 48af9d3 to bf7a0e1 Compare May 11, 2024 13:38

roanster007 force-pushed the iss-21811 branch from bf7a0e1 to 0de8550 Compare May 13, 2024 11:27

zulipbot added the has conflicts label May 14, 2024

roanster007 force-pushed the iss-21811 branch from 0de8550 to 682b78f Compare May 16, 2024 17:10

zulipbot removed the has conflicts label May 16, 2024

roanster007 force-pushed the iss-21811 branch 4 times, most recently from 1cabfda to 3c7449e Compare May 18, 2024 09:49

roanster007 force-pushed the iss-21811 branch from 3c7449e to ba1153a Compare May 18, 2024 13:16

roanster007 commented May 18, 2024

View reviewed changes

zulipbot added the has conflicts label May 20, 2024

roanster007 force-pushed the iss-21811 branch from ba1153a to 6f302a3 Compare May 20, 2024 17:05

zulipbot removed the has conflicts label May 20, 2024

sahil839 reviewed May 20, 2024

View reviewed changes

zulipbot added the has conflicts label May 20, 2024

roanster007 force-pushed the iss-21811 branch from 6f302a3 to 2083ab3 Compare May 22, 2024 13:24

zulipbot removed the has conflicts label May 22, 2024

zulipbot added the has conflicts label May 23, 2024

sahil839 reviewed May 23, 2024

View reviewed changes

roanster007 force-pushed the iss-21811 branch from 2083ab3 to 5e87a15 Compare May 24, 2024 11:52

zulipbot removed the has conflicts label May 24, 2024

roanster007 force-pushed the iss-21811 branch from 5e87a15 to 83625d4 Compare May 24, 2024 13:01

zulipbot added the has conflicts label May 28, 2024

roanster007 force-pushed the iss-21811 branch from 83625d4 to 7ea8a25 Compare May 30, 2024 11:26

zulipbot removed the has conflicts label May 30, 2024

settings: Allow "resolve topic" permissions to be managed independently. #29965

Are you sure you want to change the base?

settings: Allow "resolve topic" permissions to be managed independently. #29965

Conversation

roanster007 commented May 6, 2024 • edited

zulipbot commented May 6, 2024

timabbott commented May 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roanster007 May 9, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roanster007 commented May 11, 2024 • edited

roanster007 commented May 18, 2024

Choose a reason for hiding this comment

sahil839 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roanster007 May 22, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roanster007 May 22, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roanster007 commented May 22, 2024

sahil839 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roanster007 May 24, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roanster007 May 24, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roanster007 commented May 24, 2024

roanster007 commented May 6, 2024 •

edited

roanster007 May 9, 2024 •

edited

roanster007 commented May 11, 2024 •

edited

roanster007 May 22, 2024 •

edited

roanster007 May 22, 2024 •

edited

roanster007 May 24, 2024 •

edited

roanster007 May 24, 2024 •

edited