extract email settings into own menu item

app/controllers/admin/incoming_mails_controller.rb

@@ -0,0 +1,45 @@
+#-- encoding: UTF-8
+
+#-- copyright
+# OpenProject is an open source project management software.
+# Copyright (C) 2012-2020 the OpenProject GmbH
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License version 3.
+#
+# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows:
+# Copyright (C) 2006-2017 Jean-Philippe Lang
+# Copyright (C) 2010-2013 the ChiliProject Team
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License
+# as published by the Free Software Foundation; either version 2
+# of the License, or (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+#
+# See docs/COPYRIGHT.rdoc for more details.
+#++
+
+class Admin::IncomingMailsController < ApplicationController
+  include Concerns::AdminSettingsUpdater
+
+  current_menu_item [:show] do
+    :incoming_mails
+  end
+
+  def default_breadcrumb
+    t(:label_incoming_emails)
+  end
+
+  def show_local_breadcrumb
+    true
+  end
+end

app/controllers/admin/mail_notifications_controller.rb

@@ -0,0 +1,50 @@
+#-- encoding: UTF-8
+
+#-- copyright
+# OpenProject is an open source project management software.
+# Copyright (C) 2012-2020 the OpenProject GmbH
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License version 3.
+#
+# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows:
+# Copyright (C) 2006-2017 Jean-Philippe Lang
+# Copyright (C) 2010-2013 the ChiliProject Team
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License
+# as published by the Free Software Foundation; either version 2
+# of the License, or (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+#
+# See docs/COPYRIGHT.rdoc for more details.
+#++
+
+class Admin::MailNotificationsController < ApplicationController
+  include Concerns::AdminSettingsUpdater
+
+  current_menu_item [:show] do
+    :mail_notifications
+  end
+
+  def show
+    @deliveries = ActionMailer::Base.perform_deliveries
+    @notifiables = Redmine::Notifiable.all
+  end
+
+  def default_breadcrumb
+    t(:'activerecord.attributes.user.mail_notification')
+  end
+
+  def show_local_breadcrumb
+    true
+  end
+end

app/controllers/admin_controller.rb

@@ -61,7 +61,7 @@ class AdminController < ApplicationController
       flash[:error] = I18n.t(:notice_email_error, value: Redmine::CodesetUtil.replace_invalid_utf8(e.message.dup))
     end
     ActionMailer::Base.raise_delivery_errors = raise_delivery_errors
-    redirect_to controller: '/settings', action: 'edit', tab: 'notifications'
+    redirect_to admin_mail_notifications_path
   end
 
   def force_user_language

app/controllers/concerns/admin_settings_updater.rb

@@ -0,0 +1,52 @@
+#-- encoding: UTF-8
+
+#-- copyright
+# OpenProject is an open source project management software.
+# Copyright (C) 2012-2020 the OpenProject GmbH
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License version 3.
+#
+# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows:
+# Copyright (C) 2006-2017 Jean-Philippe Lang
+# Copyright (C) 2010-2013 the ChiliProject Team
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License
+# as published by the Free Software Foundation; either version 2
+# of the License, or (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+#
+# See docs/COPYRIGHT.rdoc for more details.
+#++
+
+module Concerns
+  module AdminSettingsUpdater
+    extend ActiveSupport::Concern
+
+    included do
+      layout 'admin'
+
+      before_action :require_admin
+
+      def update
+        if params[:settings]
+          Settings::UpdateService
+            .new(user: current_user)
+            .call(settings: permitted_params.settings.to_h)
+
+          flash[:notice] = t(:notice_successful_update)
+          redirect_to action: 'show', tab: params[:tab]
+        end
+      end
+    end
+  end
+end

app/controllers/settings_controller.rb

@@ -1,4 +1,5 @@
 #-- encoding: UTF-8
+
 #-- copyright
 # OpenProject is an open source project management software.
 # Copyright (C) 2012-2020 the OpenProject GmbH
@@ -28,13 +29,11 @@
 #++
 
 class SettingsController < ApplicationController
-  layout 'admin'
+  include Concerns::AdminSettingsUpdater
 
   helper_method :gon
 
-  before_action :require_admin
-
-  current_menu_item [:index, :edit] do
+  current_menu_item [:show] do
     :settings
   end
 
@@ -45,29 +44,13 @@ class SettingsController < ApplicationController
     :settings
   end
 
-  def index
-    edit
-    render action: 'edit'
-  end
-
-  def edit
-    @notifiables = Redmine::Notifiable.all
-    if request.post? && params[:settings]
-      Settings::UpdateService
-        .new(user: current_user)
-        .call(settings: permitted_params.settings.to_h)
+  def show
+    @options = {}
+    @options[:user_format] = User::USER_FORMATS_STRUCTURE.keys.map { |f| [User.current.name(f), f.to_s] }
 
-      flash[:notice] = l(:notice_successful_update)
-      redirect_to action: 'edit', tab: params[:tab]
-    else
-      @options = {}
-      @options[:user_format] = User::USER_FORMATS_STRUCTURE.keys.map { |f| [User.current.name(f), f.to_s] }
-      @deliveries = ActionMailer::Base.perform_deliveries
+    @guessed_host = request.host_with_port.dup
 
-      @guessed_host = request.host_with_port.dup
-
-      @custom_style = CustomStyle.current || CustomStyle.new
-    end
+    @custom_style = CustomStyle.current || CustomStyle.new
   end
 
   def plugin
@@ -89,7 +72,7 @@ class SettingsController < ApplicationController
       plugin = Redmine::Plugin.find(params[:id])
       plugin.name
     else
-      l(:label_system_settings)
+      t(:label_system_settings)
     end
   end
 

app/helpers/settings_helper.rb

@@ -37,37 +37,25 @@ module SettingsHelper
       {
         name: 'general',
         partial: 'settings/general',
-        path: settings_path,
+        path: general_settings_path,
         label: :label_general
       },
       {
         name: 'display',
         partial: 'settings/display',
-        path: settings_path(tab: :display),
+        path: general_settings_path(tab: :display),
         label: :label_display
       },
       {
         name: 'projects',
         partial: 'settings/projects',
-        path: settings_path(tab: :projects),
+        path: general_settings_path(tab: :projects),
         label: :label_project_plural
       },
-      {
-        name: 'notifications',
-        partial: 'settings/notifications',
-        path: settings_path(tab: :notifications),
-        label: :label_mail_notification
-      },
-      {
-        name: 'mail_handler',
-        partial: 'settings/mail_handler',
-        path: settings_path(tab: :mail_handler),
-        label: :label_incoming_emails
-      },
       {
         name: 'repositories',
         partial: 'settings/repositories',
-        path: settings_path(tab: :repositories),
+        path: general_settings_path(tab: :repositories),
         label: :label_repository_plural
       }
     ]

app/views/admin/incoming_mails/show.html.erb

@@ -26,7 +26,12 @@ Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
 See docs/COPYRIGHT.rdoc for more details.
 
 ++#%>
-<%= styled_form_tag({action: 'edit', tab: 'mail_handler'}) do %>
+
+<% html_title t(:label_administration), t(:label_incoming_emails) -%>
+
+<%= toolbar title: t(:label_incoming_emails) %>
+
+<%= styled_form_tag(admin_incoming_mails_path, method: :patch) do %>
   <section class="form--section">
     <div class="form--field">
       <%= setting_text_area :mail_handler_body_delimiters, rows: 5, container_class: '-wide' %>
@@ -41,5 +46,6 @@ See docs/COPYRIGHT.rdoc for more details.
       <div class="form--field-instructions"><%= t('incoming_mails.ignore_filenames') %></div>
     </div>
   </section>
+
   <%= styled_button_tag t(:button_save), class: '-highlight -with-icon icon-checkmark' %>
 <% end %>

app/views/admin/mail_notifications/show.html.erb

@@ -26,14 +26,20 @@ Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
 See docs/COPYRIGHT.rdoc for more details.
 
 ++#%>
-<% if @deliveries %>
-  <%= styled_form_tag({action: 'edit', tab: 'notifications'}) do %>
+
+<% html_title t(:label_administration), t(:'activerecord.attributes.user.mail_notification') -%>
+
+<%= toolbar title: t(:'activerecord.attributes.user.mail_notification') %>
+
+<%= styled_form_tag(admin_mail_notifications_path, method: :patch) do %>
+  <% if @deliveries %>
     <section class="form--section">
       <div class="form--field"><%= setting_text_field :mail_from, size: 60, container_class: '-middle' %></div>
       <div class="form--field"><%= setting_check_box :bcc_recipients %></div>
       <div class="form--field"><%= setting_check_box :plain_text_mail %></div>
       <div class="form--field"><%= setting_select(:default_notification_option, User.valid_notification_options.collect {|o| [t(o.last), o.first.to_s]}, container_class: '-middle') %></div>
     </section>
+
     <fieldset id="notified_events" class="form--fieldset">
       <legend class="form--fieldset-legend">
         <%= t(:text_select_mail_notifications) %>
@@ -52,37 +58,42 @@ See docs/COPYRIGHT.rdoc for more details.
         </div>
       </div>
     </fieldset>
+
     <fieldset id="emails_decorators" class="form--fieldset"><legend class="form--fieldset-legend"><%= t(:setting_emails_header) %> & <%= t(:setting_emails_footer) %></legend>
       <%= rails_cell Settings::TextSettingCell, I18n.locale, name: "emails_header" %>
       <%= rails_cell Settings::TextSettingCell, I18n.locale, name: "emails_footer" %>
     </fieldset>
-    <%= content_tag :fieldset, id: "mail_configuration", class: "form--fieldset" do %>
-      <legend class="form--fieldset-legend"><%=t(:text_setup_mail_configuration)%></legend>
-      <div class="form--field"><%= setting_select(:email_delivery_method, [:smtp, :sendmail], id: "email_delivery_method_switch", container_class: '-xslim') %></div>
-      <div id="email_delivery_method_smtp" class="email_delivery_method_settings">
-        <div class="form--field"><%= setting_text_field :smtp_address, container_class: '-middle' %></div>
-        <div class="form--field"><%= setting_text_field :smtp_port, size: 6, container_class: '-xslim' %></div>
-        <div class="form--field"><%= setting_text_field :smtp_domain, container_class: '-middle' %></div>
-        <div class="form--field"><%= setting_select(:smtp_authentication, [:none, :plain, :login, :cram_md5], container_class: '-slim') %></div>
-        <div class="form--field"><%= setting_text_field :smtp_user_name, container_class: '-middle' %></div>
-        <div class="form--field"><%= setting_password :smtp_password, container_class: '-middle' %></div>
-        <div class="form--field"><%= setting_check_box :smtp_enable_starttls_auto %></div>
-        <div class="form--field"><%= setting_check_box :smtp_ssl %></div>
-      </div>
-      <div id="email_delivery_method_sendmail" class="email_delivery_method_settings">
-        <div class="form--field"><%= setting_text_field :sendmail_location %></div>
-      </div>
-    <% end unless OpenProject::Configuration['email_delivery_configuration'] == 'legacy' %>
+  <% else %>
+    <div class="nodata">
+      <%= simple_format(t(:text_email_delivery_not_configured)) %>
+    </div>
+  <% end %>
+
+  <%= content_tag :fieldset, id: "mail_configuration", class: "form--fieldset" do %>
+    <legend class="form--fieldset-legend"><%=t(:text_setup_mail_configuration)%></legend>
+    <div class="form--field"><%= setting_select(:email_delivery_method, [:smtp, :sendmail], id: "email_delivery_method_switch", container_class: '-xslim') %></div>
+    <div id="email_delivery_method_smtp" class="email_delivery_method_settings">
+      <div class="form--field"><%= setting_text_field :smtp_address, container_class: '-middle' %></div>
+      <div class="form--field"><%= setting_text_field :smtp_port, size: 6, container_class: '-xslim' %></div>
+      <div class="form--field"><%= setting_text_field :smtp_domain, container_class: '-middle' %></div>
+      <div class="form--field"><%= setting_select(:smtp_authentication, [:none, :plain, :login, :cram_md5], container_class: '-slim') %></div>
+      <div class="form--field"><%= setting_text_field :smtp_user_name, container_class: '-middle' %></div>
+      <div class="form--field"><%= setting_password :smtp_password, container_class: '-middle' %></div>
+      <div class="form--field"><%= setting_check_box :smtp_enable_starttls_auto %></div>
+      <div class="form--field"><%= setting_check_box :smtp_ssl %></div>
+    </div>
+    <div id="email_delivery_method_sendmail" class="email_delivery_method_settings">
+      <div class="form--field"><%= setting_text_field :sendmail_location %></div>
+    </div>
+  <% end unless OpenProject::Configuration['email_delivery_configuration'] == 'legacy' %>
+
+  <%= styled_button_tag t(:button_save), class: '-highlight -with-icon icon-checkmark' %>
 
-    <%= styled_button_tag t(:button_save), class: '-highlight -with-icon icon-checkmark' %>
+  <% if @deliveries %>
     <div style="float:none;display:inline-block;font-weight:bold;">
       <%= link_to t(:label_send_test_email),
                   { controller: '/admin', action: 'test_email' },
                   method: :post %>
     </div>
   <% end %>
-<% else %>
-  <div class="nodata">
-    <%= simple_format(t(:text_email_delivery_not_configured)) %>
-  </div>
 <% end %>

app/views/enumerations/destroy.html.erb

@@ -26,7 +26,7 @@ Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
 See docs/COPYRIGHT.rdoc for more details.
 
 ++#%>
-<%= breadcrumb_toolbar link_to(l(@enumeration.option_name), enumerations_path), @enumeration %>
+<%= breadcrumb_toolbar link_to(t(@enumeration.option_name), enumerations_path), @enumeration %>
 <%= styled_form_tag({}, method: :delete) do %>
   <section class="form--section">
     <p><strong><%= l(:text_enumeration_destroy_question, @enumeration.objects_count) %></strong></p>

app/views/homescreen/blocks/_administration.html.erb

@@ -37,7 +37,7 @@
 
   </li>
   <li>
-    <%= link_to l(:label_system_settings), settings_path,
+    <%= link_to l(:label_system_settings), general_settings_path,
                title: l(:label_system_settings) %>
   </li>
   <li>

app/views/settings/_display.html.erb

@@ -27,7 +27,7 @@ See docs/COPYRIGHT.rdoc for more details.
 
 ++#%>
 
-<%= styled_form_tag({action: 'edit', tab: 'display'}) do %>
+<%= styled_form_tag(general_settings_path(tab: 'display'), { method: :patch }) do %>
 
   <section class="form--section">
     <div class="form--field" id="setting_available_languages"><%= setting_multiselect :available_languages, all_lang_options_for_select(false) %></div>

app/views/settings/show.html.erb

@@ -27,8 +27,8 @@ See docs/COPYRIGHT.rdoc for more details.
 
 ++#%>
 
-<% html_title l(:label_administration), l(:label_system_settings) -%>
+<% html_title t(:label_administration), t(:label_system_settings) -%>
 
-<%= toolbar title: l(:label_system_settings) %>
+<%= toolbar title: t(:label_system_settings) %>
 
 <%= render_tabs administration_settings_tabs %>

config/initializers/menus.rb

@@ -210,10 +210,25 @@ Redmine::MenuManager.map :admin_menu do |menu|
             icon: 'icon2 icon-enumerations'
 
   menu.push :settings,
-            { controller: '/settings' },
+            { controller: '/settings', action: 'show' },
             caption: :label_system_settings,
             icon: 'icon2 icon-settings2'
 
+  menu.push :email,
+            { controller: '/admin/mail_notifications', action: 'show' },
+            caption: :'attributes.mail',
+            icon: 'icon2 icon-mail1'
+
+  menu.push :mail_notifications,
+            { controller: '/admin/mail_notifications', action: 'show' },
+            caption: :'activerecord.attributes.user.mail_notification',
+            parent: :email
+
+  menu.push :incoming_mails,
+            { controller: '/admin/incoming_mails', action: 'show' },
+            caption: :label_incoming_emails,
+            parent: :email
+
   menu.push :authentication,
             { controller: '/authentication', action: 'authentication_settings' },
             caption: :label_authentication,

config/locales/en.yml

@@ -2327,7 +2327,7 @@ en:
   text_destroy_with_associated: "There are additional objects assossociated with the work package(s) that are to be deleted. Those objects are of the following types:"
   text_destroy_what_to_do: "What do you want to do?"
   text_diff_truncated: "... This diff was truncated because it exceeds the maximum size that can be displayed."
-  text_email_delivery_not_configured: "Email delivery is not configured, and notifications are disabled.\nConfigure your SMTP server in config/configuration.yml and restart the application to enable them."
+  text_email_delivery_not_configured: "Email delivery is not configured, and notifications are disabled.\nConfigure your SMTP server to enable them."
   text_enumeration_category_reassign_to: "Reassign them to this value:"
   text_enumeration_destroy_question: "%{count} objects are assigned to this value."
   text_file_repository_writable: "Attachments directory writable"

config/routes.rb

@@ -384,11 +384,16 @@ OpenProject::Application.routes.draw do
     end
   end
 
-  # We should fix this crappy routing (split up and rename controller methods)
-  get '/settings' => 'settings#index'
-  scope 'settings', controller: 'settings' do
-    match 'edit', action: 'edit', via: %i[get post]
-    match 'plugin/:id', action: 'plugin', via: %i[get post]
+  namespace :admin do
+    resource :incoming_mails, only: %i[show update]
+    resource :mail_notifications, only: %i[show update]
+  end
+
+  resource :settings, as: :general_settings, only: %i(update edit show) do
+    # We should fix this crappy routing (split up and rename controller methods)
+    collection do
+      match 'plugin/:id', action: 'plugin', via: %i[get post]
+    end
   end
 
   resource :workflows, only: %i[edit update show] do

spec/features/admin/test_mail_notification_spec.rb

@@ -35,7 +35,7 @@ describe 'Test mail notification', type: :feature do
 
   before do
     login_as(admin)
-    visit settings_path(tab: :notifications)
+    visit general_settings_path(tab: :notifications)
   end
 
   it 'shows the correct message on errors in test notification (Regression #28226)' do

spec/routing/admin/incoming_mails_spec.rb

@@ -0,0 +1,39 @@
+#-- copyright
+# OpenProject is an open source project management software.
+# Copyright (C) 2012-2020 the OpenProject GmbH
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License version 3.
+#
+# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows:
+# Copyright (C) 2006-2017 Jean-Philippe Lang
+# Copyright (C) 2010-2013 the ChiliProject Team
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License
+# as published by the Free Software Foundation; either version 2
+# of the License, or (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+#
+# See docs/COPYRIGHT.rdoc for more details.
+#++
+
+require 'spec_helper'
+
+describe 'admin incoming_mails routes', type: :routing do
+  it do
+    expect(get('admin/incoming_mails')).to route_to('admin/incoming_mails#show')
+  end
+
+  it do
+    expect(patch('admin/incoming_mails')).to route_to('admin/incoming_mails#update')
+  end
+end

spec/routing/admin/mail_notifications_spec.rb

@@ -0,0 +1,39 @@
+#-- copyright
+# OpenProject is an open source project management software.
+# Copyright (C) 2012-2020 the OpenProject GmbH
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License version 3.
+#
+# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows:
+# Copyright (C) 2006-2017 Jean-Philippe Lang
+# Copyright (C) 2010-2013 the ChiliProject Team
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License
+# as published by the Free Software Foundation; either version 2
+# of the License, or (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+#
+# See docs/COPYRIGHT.rdoc for more details.
+#++
+
+require 'spec_helper'
+
+describe 'admin mail_notifications routes', type: :routing do
+  it do
+    expect(get('admin/mail_notifications')).to route_to('admin/mail_notifications#show')
+  end
+
+  it do
+    expect(patch('admin/mail_notifications')).to route_to('admin/mail_notifications#update')
+  end
+end

spec/routing/settings_spec.rb

@@ -29,9 +29,15 @@
 require 'spec_helper'
 
 describe 'settings routes', type: :routing do
-  it { expect(get('/settings')).to route_to('settings#index') }
-  it { expect(get('/settings/edit')).to route_to('settings#edit') }
-  it { expect(post('/settings/edit')).to route_to('settings#edit') }
+  it { expect(get('/settings')).to route_to('settings#show') }
+
+  it do
+    expect(get('/settings/edit')).to route_to('settings#edit')
+  end
+
+  it do
+    expect(patch('/settings')).to route_to('settings#update')
+  end
 
   it do
     expect(get('/settings/plugin/abc')).to route_to(controller: 'settings',