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

Refactor settings related code #5325

Merged
merged 19 commits into from
Jan 30, 2024
Merged

Refactor settings related code #5325

merged 19 commits into from
Jan 30, 2024

Conversation

Senen
Copy link
Member

@Senen Senen commented Nov 23, 2023

References

Before this change, two important things depend on the format of each key, where to render it in the administration panel and which kind of interface to use for each setting.

Following this strategy led us to a very complex code, very difficult to maintain or modify. So, we no longer want to depend on the setting key structure to decide how or where to render each setting.

Objectives

Do not rely on the setting key structure to decide where to render a setting or which interface to use. Now we are doing this in the view layer by using view components.

We hope customizing the settings or adding new ones will be easier for developers, too.

Visual Changes

Without changes.

Release notes

Installations using custom settings should add them to the new views when upgrading to the release 2.1.0.

@Senen Senen changed the title Refactor setting related code Refactor settings related code Nov 23, 2023
@Senen Senen marked this pull request as ready for review November 23, 2023 14:29
@Senen Senen moved this from Doing to Reviewing in Consul Democracy Nov 23, 2023
@Senen Senen added the 2.1 label Nov 23, 2023
app/models/setting.rb Outdated Show resolved Hide resolved
Copy link
Member

@javierm javierm left a comment

Choose a reason for hiding this comment

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

@Senen I've left some comments; let me know what you think!

P.S. I haven't reviewed the tests yet. Sorry! 🙏

.github/workflows/tests.yml Outdated Show resolved Hide resolved
app/components/admin/settings/table_component.html.erb Outdated Show resolved Hide resolved
app/controllers/admin/settings_controller.rb Outdated Show resolved Hide resolved
Copy link
Member

@javierm javierm left a comment

Choose a reason for hiding this comment

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

@Senen I've left a few comments. Let me know what you think!

spec/components/admin/settings/table_component_spec.rb Outdated Show resolved Hide resolved
app/views/layouts/_no_script.html.erb Show resolved Hide resolved
app/views/layouts/_no_script.html.erb Outdated Show resolved Hide resolved
app/components/admin/settings/row_component.rb Outdated Show resolved Hide resolved
app/components/admin/settings/table_component.rb Outdated Show resolved Hide resolved
app/controllers/admin/settings_controller.rb Outdated Show resolved Hide resolved
@javierm javierm moved this from Reviewing to Doing in Consul Democracy Jan 18, 2024
@Senen Senen force-pushed the settings_refactor branch 3 times, most recently from 18d3857 to 8f612c9 Compare January 19, 2024 11:14
@Senen Senen moved this from Doing to Reviewing in Consul Democracy Jan 19, 2024
Consul Democracy automation moved this from Reviewing to Testing Jan 19, 2024
Senen and others added 19 commits January 25, 2024 18:29
Now, with the same template we can render all kind of settings.
Instead of using a setting nested param `setting[:tab]`. We only need
the tab param when rendering settings in the administration section.

This change will make it easier rendering the correct tab after
updating settings.
Before this change, two important things depend on the format of each key,
where to render it in the administration panel and which kind of interface
to use for each setting. Following this strategy led us to a very complex
code, very difficult to maintain or modify. So, we do not want to depend
on the setting key structure anymore to decide how or where to render each
setting.

With this commit, we get rid of the key format-based rules. Now we render
each setting explicitly passing to it the type and the tab where it belongs.
Otherwise administrators cannot access the settings from other tabs.
Otherwise we get a AuthenticityToken exception.

I was nor able to write a test to verify this change.
@Senen Senen merged commit 0a50cb4 into master Jan 30, 2024
13 checks passed
Consul Democracy automation moved this from Testing to Release 2.1.0 Jan 30, 2024
@Senen Senen deleted the settings_refactor branch January 30, 2024 16:04
@javierm javierm removed the 2.1 label Jan 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants