From 84505e2dfbc46ed027a658a289f654be43809b92 Mon Sep 17 00:00:00 2001
From: Dombi Attila <83396+dombesz@users.noreply.github.com>
Date: Tue, 8 Oct 2024 12:44:05 +0300
Subject: [PATCH] [#58144] Show email on the UI for users with permission

https://community.openproject.org/work_packages/58144
Remove hide_mail flag and replace with :view_user_email permission checks
---
 app/components/members/row_component.rb              |  3 +--
 .../users/profile/attributes_component.html.erb      |  2 +-
 app/components/users/profile/attributes_component.rb |  2 +-
 app/models/permitted_params.rb                       |  2 +-
 app/models/user_preference.rb                        |  8 --------
 .../user_preferences/set_attributes_service.rb       |  2 --
 app/views/common/feed.atom.builder                   |  4 +++-
 app/views/journals/index.atom.builder                |  3 ++-
 app/views/my/account.html.erb                        |  4 ----
 config/locales/en.yml                                |  1 -
 config/schemas/user_preferences.schema.json          |  3 ---
 ...2151949_remove_hide_mail_from_user_preferences.rb |  8 ++++++++
 .../user_preferences/user_preference_representer.rb  |  1 -
 .../user_preferences/params_contract_spec.rb         |  2 --
 .../user_preferences/update_contract_spec.rb         | 12 ------------
 spec/controllers/users_controller_spec.rb            |  2 --
 spec/factories/user_preference_factory.rb            |  1 -
 spec/features/users/my_spec.rb                       |  2 --
 18 files changed, 17 insertions(+), 45 deletions(-)
 create mode 100644 db/migrate/20241002151949_remove_hide_mail_from_user_preferences.rb

diff --git a/app/components/members/row_component.rb b/app/components/members/row_component.rb
index 5820e45f88a..a6139f23741 100644
--- a/app/components/members/row_component.rb
+++ b/app/components/members/row_component.rb
@@ -50,8 +50,7 @@ module Members
     end
 
     def mail
-      return unless user?
-      return if principal.pref.hide_mail
+      return unless user? && current_user.allowed_globally?(:view_user_email)
 
       link = mail_to(principal.mail)
 
diff --git a/app/components/users/profile/attributes_component.html.erb b/app/components/users/profile/attributes_component.html.erb
index f6dac59e953..6714b4d58aa 100644
--- a/app/components/users/profile/attributes_component.html.erb
+++ b/app/components/users/profile/attributes_component.html.erb
@@ -4,7 +4,7 @@
 
     component_wrapper do
       flex_layout do |details_container|
-        if @user.pref.can_expose_mail?
+        if current_user.allowed_globally?(:view_user_email)
           details_container.with_row(font_weight: :bold) do
             User.human_attribute_name(:mail)
           end
diff --git a/app/components/users/profile/attributes_component.rb b/app/components/users/profile/attributes_component.rb
index 5470dbc83d6..e7f7be0cf5a 100644
--- a/app/components/users/profile/attributes_component.rb
+++ b/app/components/users/profile/attributes_component.rb
@@ -40,7 +40,7 @@ module Users
       end
 
       def render?
-        @user.pref.can_expose_mail? || @user.visible_custom_field_values.any? { _1.value.present? }
+        current_user.allowed_globally?(:view_user_email) || @user.visible_custom_field_values.any? { _1.value.present? }
       end
 
       def visible_custom_fields
diff --git a/app/models/permitted_params.rb b/app/models/permitted_params.rb
index 944f962afaa..34d36be2cfc 100644
--- a/app/models/permitted_params.rb
+++ b/app/models/permitted_params.rb
@@ -263,7 +263,7 @@ class PermittedParams
   end
 
   def pref
-    params.fetch(:pref, {}).permit(:hide_mail, :time_zone, :theme,
+    params.fetch(:pref, {}).permit(:time_zone, :theme,
                                    :comments_sorting, :warn_on_leaving_unsaved,
                                    :auto_hide_popups)
   end
diff --git a/app/models/user_preference.rb b/app/models/user_preference.rb
index 220768c6a2d..041f930fe20 100644
--- a/app/models/user_preference.rb
+++ b/app/models/user_preference.rb
@@ -90,14 +90,6 @@ class UserPreference < ApplicationRecord
     settings.fetch(:diff_type, "inline")
   end
 
-  def hide_mail
-    settings.fetch(:hide_mail, true)
-  end
-
-  def can_expose_mail?
-    !hide_mail
-  end
-
   def auto_hide_popups=(value)
     settings[:auto_hide_popups] = to_boolean(value)
   end
diff --git a/app/services/user_preferences/set_attributes_service.rb b/app/services/user_preferences/set_attributes_service.rb
index ebbca864b57..61c32545e1d 100644
--- a/app/services/user_preferences/set_attributes_service.rb
+++ b/app/services/user_preferences/set_attributes_service.rb
@@ -29,8 +29,6 @@
 module UserPreferences
   class SetAttributesService < ::BaseServices::SetAttributes
     def set_attributes(params)
-      set_boolean_value(:hide_mail, params.delete(:hide_mail)) if params.key?(:hide_mail)
-
       params.each do |k, v|
         model[k] = v if model.supported_settings_method?(k)
       end
diff --git a/app/views/common/feed.atom.builder b/app/views/common/feed.atom.builder
index a95f90bd8c0..4a7aa3cd948 100644
--- a/app/views/common/feed.atom.builder
+++ b/app/views/common/feed.atom.builder
@@ -61,7 +61,9 @@ xml.feed "xmlns" => "http://www.w3.org/2005/Atom" do
       if author
         xml.author do
           xml.name(author)
-          xml.email(author.mail) if author.is_a?(User) && author.mail.present? && author.pref.can_expose_mail?
+          if author.is_a?(User) && author.mail.present? && User.current.allowed_globally?(:view_user_email)
+            xml.email(author.mail)
+          end
         end
       end
       xml.content "type" => "html" do
diff --git a/app/views/journals/index.atom.builder b/app/views/journals/index.atom.builder
index 9723e810f9d..7a89fd16d88 100644
--- a/app/views/journals/index.atom.builder
+++ b/app/views/journals/index.atom.builder
@@ -44,7 +44,8 @@ xml.feed "xmlns" => "http://www.w3.org/2005/Atom" do
       xml.updated change.created_at.xmlschema
       xml.author do
         xml.name change.user.name
-        xml.email(change.user.mail) if change.user.is_a?(User) && change.user.mail.present? && change.user.pref.can_expose_mail?
+        if change.user.is_a?(User) && change.user.mail.present? && User.current.allowed_globally?(:view_user_email)
+          xml.email(change.user.mail)
       end
       xml.content "type" => "html" do
         xml.text! "<ul>"
diff --git a/app/views/my/account.html.erb b/app/views/my/account.html.erb
index 3b67ce5144b..f804866228e 100644
--- a/app/views/my/account.html.erb
+++ b/app/views/my/account.html.erb
@@ -79,10 +79,6 @@ See COPYRIGHT and LICENSE files for more details.
       <% end %>
     </div>
 
-    <%= fields_for :pref, @user.pref, builder: TabularFormBuilder, lang: current_language do |pref_fields| %>
-      <div class="form--field"><%= pref_fields.check_box :hide_mail %></div>
-    <% end %>
-
     <%= call_hook(:view_my_account, user: @user, form: f) %>
 
     <%= render partial: "customizable/form",
diff --git a/config/locales/en.yml b/config/locales/en.yml
index f6c51f34f74..c47fb3c241e 100644
--- a/config/locales/en.yml
+++ b/config/locales/en.yml
@@ -877,7 +877,6 @@ en:
         consented_at: "Consented at"
       user_preference:
         comments_sorting: "Display comments"
-        hide_mail: "Hide my email address"
         impaired: "Accessibility mode"
         time_zone: "Time zone"
         auto_hide_popups: "Auto-hide success notifications"
diff --git a/config/schemas/user_preferences.schema.json b/config/schemas/user_preferences.schema.json
index b6dfb34596a..55f9af30c0b 100644
--- a/config/schemas/user_preferences.schema.json
+++ b/config/schemas/user_preferences.schema.json
@@ -6,9 +6,6 @@
             "type": "object",
             "additionalProperties": false,
             "properties": {
-                "hide_mail": {
-                    "type": "boolean"
-                },
                 "time_zone": {
                     "type": ["string", "null"]
                 },
diff --git a/db/migrate/20241002151949_remove_hide_mail_from_user_preferences.rb b/db/migrate/20241002151949_remove_hide_mail_from_user_preferences.rb
new file mode 100644
index 00000000000..3a9ccbca302
--- /dev/null
+++ b/db/migrate/20241002151949_remove_hide_mail_from_user_preferences.rb
@@ -0,0 +1,8 @@
+class RemoveHideMailFromUserPreferences < ActiveRecord::Migration[7.1]
+  def up
+    execute <<~SQL.squish
+      UPDATE user_preferences
+      SET settings =  settings - 'hide_mail'
+    SQL
+  end
+end
diff --git a/lib/api/v3/user_preferences/user_preference_representer.rb b/lib/api/v3/user_preferences/user_preference_representer.rb
index 9e4591e6903..b4113392c81 100644
--- a/lib/api/v3/user_preferences/user_preference_representer.rb
+++ b/lib/api/v3/user_preferences/user_preference_representer.rb
@@ -53,7 +53,6 @@ module API
           }
         end
 
-        property :hide_mail
         property :time_zone,
                  render_nil: true
 
diff --git a/spec/contracts/user_preferences/params_contract_spec.rb b/spec/contracts/user_preferences/params_contract_spec.rb
index d2fd1c17f63..43e642cf3d8 100644
--- a/spec/contracts/user_preferences/params_contract_spec.rb
+++ b/spec/contracts/user_preferences/params_contract_spec.rb
@@ -45,7 +45,6 @@ RSpec.describe UserPreferences::ParamsContract do
   end
   let(:params) do
     {
-      hide_mail: true,
       auto_hide_popups: true,
       comments_sorting: "desc",
       daily_reminders: {
@@ -204,7 +203,6 @@ RSpec.describe UserPreferences::ParamsContract do
     context "when notification_settings empty" do
       let(:params) do
         {
-          hide_mail: true,
           auto_hide_popups: true,
           comments_sorting: "desc",
           daily_reminders: {
diff --git a/spec/contracts/user_preferences/update_contract_spec.rb b/spec/contracts/user_preferences/update_contract_spec.rb
index d624d7208d8..0511b406ed8 100644
--- a/spec/contracts/user_preferences/update_contract_spec.rb
+++ b/spec/contracts/user_preferences/update_contract_spec.rb
@@ -41,7 +41,6 @@ RSpec.describe UserPreferences::UpdateContract do
   end
   let(:settings) do
     {
-      hide_mail: true,
       auto_hide_popups: true,
       comments_sorting: "desc",
       daily_reminders: {
@@ -107,16 +106,6 @@ RSpec.describe UserPreferences::UpdateContract do
       it_behaves_like "contract is valid"
     end
 
-    context "with a string for hide_mail" do
-      let(:settings) do
-        {
-          hide_mail: "yes please"
-        }
-      end
-
-      it_behaves_like "contract is invalid", hide_mail: :type_mismatch
-    end
-
     context "with a field within the daily_reminders having the wrong type" do
       let(:settings) do
         {
@@ -205,7 +194,6 @@ RSpec.describe UserPreferences::UpdateContract do
     context "without a time_zone" do
       let(:settings) do
         {
-          hide_mail: true,
           auto_hide_popups: true,
           comments_sorting: "desc",
           daily_reminders: {
diff --git a/spec/controllers/users_controller_spec.rb b/spec/controllers/users_controller_spec.rb
index 03e95140ee7..db68042e498 100644
--- a/spec/controllers/users_controller_spec.rb
+++ b/spec/controllers/users_controller_spec.rb
@@ -629,7 +629,6 @@ RSpec.describe UsersController do
             force_password_change: true
           },
           pref: {
-            hide_mail: "1",
             comments_sorting: "desc"
           }
         }
@@ -650,7 +649,6 @@ RSpec.describe UsersController do
         expect(some_user_from_db.firstname).to eq("Changed")
         expect(some_user_from_db.login).to eq("changed_login")
         expect(some_user_from_db.force_password_change).to be(true)
-        expect(some_user_from_db.pref[:hide_mail]).to be_truthy
         expect(some_user_from_db.pref[:comments_sorting]).to eq("desc")
       end
 
diff --git a/spec/factories/user_preference_factory.rb b/spec/factories/user_preference_factory.rb
index 3fc6913c2a5..b0976b5332e 100644
--- a/spec/factories/user_preference_factory.rb
+++ b/spec/factories/user_preference_factory.rb
@@ -29,7 +29,6 @@
 FactoryBot.define do
   factory :user_preference do
     user
-    hide_mail { true }
     transient do
       others { {} }
     end
diff --git a/spec/features/users/my_spec.rb b/spec/features/users/my_spec.rb
index 9788f628762..bd0bc5d23fe 100644
--- a/spec/features/users/my_spec.rb
+++ b/spec/features/users/my_spec.rb
@@ -458,14 +458,12 @@ RSpec.describe "my", :js, :with_cuprite do
         expect(page).to have_text(I18n.t("user.text_change_disabled_for_ldap_login"), count: 3)
 
         fill_in "Hobbies", with: "Ruby, DCS"
-        uncheck "pref[hide_mail]"
         click_on "Save"
 
         expect(page).to have_content I18n.t(:notice_account_updated)
 
         user.reload
         expect(user.custom_values.find_by(custom_field_id: string_cf).value).to eql "Ruby, DCS"
-        expect(user.pref.hide_mail).to be false
       end
     end