Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support to update can_access_all_users_group setting in dev environment. #27873

Merged
merged 2 commits into from
Nov 23, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions web/src/settings_components.js
Original file line number Diff line number Diff line change
Expand Up @@ -239,6 +239,7 @@ export let notifications_stream_widget = null;
export let signup_notifications_stream_widget = null;
export let create_multiuse_invite_group_widget = null;
export let can_remove_subscribers_group_widget = null;
export let can_access_all_users_group_widget = null;

export function get_widget_for_dropdown_list_settings(property_name) {
switch (property_name) {
Expand All @@ -252,6 +253,8 @@ export function get_widget_for_dropdown_list_settings(property_name) {
return create_multiuse_invite_group_widget;
case "can_remove_subscribers_group":
return can_remove_subscribers_group_widget;
case "realm_can_access_all_users_group":
return can_access_all_users_group_widget;
default:
blueslip.error("No dropdown list widget for property", {property_name});
return null;
Expand All @@ -278,6 +281,10 @@ export function set_can_remove_subscribers_group_widget(widget) {
can_remove_subscribers_group_widget = widget;
}

export function set_can_access_all_users_group_widget(widget) {
can_access_all_users_group_widget = widget;
}

export function set_dropdown_list_widget_setting_value(property_name, value) {
const widget = get_widget_for_dropdown_list_settings(property_name);
widget.render(value);
Expand Down
28 changes: 28 additions & 0 deletions web/src/settings_org.js
Original file line number Diff line number Diff line change
Expand Up @@ -477,6 +477,7 @@ export function discard_property_element_changes(elem, for_realm_default_setting
case "realm_default_code_block_language":
case "can_remove_subscribers_group":
case "realm_create_multiuse_invite_group":
case "realm_can_access_all_users_group":
settings_components.set_dropdown_list_widget_setting_value(
property_name,
property_value,
Expand Down Expand Up @@ -737,6 +738,33 @@ export function init_dropdown_widgets() {
create_multiuse_invite_group_widget,
);
create_multiuse_invite_group_widget.setup();

const can_access_all_users_group_widget = new dropdown_widget.DropdownWidget({
widget_name: "realm_can_access_all_users_group",
get_options: () =>
user_groups.get_realm_user_groups_for_dropdown_list_widget(
"can_access_all_users_group",
"realm",
),
$events_container: $("#settings_overlay_container #organization-permissions"),
item_click_callback(event, dropdown) {
dropdown.hide();
event.preventDefault();
event.stopPropagation();
settings_components.can_access_all_users_group_widget.render();
settings_components.save_discard_widget_status_handler($("#org-guest-settings"));
},
tippy_props: {
placement: "bottom-start",
},
default_id: page_params.realm_can_access_all_users_group,
unique_id_type: dropdown_widget.DATA_TYPES.NUMBER,
on_mount_callback(dropdown) {
$(dropdown.popper).css("min-width", "300px");
},
});
settings_components.set_can_access_all_users_group_widget(can_access_all_users_group_widget);
can_access_all_users_group_widget.setup();
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't a task for this PR, but as a follow-up, we should work to make this a component where one can just pass in the setting name and it'll initialize everything; or maybe even a fancier version where one can declare it just via including the right block in an HTML template.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I was also thinking of doing something similar, would be needed especially when we would add more group-based settings.

}

export function populate_data_for_request(subsection, for_realm_default_settings, sub) {
Expand Down
1 change: 1 addition & 0 deletions web/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -203,4 +203,5 @@ export type GroupPermissionSetting = {
default_group_name: string;
id_field_name: string;
default_for_system_groups: string | null;
allowed_system_groups: string[];
};
5 changes: 5 additions & 0 deletions web/src/user_groups.ts
Original file line number Diff line number Diff line change
Expand Up @@ -240,6 +240,7 @@ export function get_realm_user_groups_for_dropdown_list_widget(
allow_owners_group,
allow_nobody_group,
allow_everyone_group,
allowed_system_groups,
} = group_setting_config;

const system_user_groups = settings_config.system_user_groups_list
Expand All @@ -260,6 +261,10 @@ export function get_realm_user_groups_for_dropdown_list_widget(
return false;
}

if (allowed_system_groups.length && !allowed_system_groups.includes(group.name)) {
return false;
}

return true;
})
.map((group) => {
Expand Down
3 changes: 2 additions & 1 deletion web/styles/settings.css
Original file line number Diff line number Diff line change
Expand Up @@ -821,7 +821,8 @@ input[type="checkbox"] {
margin: 20px 0 0;
}

#org-join-settings {
#org-join-settings,
#org-guest-settings {
.dropdown-widget-button {
width: 325px;
color: hsl(0deg 0% 33%);
Expand Down
81 changes: 49 additions & 32 deletions web/templates/settings/organization_permissions_admin.hbs
Original file line number Diff line number Diff line change
Expand Up @@ -62,38 +62,6 @@
</div>
</div>

<div id="org-user-identity" class="settings-subsection-parent">
<div class="subsection-header">
<h3>{{t "User identity" }}</h3>
{{> settings_save_discard_widget section_name="user-identity" }}
</div>
<div class="inline-block organization-permissions-parent">
{{> settings_checkbox
setting_name="realm_name_changes_disabled"
prefix="id_"
is_checked=realm_name_changes_disabled
label=admin_settings_label.realm_name_changes_disabled}}

{{> settings_checkbox
setting_name="realm_email_changes_disabled"
prefix="id_"
is_checked=realm_email_changes_disabled
label=admin_settings_label.realm_email_changes_disabled}}

{{> settings_checkbox
setting_name="realm_avatar_changes_disabled"
prefix="id_"
is_checked=realm_avatar_changes_disabled
label=admin_settings_label.realm_avatar_changes_disabled}}

{{> settings_checkbox
setting_name="realm_enable_guest_user_indicator"
prefix="id_"
is_checked=realm_enable_guest_user_indicator
label=admin_settings_label.realm_enable_guest_user_indicator}}
</div>
</div>

<div id="org-stream-permissions" class="settings-subsection-parent">
<div class="subsection-header">
<h3>{{t "Stream permissions" }}</h3>
Expand Down Expand Up @@ -287,6 +255,55 @@
</div>
</div>

<div id="org-user-identity" class="settings-subsection-parent">
<div class="subsection-header">
<h3>{{t "User identity" }}</h3>
{{> settings_save_discard_widget section_name="user-identity" }}
</div>
<div class="inline-block organization-permissions-parent">
{{> settings_checkbox
setting_name="realm_name_changes_disabled"
prefix="id_"
is_checked=realm_name_changes_disabled
label=admin_settings_label.realm_name_changes_disabled}}

{{> settings_checkbox
setting_name="realm_email_changes_disabled"
prefix="id_"
is_checked=realm_email_changes_disabled
label=admin_settings_label.realm_email_changes_disabled}}

{{> settings_checkbox
setting_name="realm_avatar_changes_disabled"
prefix="id_"
is_checked=realm_avatar_changes_disabled
label=admin_settings_label.realm_avatar_changes_disabled}}

</div>
</div>

<div id="org-guest-settings" class="settings-subsection-parent">
<div class="subsection-header">
<h3>{{t "Guests" }}</h3>
{{> settings_save_discard_widget section_name="guest-settings" }}
</div>

<div class="inline-block organization-permissions-parent">
{{> settings_checkbox
setting_name="realm_enable_guest_user_indicator"
prefix="id_"
is_checked=realm_enable_guest_user_indicator
label=admin_settings_label.realm_enable_guest_user_indicator}}
</div>

{{#if development}}
{{> ../dropdown_widget_with_label
widget_name="realm_can_access_all_users_group"
label=(t 'Which users have access to all other users in the organization')
value_type="number"}}
{{/if}}
</div>

<div id="org-other-permissions" class="settings-subsection-parent">
<div class="subsection-header">
<h3>{{t "Other permissions" }}</h3>
Expand Down
27 changes: 26 additions & 1 deletion web/tests/user_groups.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -328,6 +328,7 @@ run_test("get_realm_user_groups_for_dropdown_list_widget", () => {
allow_everyone_group: true,
default_group_name: "role:administrators",
id_field_name: "can_remove_subscribers_group_id",
allowed_system_groups: [],
},
},
realm: {
Expand All @@ -339,11 +340,22 @@ run_test("get_realm_user_groups_for_dropdown_list_widget", () => {
allow_everyone_group: false,
default_group_name: "role:administrators",
id_field_name: "create_multiuse_invite_group_id",
allowed_system_groups: [],
},
can_access_all_users_group: {
require_system_group: true,
allow_internet_group: false,
allow_owners_group: false,
allow_nobody_group: false,
allow_everyone_group: true,
default_group_name: "role:everyone",
id_field_name: "can_access_all_users_group_id",
allowed_system_groups: ["role:everyone", "role:members"],
},
},
};

const expected_groups_list = [
let expected_groups_list = [
{name: "translated: Admins, moderators, members and guests", unique_id: 6},
{name: "translated: Admins, moderators and members", unique_id: 5},
{name: "translated: Admins, moderators and full members", unique_id: 7},
Expand Down Expand Up @@ -373,6 +385,19 @@ run_test("get_realm_user_groups_for_dropdown_list_widget", () => {
expected_groups_list,
);

expected_groups_list = [
{name: "translated: Admins, moderators, members and guests", unique_id: 6},
{name: "translated: Admins, moderators and members", unique_id: 5},
];

assert.deepEqual(
user_groups.get_realm_user_groups_for_dropdown_list_widget(
"can_access_all_users_group",
"realm",
),
expected_groups_list,
);

assert.throws(
() =>
user_groups.get_realm_user_groups_for_dropdown_list_widget("invalid_setting", "stream"),
Expand Down
2 changes: 1 addition & 1 deletion zerver/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -830,7 +830,7 @@ class Realm(models.Model): # type: ignore[django-manager-missing] # django-stub
allow_everyone_group=True,
default_group_name=SystemGroups.EVERYONE,
id_field_name="can_access_all_users_group_id",
allowed_system_groups=[SystemGroups.EVERYONE],
allowed_system_groups=[SystemGroups.EVERYONE, SystemGroups.MEMBERS],
),
)

Expand Down
24 changes: 24 additions & 0 deletions zerver/tests/test_realm.py
Original file line number Diff line number Diff line change
Expand Up @@ -1337,6 +1337,30 @@ def test_update_realm_properties(self) -> None:
with self.subTest(property=prop):
self.do_test_realm_permission_group_setting_update_api(prop)

def test_update_can_access_all_users_group_setting(self) -> None:
realm = get_realm("zulip")
self.login("iago")
members_group = UserGroup.objects.get(realm=realm, name=SystemGroups.MEMBERS)

with self.settings(DEVELOPMENT=False):
with self.assertRaises(AssertionError), self.assertLogs(
"django.request", "ERROR"
) as error_log:
self.client_patch("/json/realm", {"can_access_all_users_group": members_group.id})

self.assertTrue(
"ERROR:django.request:Internal Server Error: /json/realm" in error_log.output[0]
)
self.assertTrue("AssertionError" in error_log.output[0])

with self.settings(DEVELOPMENT=True):
result = self.client_patch(
"/json/realm", {"can_access_all_users_group": members_group.id}
)
self.assert_json_success(result)
realm = get_realm("zulip")
self.assertEqual(realm.can_access_all_users_group_id, members_group.id)

# Not in Realm.property_types because org_type has
# a unique RealmAuditLog event_type.
def test_update_realm_org_type(self) -> None:
Expand Down
5 changes: 5 additions & 0 deletions zerver/views/realm.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
from typing import Any, Dict, Mapping, Optional, Union

from django.conf import settings
from django.core.exceptions import ValidationError
from django.http import HttpRequest, HttpResponse
from django.shortcuts import render
Expand Down Expand Up @@ -232,6 +233,10 @@ def update_realm(
if enable_spectator_access:
realm.ensure_not_on_limited_plan()

if can_access_all_users_group_id is not None:
# Remove this when the feature is ready for production.
assert settings.DEVELOPMENT
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have used assert here instead of raising JsonableErrror since we do the same for demo orgs feature.


data: Dict[str, Any] = {}

message_content_delete_limit_seconds: Optional[int] = None
Expand Down