Apply review recommendations

decidim-core/app/models/decidim/organization.rb

@@ -64,7 +64,15 @@ class Organization < ApplicationRecord
 
     def unique_name
       base_query = new_record? ? Decidim::Organization.all : Decidim::Organization.where.not(id:).all
-      organization_names = base_query.pluck(:name).collect(&:values).flatten.map(&:downcase)
+
+      organization_names = []
+
+      base_query.pluck(:name).each do |value|
+        organization_names += value.except("machine_translations").values
+        organization_names += value.fetch("machine_translations", {}).values
+      end
+
+      organization_names = organization_names.map(&:downcase)
 
       name.each do |language, value|
         next if value.is_a?(Hash)

decidim-system/app/commands/decidim/system/register_organization.rb

@@ -49,7 +49,7 @@ def call
 
       def create_organization
         Decidim::Organization.create!(
-          name: form.name,
+          name: { form.default_locale => form.name },
           host: form.host,
           secondary_hosts: form.clean_secondary_hosts,
           reference_prefix: form.reference_prefix,

decidim-system/app/forms/decidim/system/organization_form.rb

@@ -0,0 +1,112 @@
+# frozen_string_literal: true
+
+require "decidim/translatable_attributes"
+
+module Decidim
+  module System
+    # A form object used to update organizations from the system dashboard.
+    #
+    class OrganizationForm < Form
+      include TranslatableAttributes
+      include JsonbAttributes
+
+      mimic :organization
+
+      attribute :host, String
+      attribute :secondary_hosts, String
+      attribute :force_users_to_authenticate_before_access_organization, Boolean
+      attribute :available_authorizations, Array[String]
+      attribute :users_registration_mode, String
+      attribute :default_locale, String
+
+      jsonb_attribute :smtp_settings, [
+        [:from, String],
+        [:from_email, String],
+        [:from_label, String],
+        [:user_name, String],
+        [:encrypted_password, String],
+        [:address, String],
+        [:port, Integer],
+        [:authentication, String],
+        [:enable_starttls_auto, Boolean]
+      ]
+
+      jsonb_attribute :content_security_policy, [
+        [:"default-src", String],
+        [:"img-src", String],
+        [:"media-src", String],
+        [:"script-src", String],
+        [:"style-src", String],
+        [:"frame-src", String],
+        [:"font-src", String],
+        [:"connect-src", String]
+      ]
+
+      attribute :password, String
+      attribute :file_upload_settings, FileUploadSettingsForm
+
+      OMNIATH_PROVIDERS_ATTRIBUTES = Decidim::OmniauthProvider.available.keys.map do |provider|
+        Rails.application.secrets.dig(:omniauth, provider).keys.map do |setting|
+          if setting == :enabled
+            ["omniauth_settings_#{provider}_enabled".to_sym, Boolean]
+          else
+            ["omniauth_settings_#{provider}_#{setting}".to_sym, String]
+          end
+        end
+      end.flatten(1)
+
+      jsonb_attribute :omniauth_settings, OMNIATH_PROVIDERS_ATTRIBUTES
+
+      validates :host, :users_registration_mode, presence: true
+      validate :validate_organization_uniqueness
+      validates :users_registration_mode, inclusion: { in: Decidim::Organization.users_registration_modes }
+
+      def map_model(model)
+        self.default_locale = model.default_locale
+        self.secondary_hosts = model.secondary_hosts.join("\n")
+        self.omniauth_settings = (model.omniauth_settings || {}).transform_values do |v|
+          Decidim::OmniauthProvider.value_defined?(v) ? Decidim::AttributeEncryptor.decrypt(v) : v
+        end
+        self.file_upload_settings = FileUploadSettingsForm.from_model(model.file_upload_settings)
+      end
+
+      def clean_secondary_hosts
+        return unless secondary_hosts
+
+        secondary_hosts.split("\n").map(&:chomp).select(&:present?)
+      end
+
+      def clean_available_authorizations
+        return unless available_authorizations
+
+        available_authorizations.select(&:present?)
+      end
+
+      def password
+        encrypted_password.nil? ? super : Decidim::AttributeEncryptor.decrypt(encrypted_password)
+      end
+
+      def encrypted_smtp_settings
+        smtp_settings["from"] = set_from
+
+        smtp_settings.merge(encrypted_password: Decidim::AttributeEncryptor.encrypt(password))
+      end
+
+      def set_from
+        return from_email if from_label.blank?
+
+        "#{from_label} <#{from_email}>"
+      end
+
+      def encrypted_omniauth_settings
+        omniauth_settings.transform_values do |v|
+          Decidim::OmniauthProvider.value_defined?(v) ? Decidim::AttributeEncryptor.encrypt(v) : v
+        end
+      end
+
+      private
+
+      def validate_organization_uniqueness; end
+    end
+  end
+end

decidim-system/app/forms/decidim/system/register_organization_form.rb

@@ -6,10 +6,12 @@ module Decidim
   module System
     # A form object used to create organizations from the system dashboard.
     #
-    class RegisterOrganizationForm < UpdateOrganizationForm
+    class RegisterOrganizationForm < OrganizationForm
       include JsonbAttributes
       mimic :organization
 
+      attribute :name, String
+
       attribute :organization_admin_email, String
       attribute :organization_admin_name, String
       attribute :available_locales, Array
@@ -19,10 +21,29 @@ class RegisterOrganizationForm < UpdateOrganizationForm
       attribute :force_users_to_authenticate_before_access_organization, Boolean
 
       validates :organization_admin_email, :organization_admin_name, :name, :host, :reference_prefix, :users_registration_mode, presence: true
+      validates :name, presence: true
       validates :available_locales, presence: true
       validates :default_locale, presence: true
       validates :default_locale, inclusion: { in: :available_locales }
       validates :users_registration_mode, inclusion: { in: Decidim::Organization.users_registration_modes }
+
+      private
+
+      def validate_organization_uniqueness
+        base_query = Decidim::Organization.pluck(:name)
+
+        organization_names = []
+
+        base_query.each do |value|
+          organization_names += value.except("machine_translations").values
+          organization_names += value.fetch("machine_translations", {}).values
+        end
+
+        organization_names = organization_names.map(&:downcase)
+
+        errors.add(:name, :taken) if organization_names.include?(name&.downcase)
+        errors.add(:host, :taken) if Decidim::Organization.where(host:).where.not(id:).exists?
+      end
     end
   end
 end

decidim-system/app/forms/decidim/system/update_organization_form.rb

@@ -4,107 +4,10 @@
 
 module Decidim
   module System
-    # A form object used to update organizations from the system dashboard.
-    #
-    class UpdateOrganizationForm < Form
-      include TranslatableAttributes
-      include JsonbAttributes
-
-      mimic :organization
-
+    class UpdateOrganizationForm < OrganizationForm
       translatable_attribute :name, String
-      attribute :host, String
-      attribute :secondary_hosts, String
-      attribute :force_users_to_authenticate_before_access_organization, Boolean
-      attribute :available_authorizations, Array[String]
-      attribute :users_registration_mode, String
-      attribute :default_locale, String
-
-      jsonb_attribute :smtp_settings, [
-        [:from, String],
-        [:from_email, String],
-        [:from_label, String],
-        [:user_name, String],
-        [:encrypted_password, String],
-        [:address, String],
-        [:port, Integer],
-        [:authentication, String],
-        [:enable_starttls_auto, Boolean]
-      ]
-
-      jsonb_attribute :content_security_policy, [
-        [:"default-src", String],
-        [:"img-src", String],
-        [:"media-src", String],
-        [:"script-src", String],
-        [:"style-src", String],
-        [:"frame-src", String],
-        [:"font-src", String],
-        [:"connect-src", String]
-      ]
-
-      attribute :password, String
-      attribute :file_upload_settings, FileUploadSettingsForm
-
-      OMNIATH_PROVIDERS_ATTRIBUTES = Decidim::OmniauthProvider.available.keys.map do |provider|
-        Rails.application.secrets.dig(:omniauth, provider).keys.map do |setting|
-          if setting == :enabled
-            ["omniauth_settings_#{provider}_enabled".to_sym, Boolean]
-          else
-            ["omniauth_settings_#{provider}_#{setting}".to_sym, String]
-          end
-        end
-      end.flatten(1)
-
-      jsonb_attribute :omniauth_settings, OMNIATH_PROVIDERS_ATTRIBUTES
 
-      validates :host, :users_registration_mode, presence: true
       validate :validate_organization_name_presence
-      validate :validate_organization_uniqueness
-      validates :users_registration_mode, inclusion: { in: Decidim::Organization.users_registration_modes }
-
-      def map_model(model)
-        self.default_locale = model.default_locale
-        self.secondary_hosts = model.secondary_hosts.join("\n")
-        self.omniauth_settings = (model.omniauth_settings || {}).transform_values do |v|
-          Decidim::OmniauthProvider.value_defined?(v) ? Decidim::AttributeEncryptor.decrypt(v) : v
-        end
-        self.file_upload_settings = FileUploadSettingsForm.from_model(model.file_upload_settings)
-      end
-
-      def clean_secondary_hosts
-        return unless secondary_hosts
-
-        secondary_hosts.split("\n").map(&:chomp).select(&:present?)
-      end
-
-      def clean_available_authorizations
-        return unless available_authorizations
-
-        available_authorizations.select(&:present?)
-      end
-
-      def password
-        encrypted_password.nil? ? super : Decidim::AttributeEncryptor.decrypt(encrypted_password)
-      end
-
-      def encrypted_smtp_settings
-        smtp_settings["from"] = set_from
-
-        smtp_settings.merge(encrypted_password: Decidim::AttributeEncryptor.encrypt(password))
-      end
-
-      def set_from
-        return from_email if from_label.blank?
-
-        "#{from_label} <#{from_email}>"
-      end
-
-      def encrypted_omniauth_settings
-        omniauth_settings.transform_values do |v|
-          Decidim::OmniauthProvider.value_defined?(v) ? Decidim::AttributeEncryptor.encrypt(v) : v
-        end
-      end
 
       private
 
@@ -115,7 +18,15 @@ def validate_organization_name_presence
 
       def validate_organization_uniqueness
         base_query = persisted? ? Decidim::Organization.where.not(id:).all : Decidim::Organization.all
-        organization_names = base_query.pluck(:name).collect(&:values).flatten.map(&:downcase)
+
+        organization_names = []
+
+        base_query.pluck(:name).each do |value|
+          organization_names += value.except("machine_translations").values
+          organization_names += value.fetch("machine_translations", {}).values
+        end
+
+        organization_names = organization_names.map(&:downcase)
 
         name.each do |language, value|
           next if value.is_a?(Hash)

decidim-system/app/views/decidim/system/organizations/edit.html.erb

@@ -4,7 +4,7 @@
   <h1 class="h1"><%= t ".title" %></h1>
 <% end %>
 
-<%= decidim_form_for(@form) do |f| %>
+<%= decidim_form_for(@form, url: organization_path(@organization)) do |f| %>
   <div class="form__wrapper">
     <div>
       <%= f.translated :text_field, :name, autofocus: true %>

decidim-system/app/views/decidim/system/organizations/new.html.erb

@@ -6,9 +6,7 @@
 
 <%= decidim_form_for(@form) do |f| %>
   <div class="form__wrapper">
-    <div>
-      <%= f.translated :text_field, :name %>
-    </div>
+    <%= f.text_field :name, autofocus: true %>
 
     <%= f.text_field :reference_prefix, help_text: t(".reference_prefix_hint") %>
 

decidim-system/spec/commands/decidim/system/register_organization_spec.rb

@@ -16,7 +16,7 @@ module System
           let(:from_label) { "Decide Gotham" }
           let(:params) do
             {
-              name: { en: "Gotham City" },
+              name: "Gotham City",
               host: "decide.gotham.gov",
               secondary_hosts: "foo.gotham.gov\r\n\r\nbar.gotham.gov",
               reference_prefix: "JKR",

decidim-system/spec/system/organizations_spec.rb

@@ -41,7 +41,7 @@
       end
 
       it "creates a new organization" do
-        fill_in_i18n :organization_name, "#organization-name-tabs", en: "Citizen Corp"
+        fill_in "Name", with: "Citizen Corp"
         fill_in "Host", with: "www.example.org"
         fill_in "Secondary hosts", with: "foo.example.org\n\rbar.example.org"
         fill_in "Reference prefix", with: "CCORP"
@@ -64,7 +64,7 @@
 
       context "with invalid data" do
         it "does not create an organization" do
-          fill_in_i18n :organization_name, "#organization-name-tabs", en: "Bad"
+          fill_in "Name", with: "Bad"
           click_on "Create organization & invite admin"
 
           expect(page).to have_content("There is an error in this field")
@@ -122,16 +122,16 @@
       it_behaves_like "form hiding advanced settings"
 
       it "edits the data" do
-        fill_in_i18n :organization_name, "#organization-name-tabs", en: "Citizens Rule!"
+        fill_in_i18n :update_organization_name, "#update_organization-name-tabs", en: "Citizens Rule!"
         fill_in "Host", with: "www.example.org"
         fill_in "Secondary hosts", with: "foobar.example.org\n\rbar.example.org"
         choose "Do not allow participants to register, but allow existing participants to login"
         check "Example authorization (Direct)"
 
         click_on "Show advanced settings"
-        check "organization_omniauth_settings_facebook_enabled"
-        fill_in "organization_omniauth_settings_facebook_app_id", with: "facebook-app-id"
-        fill_in "organization_omniauth_settings_facebook_app_secret", with: "facebook-app-secret"
+        check "update_organization_omniauth_settings_facebook_enabled"
+        fill_in "update_organization_omniauth_settings_facebook_app_id", with: "facebook-app-id"
+        fill_in "update_organization_omniauth_settings_facebook_app_secret", with: "facebook-app-secret"
 
         click_on "Save"
 
@@ -174,7 +174,9 @@
         )
 
         # Reload the UpdateOrganizationForm
+        Decidim::System.send(:remove_const, :OrganizationForm)
         Decidim::System.send(:remove_const, :UpdateOrganizationForm)
+        load "#{Decidim::System::Engine.root}/app/forms/decidim/system/organization_form.rb"
         load "#{Decidim::System::Engine.root}/app/forms/decidim/system/update_organization_form.rb"
 
         click_on "Organizations"
@@ -187,7 +189,9 @@
 
       after do
         # Reload the UpdateOrganizationForm
+        Decidim::System.send(:remove_const, :OrganizationForm)
         Decidim::System.send(:remove_const, :UpdateOrganizationForm)
+        load "#{Decidim::System::Engine.root}/app/forms/decidim/system/organization_form.rb"
         load "#{Decidim::System::Engine.root}/app/forms/decidim/system/update_organization_form.rb"
       end