Multilingual organization name #12681

alecslupu · 2024-04-10T09:23:30Z

🎩 What? Why?

This PR adds support of internationalized organization name.

📌 Related Issues

Link your PR to an issue

Fixes Multilingual organization name #12286

Testing

Visit the organization settings of an admin
See there is only one field for the organization.
Apply patch, run migrations and see that you have a language selector for organization name
Fill in that with some results ( ex: European Commission + some translations )
Save and navigate website
Change the current locale and see that the organization name is being translated.
Check the admin panel.

📷 Screenshots

Please add screenshots of the changes you are proposing

♥️ Thank you!

github-actions

This pull request does not contain a valid label. Please add one of the following labels: ['type: feature', 'type: change', 'type: fix', 'type: removal', 'target: developer-experience', 'type: internal']

…n-org-name

andreslucena

I made a first round to give you feedback. Can you check it out please? Thanks

andreslucena · 2024-05-09T07:46:17Z

decidim-core/lib/decidim/core/engine.rb

@@ -706,6 +706,8 @@ class Engine < ::Rails::Engine
          # The time you want to timeout the user session without activity. After this
          # time the user will be asked for credentials again. Default is 30 minutes.
          config.timeout_in = Decidim.config.expire_session_after
+
+          config.parent_mailer = "Decidim::ApplicationMailer"


We already define this in

decidim/decidim-core/config/initializers/devise.rb

Line 33 in eb65155

config.parent_mailer = "Decidim::ApplicationMailer"

We should add a comment if this is really necessary here and also update the comment in line 688-689:

# These are moved from initializers/devise.rb because we need to run initializers folder before # setting these or Decidim.config variables have default values.

(We're not using the Decidim.config variables here)

decidim-core/lib/decidim/core/seeds.rb

andreslucena · 2024-05-09T08:16:27Z

decidim-core/app/helpers/decidim/organization_helper.rb

+    def organization_name(organization = current_organization)
+      translated_attribute(organization.name, organization)
+    end
+
+    def current_organization_name
+      organization_name(current_organization)
+    end


I see that sometimes we use current_organization_name, but other times we use organization_name(@organization) and we also do organization_name (without parameters). I'd prefer to have it consistent always, and do the same call always so it's consistent, and it means the same. I'm leaning to current_organization_name as it's easy to remember.

Would that be possible? Or there's something that I'm missing (i.e. current_organization is not always available). If that's the case, for instance for mailers I'd prefer to define the current_organization_name method in the ApplicationMailer and try this kind of strategies (this can go to a subsequent PR if you feel that this can be too big to tackle in this PR 😄 )

I have went and read all the files from above, and i would say they are correctly implemented.

All the erb Views that are being handled by controllers we are using current_organization_name

All the mailers do implement organization_name(....)

All the places where we need to display the organization name starting from a param is using organization_name(....)

Specs are using translated(organization_name)

Starting from your comment, I have changed a few places where this was fit to do.
Being a 150+ Files PR, i would not start changing the mailers so that we can use current_organization_name.

decidim-core/db/migrate/20240401192628_change_name_on_decidim_organizations.rb

andreslucena · 2024-05-09T08:24:20Z

decidim-core/spec/mailers/notification_digest_mailer_spec.rb

@@ -4,7 +4,7 @@

 module Decidim
  describe NotificationsDigestMailer do
-    let(:organization) { create(:organization, name: "O'Connor") }


These kinds of organizations names are to detect the apostrophe bug that happens usually. Let's leave it or maybe refactor it to a context (when the organization has an apostrophe in its name)

I understand this, but here we do not have any assertion to test the organization name is actually displayed like it should. Therefore, i do not see it relevant in this place

andreslucena · 2024-05-09T08:24:42Z

decidim-core/spec/mailers/notification_mailer_spec.rb

@@ -4,7 +4,7 @@

 module Decidim
  describe NotificationMailer do
-    let(:organization) { create(:organization, name: "O'Connor") }


I understand this, but here we do not have any assertion to test the organization name is actually displayed like it should. Therefore, i do not see it relevant in this place

andreslucena · 2024-05-09T08:38:38Z

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

@@ -6,7 +6,9 @@

 <%= decidim_form_for(@form) do |f| %>
  <div class="form__wrapper">
-    <%= f.text_field :name, autofocus: true %>
+    <div>
+      <%= f.translated :text_field, :name %>


I think the logic it's a bit tricky: as a new organization does not have defined any language, I think we should not show the language selector for new Organizations. It should be a text_field as it was before, and then in the edit form show the language tabs for the language enabled for this organization. Can you implement that please?

Actually here is a small issue. The Form used here is a RegisterOrganizationForm which is being extended from UpdateOrganizationForm. This UpdateOrganizationForm is actually having and using the translated attribute name. In order to implement your suggestion, we should create a new form that would have most of the elements that exist in UpdateOrganizationForm, so that we can add the requested behavior.

fixed in 0e9e1fb

…n-org-name

Co-authored-by: Andrés Pereira de Lucena <andreslucena@users.noreply.github.com>

…im into feature/i18n-org-name

alecslupu added 17 commits April 6, 2024 02:03

Initial commit

343218c

More mailers fixed

9a19fcd

Fix more specs

adf6c81

Lint

36919e5

Fix more specs

fa0a518

refactor

9c65c3a

Fix Specs

3f5802e

Fix specs

c152335

Fix more specs

2dad4c3

Fix specs

2aae6a2

lint

b6bec09

Remove helper methods

cc294cd

Add organization uniqueness validator

80619bb

mark name as translattable

9aa576b

Fix more specs

4243ba7

Fix specs

76a2836

Running lint

6c7bb92

github-actions bot requested changes Apr 10, 2024

View reviewed changes

alecslupu added the type: change PRs that implement a change for an existing feature label Apr 24, 2024

Merge branch 'develop' of github.com:decidim/decidim into feature/i18…

dd56cd1

…n-org-name

github-actions bot previously approved these changes Apr 24, 2024

View reviewed changes

alecslupu assigned andreslucena Apr 24, 2024

alecslupu requested a review from a team April 24, 2024 15:08

alecslupu dismissed github-actions’s stale review via dd56cd1 April 24, 2024 15:09

alecslupu marked this pull request as ready for review April 24, 2024 15:09

github-actions bot previously approved these changes Apr 24, 2024

View reviewed changes

Merge branch 'develop' of github.com:decidim/decidim into feature/i18…

bb1fb97

…n-org-name

probot-autolabeler bot added module: admin module: api module: blogs labels Apr 29, 2024

probot-autolabeler bot added module: budgets module: conferences module: core labels Apr 29, 2024

alecslupu added 2 commits May 1, 2024 18:03

Merge branch 'develop' of github.com:decidim/decidim into feature/i18…

61fd32c

…n-org-name

Merge branch 'develop' of github.com:decidim/decidim into feature/i18…

9590eb6

…n-org-name

alecslupu dismissed github-actions’s stale review via 9590eb6 May 6, 2024 08:04

github-actions bot previously approved these changes May 6, 2024

View reviewed changes

Fix specs

bf8a1ee

alecslupu dismissed github-actions’s stale review via bf8a1ee May 6, 2024 09:24

github-actions bot previously approved these changes May 6, 2024

View reviewed changes

andreslucena requested changes May 9, 2024

View reviewed changes

alecslupu and others added 2 commits May 9, 2024 19:14

Merge branch 'develop' of github.com:decidim/decidim into feature/i18…

4e075b3

…n-org-name

Apply suggestions from code review

4f4f87a

Co-authored-by: Andrés Pereira de Lucena <andreslucena@users.noreply.github.com>

alecslupu dismissed github-actions’s stale review via 4f4f87a May 9, 2024 16:40

github-actions bot previously approved these changes May 9, 2024

View reviewed changes

alecslupu added 2 commits May 9, 2024 19:40

use current_organization_name helper

3d3f143

Merge branch 'feature/i18n-org-name' of github.com:tremend-cofe/decid…

a2a4a87

…im into feature/i18n-org-name

alecslupu dismissed github-actions’s stale review via a2a4a87 May 9, 2024 16:41

github-actions bot previously approved these changes May 9, 2024

View reviewed changes

alecslupu added 2 commits May 9, 2024 19:55

Fis current_organization_name usage

308deaa

Apply review recommendations

0e9e1fb

alecslupu dismissed github-actions’s stale review via 0e9e1fb May 9, 2024 22:00

github-actions bot approved these changes May 9, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multilingual organization name #12681

Multilingual organization name #12681

alecslupu commented Apr 10, 2024 •

edited

github-actions bot left a comment

andreslucena left a comment

andreslucena May 9, 2024

andreslucena May 9, 2024

alecslupu May 9, 2024

andreslucena May 9, 2024

alecslupu May 9, 2024

andreslucena May 9, 2024

alecslupu May 9, 2024

andreslucena May 9, 2024

alecslupu May 9, 2024

alecslupu May 9, 2024

Multilingual organization name #12681

Are you sure you want to change the base?

Multilingual organization name #12681

Conversation

alecslupu commented Apr 10, 2024 • edited

🎩 What? Why?

📌 Related Issues

Testing

📷 Screenshots

github-actions bot left a comment

Choose a reason for hiding this comment

andreslucena left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alecslupu commented Apr 10, 2024 •

edited