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: group_list site settings should store IDs instead of group names #7860

Merged
merged 4 commits into from
Jul 19, 2019

Conversation

romanrizzi
Copy link
Member

@romanrizzi romanrizzi commented Jul 4, 2019

Why do we need this?

Identifying groups by name is not reliable enough when you want to use defaults since they could have been renamed. IDs are a better alternative because they are unique.

After some discussion here, we decided that we should change core to simplify the plugin logic and use defaults safely.

@discoursebot
Copy link

You've signed the CLA, romanrizzi. Thank you! This pull request is ready for review.

@eviltrout
Copy link
Contributor

My first question is often: is there a context for this change? It always helps to tie it back to a post on one of our forums in case someone in the future wants to know why we did this.

Secondly, as you noticed in your plugin follow ups this is a breaking change. Did you consider adding a new site setting type, for example, group_id.

In that case we can use group ids for new settings without breaking old ones in plugins.

@nlalonde
Copy link
Member

nlalonde commented Jul 4, 2019

I'm guessing the reason for this is because group names are visible in client code.

@romanrizzi
Copy link
Member Author

I updated the plugin description with some context. Only discourse-adplugin and discourse-assign use this setting type. Maintaining two different setting types will be confusing since we should always use IDs to reference groups.

config/site_settings.yml Outdated Show resolved Hide resolved
@romanrizzi romanrizzi force-pushed the group_list_store_ids branch 5 times, most recently from 3700b0d to 08b3eeb Compare July 8, 2019 19:51
@romanrizzi romanrizzi force-pushed the group_list_store_ids branch 2 times, most recently from bd97f17 to c5857d3 Compare July 17, 2019 18:12
@romanrizzi romanrizzi merged commit eb26bee into discourse:master Jul 19, 2019
@romanrizzi romanrizzi deleted the group_list_store_ids branch July 19, 2019 18:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

5 participants