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

DEV: Use site setting mandatory_values for chat allowed groups #26994

Merged
merged 1 commit into from
May 13, 2024

Conversation

martin-brennan
Copy link
Contributor

For both chat_allowed_groups and chat_message_flag_allowed_groups,
this commit removes the is_staff? guardian check, and instead
adds both moderators and admins auto groups as mandatory_values
to those settings, as part of an ongoing effort to do this for
group-based setting values.

@github-actions github-actions bot added chat PRs which include a change to Chat plugin i18n PRs which update English locale files or i18n related code labels May 13, 2024
For both `chat_allowed_groups` and `chat_message_flag_allowed_groups`,
this commit removes the `is_staff?` guardian check, and instead
adds both `moderators` and `admins` auto groups as `mandatory_values`
to those settings, as part of an ongoing effort to do this for
group-based setting values.
@martin-brennan martin-brennan force-pushed the dev/chat-mandatory-values-settings branch from 55e3fe5 to d65cd04 Compare May 13, 2024 01:55
@SamSaffron
Copy link
Member

I like this, but does this need any kind of migration? what happens to existing site settings here that don't have it set?

@martin-brennan
Copy link
Contributor Author

@SamSaffron No it's not necessary; the way this mandatory_values code works is that it injects these values no matter what, both in the get and the set for the site setting. So if you call SiteSetting.chat_allowed_groups, and the database already had group 99|76 in it, you would get back 1|2|99|76. And if you did SiteSetting.chat_allowed_groups = "54" we save the mandatory values too, so you will also get back SiteSetting.chat_allowed_groups => '1|2|54'

@SamSaffron
Copy link
Member

approved but just double checking, will this handle duplication?

@martin-brennan
Copy link
Contributor Author

will this handle duplication?

@SamSaffron as in, if I do SiteSetting.chat_allowed_groups = "54" multiple times it won't keep adding 1|2 on forever until we get 1|2|1|2|1|2|1|2|1|2? If so yes that was handled by Kris when he made this mandatory values thing :)

@martin-brennan martin-brennan merged commit 10b2715 into main May 13, 2024
16 checks passed
@martin-brennan martin-brennan deleted the dev/chat-mandatory-values-settings branch May 13, 2024 04:38
@discoursebot
Copy link

This pull request has been mentioned on Discourse Meta. There might be relevant details there:

https://meta.discourse.org/t/mandatory-fields-for-group-site-setting/307219/2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chat PRs which include a change to Chat plugin i18n PRs which update English locale files or i18n related code
4 participants