Enum and readonly component settings by leio10 · Pull Request #6001 · decidim/decidim

leio10 · 2020-04-20T17:38:23Z

🎩 What? Why?

This PR adds the enum type of attribute for component settings. This settings are rendered as a collection checkboxes, and allow admins to choose between a set of options. I'm going to use this in #5993, but I created as a separated PR to simplify reviewing it.

During this work, I've found that this logic was already implemented for a proposals setting (amendment visibility), so I decided to reuse that logic as a generic feature.
Also, I've refactored the texts generation for the settings helpers and added the ability to make some settings read-only in specific conditions, to remove proposals logic from that file and thus reduce coupling between modules.

📌 Related Issues

Related to Budgets complex votings #5993

📋 Subtasks

Add CHANGELOG entry
Add tests
Add documentation regarding the feature

📷 Screenshots (optional)

mrcasals

Wohooo good job here! 😍

agustibr

🎉 😃 Great!

leio10 · 2020-04-21T08:24:03Z

@decidim/core this is ready to be reviewed. I added the PAM2020 because I need this for #5993.
Also, coverage % is not accurate (comments part has failed during upload), and probably #5991 should fix it. (edit: coverage is now ok, as it was executed again)

tramuntanal

Hi @leio10 good refactoring!

I only have doubts about how the disabled setting attribute is used in the proposals component.

tramuntanal · 2020-04-21T15:59:42Z

decidim-admin/app/commands/decidim/admin/update_component.rb

        @component.attributes["settings"]
      end
+
+      def restore_disabled_settings(step, settings)


Can you please complete the documentation and rename the step method?

Suggested change

def restore_disabled_settings(step, settings)

# Loads persisted settings from `ComponentManifest`.

# To be used ...

def restore_disabled_settings(settings_name, settings)

done, thanks for the feedback! I've refactored that part, that it was confusing and with a bug, I hope that it is much clear now, without the need of extra comments to be understood :)

tramuntanal · 2020-04-21T16:21:05Z

decidim-proposals/lib/decidim/proposals/component.rb

-    settings.attribute :participatory_texts_enabled, type: :boolean, default: false
+    settings.attribute :participatory_texts_enabled,
+                       type: :boolean, default: false,
+                       disabled: ->(context) { Decidim::Proposals::Proposal.where(component: context[:component]).any? }


Does this mean that the admin will only be able to enable participatory texts if there are some proposals for the current component?
We must take into account that participatory texts should be enabled for the admins to upload the text that will be converted into proposals.
Am I correct?

Mmm, it's just the other way around: this setting will only be allowed to be changed when there are no proposals in the component. 😄

if disabled is true then the attribute is disabled no? Then, if the lambda returns true (the component has proposals) it will be disabled. Am I missing something? 😅

Oh, I think that now I understand your concern.

Maybe is a naming issue, if I rename from disabled to readonly it would be clearer for you?

I mean, the idea is that admins can change the value for this setting until the component has any proposal. After that, the setting's value just keeps the same, doesn't change to false.

tramuntanal

sorry I just started a review when I was simply commenting the previous one

tramuntanal · 2020-04-23T14:52:19Z

decidim-admin/app/commands/decidim/admin/update_component.rb

+        @component.manifest.settings(settings_name).attributes
+                  .select { |_attribute, obj| obj.disabled?(component: @component) }


It would be great to have a enabled_attributes in SettingsManifest. Or event better, make the getter filter by only enalbed and have another all_attributes. What do you think?

Ok, I think that is definitely a bad idea to have a disabled option when a lot of settings are called ...._enabled. It seems that it could affect the value, while it only affects the ability of admins to modify it. If you are ok with the readonly name I'll change it that way.

tramuntanal · 2020-04-23T14:55:45Z

decidim-proposals/lib/decidim/proposals/component.rb

-    settings.attribute :participatory_texts_enabled, type: :boolean, default: false
+    settings.attribute :participatory_texts_enabled,
+                       type: :boolean, default: false,
+                       disabled: ->(context) { Decidim::Proposals::Proposal.where(component: context[:component]).any? }


if disabled is true then the attribute is disabled no? Then, if the lambda returns true (the component has proposals) it will be disabled. Am I missing something? 😅

Also refactors labels and help texts generation. With the new features is possible to remove all references to proposals in the helper.

* Participatory texts enabled attribute is disabled when there are proposals created on the component. * Amendments visibility attribute is converted to the enum type, using `Decidim.config.amendments_visibility_options` as the choices list

leio10 · 2020-04-30T09:26:01Z

@decidim/core finally everything is green! 🎉 🎉 Can we merge it?

tramuntanal

Excellent!!

tramuntanal · 2020-05-06T10:48:15Z

🎉 🎉 🎉

* feature/initiatives_search_fo_new_design: Updates changelog Harmonizes the design of initiatives search in FO New question type "Matrix" in questionnaires (decidim#5948) Add filter options to Timeline and Activity tabs (decidim#5845) Remove relations between user and spaces on destroy account command (decidim#6041) Explain how to initialize a custom oauth2 client provider (decidim#6055) Reenable main tests on Crowdin PRs (decidim#6076) Enum and readonly component settings (decidim#6001) New Crowdin translations (decidim#6066) Add missing notifications (decidim#5906)

leio10 added the project: PAM2020 Barcelona City Council contract label Apr 20, 2020

mrcasals previously approved these changes Apr 21, 2020

View reviewed changes

leio10 dismissed mrcasals’s stale review via 8c494de April 21, 2020 06:54

leio10 force-pushed the feat/enum-settings branch from 21f1e2b to 8c494de Compare April 21, 2020 06:54

leio10 requested a review from agustibr April 21, 2020 07:47

agustibr previously approved these changes Apr 21, 2020

View reviewed changes

leio10 dismissed agustibr’s stale review via a8cb1c4 April 21, 2020 09:27

tramuntanal added the in-review label Apr 21, 2020

tramuntanal reviewed Apr 21, 2020

View reviewed changes

leio10 force-pushed the feat/enum-settings branch 3 times, most recently from 67933ef to 2c68391 Compare April 22, 2020 17:18

tramuntanal reviewed Apr 23, 2020

View reviewed changes

leio10 force-pushed the feat/enum-settings branch 3 times, most recently from 45ce735 to c653161 Compare April 23, 2020 17:05

leio10 changed the title ~~Enum and disabled component settings~~ Enum and readonly component settings Apr 23, 2020

leio10 force-pushed the feat/enum-settings branch 3 times, most recently from 7212e84 to 864b387 Compare April 29, 2020 07:56

leio10 added 9 commits April 30, 2020 10:09

feat: adds support for enum attributes in component settings

665adeb

feat: adds support for disabled attributes in component settings

1737135

refactor: supports new attributes features in settings_helper

326dd44

Also refactors labels and help texts generation. With the new features is possible to remove all references to proposals in the helper.

feat: ensures that disabled attributes are not changed during updates

80f5ac2

refactor: removes unneeded validations for the proposals component

33e8182

test: fixes and completes settings_helper test

1fc0cc6

style: fixes lint and locales issues

455c4b5

test: fixes proposals tests

c45240d

leio10 added 8 commits April 30, 2020 10:11

test: adds coverage for disabled settings

9bf614c

fix(budgets): restore broken rule toggler

f0f530d

docs: adds changelog entry

ba39794

fix: fixes issues

1bb634b

docs: adds settings section to component documentation

0b0f3ec

fix: improves restoring disabled attributes logic, readability and tests

41a07aa

refactor: rename disabled settings feature as readonly settings

efd61a9

test: add missing settings manifest tests

4c84d78

leio10 force-pushed the feat/enum-settings branch from 864b387 to 4c84d78 Compare April 30, 2020 08:12

tramuntanal approved these changes May 6, 2020

View reviewed changes

tramuntanal merged commit efa9aae into develop May 6, 2020

tramuntanal deleted the feat/enum-settings branch May 6, 2020 10:47

-      def restore_disabled_settings(step, settings)
+      # Loads persisted settings from `ComponentManifest`.
+      # To be used ...
+      def restore_disabled_settings(settings_name, settings)

		@component.manifest.settings(settings_name).attributes
		.select { \|_attribute, obj\| obj.disabled?(component: @component) }

Uh oh!

Conversation

leio10 commented Apr 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🎩 What? Why?

📌 Related Issues

📋 Subtasks

📷 Screenshots (optional)

Uh oh!

mrcasals left a comment

Choose a reason for hiding this comment

Uh oh!

agustibr left a comment

Choose a reason for hiding this comment

Uh oh!

leio10 commented Apr 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tramuntanal left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tramuntanal left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leio10 commented Apr 30, 2020

Uh oh!

tramuntanal left a comment

Choose a reason for hiding this comment

Uh oh!

tramuntanal commented May 6, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

leio10 commented Apr 20, 2020 •

edited

Loading

leio10 commented Apr 21, 2020 •

edited

Loading