Add a setting for assemblies to toggle the visibility of the organization chart#6040
Add a setting for assemblies to toggle the visibility of the organization chart#6040microstudi merged 25 commits intodevelopfrom
Conversation
cf75e6b to
7a81272
Compare
mrcasals
left a comment
There was a problem hiding this comment.
Code look good to me so far! Let's check what's going on eith the asets file 😄
agustibr
left a comment
There was a problem hiding this comment.
great work! please check the comments 😃
| module Assemblies | ||
| module Admin | ||
| # A command with all the business logic when updating assemblies | ||
| # settings in the system. |
There was a problem hiding this comment.
As this command is performed from the /admin, not the /system, I suggest to change the doc to admin:
| # settings in the system. | |
| # settings in the admin area. |
| private | ||
|
|
||
| def current_assemblies_settings | ||
| @current_assemblies_settings = Decidim::AssembliesSetting.find_or_create_by!(decidim_organization_id: current_organization.id) |
There was a problem hiding this comment.
Here I'd suggest to memoize, so the cached result is returned when the method is called the second time:
| @current_assemblies_settings = Decidim::AssembliesSetting.find_or_create_by!(decidim_organization_id: current_organization.id) | |
| @current_assemblies_settings ||= Decidim::AssembliesSetting.find_or_create_by!(decidim_organization_id: current_organization.id) |
| module Admin | ||
| # A form object used to create assembly setting from the admin dashboard. | ||
| class AssembliesSettingForm < Form | ||
| include TranslatableAttributes |
There was a problem hiding this comment.
Not sure if TranslatableAttributes is needed as the attribute is Boolean, can you check it?
| end | ||
|
|
||
| def current_assemblies_settings | ||
| @current_assemblies_settings = Decidim::AssembliesSetting.find_or_create_by(decidim_organization_id: current_organization.id) |
There was a problem hiding this comment.
As commented in the other controller, I'd suggest to memoize this one also:
| @current_assemblies_settings = Decidim::AssembliesSetting.find_or_create_by(decidim_organization_id: current_organization.id) | |
| @current_assemblies_settings ||= Decidim::AssembliesSetting.find_or_create_by(decidim_organization_id: current_organization.id) |
decidim-assemblies/spec/controllers/assemblies_settings_controller_spec.rb
Outdated
Show resolved
Hide resolved
|
@decidim/core can you please review the PR? |
|
@decidim/core this has been approved by @decidim/product, can you review it please? 😄 |
microstudi
left a comment
There was a problem hiding this comment.
Hi @anaghavl the code looks good to me.
I have only some concerns regarding the semantics of the words used in classes and methods.
I think we should use the concept "assembly settings" instead of "assemblies setting". I think that because these are setting that apply to each assembly independently (which is different that assemblies_types for instance).
What do you think, do you agree?
| @@ -1,2 +1,3 @@ | |||
| //= link_directory ../images/decidim/assemblies | |||
| //= link_directory ../javascripts/decidim/assemblies | |||
| //= require decidim/assemblies/orgchart.js | |||
There was a problem hiding this comment.
Is this necessary? shouldn't the previous line include it automatically?
There was a problem hiding this comment.
yes, that makes sense. Removed.
decidim-assemblies/app/presenters/decidim/assemblies/admin_log/assemblies_setting_presenter.rb
Show resolved
Hide resolved
decidim-assemblies/app/presenters/decidim/assemblies/admin_log/assemblies_setting_presenter.rb
Show resolved
Hide resolved
decidim-assemblies/app/views/decidim/assemblies/assemblies/index.html.erb
Show resolved
Hide resolved
|
@anaghavl if you can check that line in the manifest and resolve conflicts, we're done here. Thanks! |
🎩 What? Why?
📌 Related Issues
📋 Subtasks
CHANGELOGentry📷 Screenshots (optional)