Merge pull request #3166 from manyfold3d/refactor-settings

Reorganise settings pages

app/controllers/settings_controller.rb

@@ -1,88 +1,18 @@
-require "i18n_data"
-
 class SettingsController < ApplicationController
-  before_action :get_user
   before_action :check_owner_permission
 
-  def show
-    @languages = [[t("settings.general_settings.interface_language.autodetect"), nil]].concat(
-      I18n.available_locales.map { |locale| [I18nData.languages(locale)[locale.upcase.to_s]&.capitalize, locale] }
-    )
-  end
-
   def update
-    # Save personal settings
-    update_general_settings(params[:general])
-    update_pagination_settings(params[:pagination])
-    update_renderer_settings(params[:renderer])
-    update_tag_cloud_settings(params[:tag_cloud])
-    update_problem_settings(params[:problems])
-    update_file_list_settings(params[:file_list])
-    @user.save!
     # Save site-wide settings if user is an admin
-    if current_user.is_administrator?
-      update_folder_settings(params[:folders])
-      update_tagging_settings(params[:model_tags])
-      update_multiuser_settings(params[:multiuser])
-      update_usage_settings(params[:usage])
-    end
-    redirect_to user_settings_path(@user), notice: t(".success")
+    update_folder_settings(params[:folders])
+    update_tagging_settings(params[:model_tags])
+    update_multiuser_settings(params[:multiuser])
+    update_analysis_settings(params[:analysis])
+    update_usage_settings(params[:usage])
+    redirect_to settings_path(@user), notice: t(".success")
   end
 
   private
 
-  def update_general_settings(settings)
-    return unless settings
-    @user.interface_language = settings[:interface_language].presence
-    @user.sensitive_content_handling = settings[:sensitive_content].presence
-  end
-
-  def update_pagination_settings(settings)
-    return unless settings
-    @user.pagination_settings = {
-      "models" => settings[:models] == "1",
-      "creators" => settings[:creators] == "1",
-      "collections" => settings[:collections] == "1",
-      "per_page" => settings[:per_page].to_i
-    }
-  end
-
-  def update_tag_cloud_settings(settings)
-    return unless settings
-    @user.tag_cloud_settings = {
-      "threshold" => settings[:threshold].to_i,
-      "heatmap" => settings[:heatmap] == "1",
-      "keypair" => settings[:keypair] == "1",
-      "sorting" => settings[:sorting]
-    }
-  end
-
-  def update_file_list_settings(settings)
-    return unless settings
-    @user.file_list_settings = {
-      "hide_presupported_versions" => settings[:hide_presupported_versions] == "1"
-    }
-  end
-
-  def update_renderer_settings(settings)
-    return unless settings
-    @user.renderer_settings = {
-      "grid_width" => settings[:grid_width].to_i,
-      "grid_depth" => settings[:grid_width].to_i, # Store width in both for now. See #834
-      "show_grid" => settings[:show_grid] == "1",
-      "enable_pan_zoom" => settings[:enable_pan_zoom] == "1",
-      "background_colour" => settings[:background_colour],
-      "object_colour" => settings[:object_colour],
-      "render_style" => settings[:render_style],
-      "auto_load_max_size" => settings[:auto_load_max_size].to_i
-    }
-  end
-
-  def update_problem_settings(settings)
-    return unless settings
-    @user.problem_settings = settings
-  end
-
   def update_folder_settings(settings)
     return unless settings
     SiteSettings.model_path_template = settings[:model_path_template].gsub(/^\//, "") # Remove leading slashes
@@ -114,12 +44,8 @@ def update_usage_settings(settings)
     (settings[:report] == "1") ? UsageReport.enable! : UsageReport.disable!
   end
 
-  def get_user
-    @user = User.find_param(params[:user_id])
-    authorize @user
-  end
-
   def check_owner_permission
-    render plain: "401 Unauthorized", status: :unauthorized unless @user == current_user || current_user.is_administrator?
+    render plain: "401 Unauthorized", status: :unauthorized unless current_user.is_administrator?
+    authorize current_user
   end
 end

app/controllers/users/registrations_controller.rb

@@ -4,7 +4,8 @@ class Users::RegistrationsController < Devise::RegistrationsController
   before_action :random_delay, only: [:create, :cancel]
   before_action :configure_sign_up_params, only: [:create]
   before_action :detect_if_first_use, only: [:edit, :update]
-  # before_action :configure_account_update_params, only: [:update]
+  before_action :load_languages, only: [:edit, :update]
+  before_action :configure_account_update_params, only: [:update]
   skip_before_action :check_for_first_use, only: [:edit, :update]
 
   # GET /resource/sign_up
@@ -66,6 +67,45 @@ def configure_sign_up_params
     devise_parameter_sanitizer.permit(:sign_up, keys: [:username])
   end
 
+  def configure_account_update_params
+    devise_parameter_sanitizer.permit(:account_update) do |user|
+      user.permit(
+        :email,
+        :password,
+        :password_confirmation,
+        :current_password,
+        :interface_language,
+        :sensitive_content_handling,
+        pagination_settings: [
+          :models,
+          :creators,
+          :collections,
+          :per_page
+        ],
+        tag_cloud_settings: [
+          :threshold,
+          :heatmap,
+          :keypair,
+          :sorting
+        ],
+        file_list_settings: [
+          :hide_presupported_versions
+        ],
+        renderer_settings: [
+          :grid_width,
+          :grid_depth,
+          :show_grid,
+          :enable_pan_zoom,
+          :background_colour,
+          :object_colour,
+          :render_style,
+          :auto_load_max_size
+        ],
+        problem_settings: Problem::CATEGORIES
+      )
+    end
+  end
+
   def detect_if_first_use
     if current_user.reset_password_token == "first_use"
       @first_use = true
@@ -82,4 +122,64 @@ def detect_if_first_use
   # def after_inactive_sign_up_path_for(resource)
   #   super(resource)
   # end
+
+  def pagination_json(settings)
+    return nil unless settings
+    {
+      "models" => settings[:models] == "1",
+      "creators" => settings[:creators] == "1",
+      "collections" => settings[:collections] == "1",
+      "per_page" => settings[:per_page].to_i
+    }
+  end
+
+  def tag_cloud_json(settings)
+    return nil unless settings
+    {
+      "threshold" => settings[:threshold].to_i,
+      "heatmap" => settings[:heatmap] == "1",
+      "keypair" => settings[:keypair] == "1"
+    }
+  end
+
+  def file_list_json(settings)
+    return nil unless settings
+    {
+      "hide_presupported_versions" => settings[:hide_presupported_versions] == "1"
+    }
+  end
+
+  def renderer_json(settings)
+    return nil unless settings
+    {
+      "grid_width" => settings[:grid_width].to_i,
+      "grid_depth" => settings[:grid_width].to_i, # Store width in both for now. See #834
+      "show_grid" => settings[:show_grid] == "1",
+      "enable_pan_zoom" => settings[:enable_pan_zoom] == "1",
+      "background_colour" => settings[:background_colour],
+      "object_colour" => settings[:object_colour],
+      "render_style" => settings[:render_style],
+      "auto_load_max_size" => settings[:auto_load_max_size].to_i
+    }
+  end
+
+  def load_languages
+    @languages = [[t("devise.registrations.general_settings.interface_language.autodetect"), nil]].concat(
+      I18n.available_locales.map { |locale| [I18nData.languages(locale)[locale.upcase.to_s]&.capitalize, locale] }
+    )
+  end
+
+  def update_resource(resource, data)
+    # Transform form data to crrect types
+    data[:pagination_settings] = pagination_json(data[:pagination_settings])
+    data[:renderer_settings] = renderer_json(data[:renderer_settings])
+    data[:tag_cloud_settings] = tag_cloud_json(data[:tag_cloud_settings])
+    data[:file_list_settings] = file_list_json(data[:file_list_settings])
+    # Require password if important details have changed
+    if data[:email] != resource.email || data[:password].present?
+      resource.update_with_password(data)
+    else
+      resource.update_without_password(data.except(:email, :password, :password_confirmation, :current_password))
+    end
+  end
 end

app/views/application/_navbar.html.erb

@@ -70,7 +70,7 @@
             </li>
           <% end %>
           <li class="nav-item">
-            <%= nav_link "sliders", t(".settings"), user_settings_path(current_user), text_style: "d-lg-none" %>
+            <%= nav_link "gear", t(".settings"), settings_path, text_style: "d-lg-none" %>
           </li>
         <% end %>
         <li class="nav-item">
@@ -81,7 +81,7 @@
         <%- if SiteSettings.multiuser_enabled? %>
           <%- if current_user %>
             <li class="nav-item">
-              <%= nav_link "person", t(".account"), edit_user_registration_path, text_style: "d-lg-none" %>
+              <%= nav_link "sliders", t(".account"), edit_user_registration_path, text_style: "d-lg-none" %>
             </li>
           <% end %>
           <li class="nav-item">

app/views/settings/_file_list_settings.html.erb → app/views/devise/registrations/_file_list_settings.html.erb

@@ -3,9 +3,9 @@
   <div class="card-body">
     <p class="lead"><%= t(".summary") %></p>
     <div class="row">
-      <%= form.label nil, t(".hide_presupported_versions.label"), for: "file_list[hide_presupported_versions]", class: "col col-form-label" %>
+      <%= form.label nil, t(".hide_presupported_versions.label"), for: "file_list_settings[hide_presupported_versions]", class: "col col-form-label" %>
       <div class="col form-check form-switch">
-        <%= form.check_box "file_list[hide_presupported_versions]", checked: @user.file_list_settings["hide_presupported_versions"], class: "form-check-input" %>
+        <%= form.check_box "file_list_settings[hide_presupported_versions]", checked: @user.file_list_settings["hide_presupported_versions"], class: "form-check-input" %>
       </div>
     </div>
   </div>

app/views/settings/_general_settings.html.erb → app/views/devise/registrations/_general_settings.html.erb

@@ -3,9 +3,9 @@
   <div class="card-body">
 
     <div class="row mb-2">
-      <%= form.label nil, t(".interface_language.label"), for: "general[interface_language]", class: "col col-form-label" %>
+      <%= form.label nil, t(".interface_language.label"), for: "interface_language", class: "col col-form-label" %>
       <div class="col">
-        <%= form.select "general[interface_language]",
+        <%= form.select "interface_language",
               @languages,
               {selected: @user.interface_language},
               {class: "form-select"} %>
@@ -14,9 +14,9 @@
     </div>
 
     <div class="row">
-      <%= form.label nil, t(".sensitive_content.label"), for: "general[sensitive_content]", class: "col col-form-label" %>
+      <%= form.label nil, t(".sensitive_content.label"), for: "sensitive_content_handling", class: "col col-form-label" %>
       <div class="col">
-        <%= form.select "general[sensitive_content]",
+        <%= form.select "sensitive_content_handling",
               [
                 [t(".sensitive_content.show"), nil],
                 [t(".sensitive_content.mask"), "mask"],

app/views/devise/registrations/_pagination_settings.html.erb

@@ -0,0 +1,34 @@
+<div class="card mb-2">
+  <h3 class="card-header"><%= t(".heading") %></h3>
+  <div class="card-body">
+
+    <p class='lead'>
+      <%= t(".description") %>
+    </p>
+
+    <div class="row">
+      <%= form.label nil, t(".models.label"), for: "pagination_settings[models]", class: "col col-form-label" %>
+      <div class="col form-check form-switch">
+        <%= form.check_box "pagination_settings[models]", checked: @user.pagination_settings["models"], class: "form-check-input" %>
+      </div>
+    </div>
+    <div class="row">
+      <%= form.label nil, t(".creators.label"), for: "pagination_settings[creators]", class: "col col-form-label" %>
+      <div class="col form-check form-switch">
+        <%= form.check_box "pagination_settings[creators]", checked: @user.pagination_settings["creators"], class: "form-check-input" %>
+      </div>
+    </div>
+    <div class="row">
+      <%= form.label nil, t(".collections.label"), for: "pagination_settings[collections]", class: "col col-form-label" %>
+      <div class="col form-check form-switch">
+        <%= form.check_box "pagination_settings[collections]", checked: @user.pagination_settings["collections"], class: "form-check-input" %>
+      </div>
+    </div>
+    <div class="row">
+      <%= form.label nil, t(".per_page.label"), for: "pagination_settings[per_page]", class: "col col-form-label" %>
+      <div class="col">
+        <%= form.number_field "pagination_settings[per_page]", value: @user.pagination_settings["per_page"], in: 1..100, class: "form-control" %>
+      </div>
+    </div>
+  </div>
+</div>

app/views/settings/_problem_settings.html.erb → app/views/devise/registrations/_problem_settings.html.erb

@@ -8,10 +8,10 @@
       <% next if Problem::DEFAULT_SEVERITIES[category].nil? # Skip deprecated categories; they don't appear in the defaults list %>
       <div class="row">
         <%= form.label nil, t("problems.categories.%{c}" % {c: category}),
-              for: "problems[#{category}]",
+              for: "problem_settings[#{category}]",
               class: "col col-form-label" %>
         <div class="col">
-          <%= form.select "problems[#{category}]",
+          <%= form.select "problem_settings[#{category}]",
                 Problem::SEVERITIES.map { |s| [t("problems.severities.%{s}" % {s: s}), s] },
                 {selected: @user.problem_severity(category)},
                 class: "form-select" %>

app/views/settings/_renderer_settings.html.erb → app/views/devise/registrations/_renderer_settings.html.erb

@@ -5,9 +5,9 @@
       <%= t(".description") %>
     </p>
     <div class="row">
-      <%= form.label nil, t(".auto_load_max_size.label"), for: "renderer[auto_load_max_size]", class: "col col-form-label" %>
+      <%= form.label nil, t(".auto_load_max_size.label"), for: "renderer_settings[auto_load_max_size]", class: "col col-form-label" %>
       <div class="col">
-        <%= form.select "renderer[auto_load_max_size]",
+        <%= form.select "renderer_settings[auto_load_max_size]",
               [
                 [t(".auto_load_max_size.never"), 0],
                 [t(".auto_load_max_size.under_2"), 2],
@@ -27,39 +27,39 @@
       </div>
     </div>
     <div class="row">
-      <%= form.label nil, t(".show_grid.label"), for: "renderer[show_grid]", class: "col col-form-label" %>
+      <%= form.label nil, t(".show_grid.label"), for: "renderer_settings[show_grid]", class: "col col-form-label" %>
       <div class="col form-check form-switch">
-        <%= form.check_box "renderer[show_grid]", checked: @user.renderer_settings["show_grid"], class: "form-check-input" %>
+        <%= form.check_box "renderer_settings[show_grid]", checked: @user.renderer_settings["show_grid"], class: "form-check-input" %>
       </div>
     </div>
     <div class="row">
-      <%= form.label nil, t(".grid_width.label"), for: "renderer[grid_width]", class: "col col-form-label" %>
+      <%= form.label nil, t(".grid_width.label"), for: "renderer_settings[grid_width]", class: "col col-form-label" %>
       <div class="col">
-        <%= form.number_field "renderer[grid_width]", value: @user.renderer_settings["grid_width"], class: "form-control" %>
+        <%= form.number_field "renderer_settings[grid_width]", value: @user.renderer_settings["grid_width"], class: "form-control" %>
       </div>
     </div>
     <div class="row">
-      <%= form.label nil, t(".enable_pan_zoom.label"), for: "renderer[enable_pan_zoom]", class: "col col-form-label" %>
+      <%= form.label nil, t(".enable_pan_zoom.label"), for: "renderer_settings[enable_pan_zoom]", class: "col col-form-label" %>
       <div class="col form-check form-switch">
-        <%= form.check_box "renderer[enable_pan_zoom]", checked: @user.renderer_settings["enable_pan_zoom"], class: "form-check-input" %>
+        <%= form.check_box "renderer_settings[enable_pan_zoom]", checked: @user.renderer_settings["enable_pan_zoom"], class: "form-check-input" %>
       </div>
     </div>
     <div class="row">
-      <%= form.label nil, t(".background_colour.label"), for: "renderer[background_colour]", class: "col col-form-label" %>
+      <%= form.label nil, t(".background_colour.label"), for: "renderer_settings[background_colour]", class: "col col-form-label" %>
       <div class="col">
-        <%= form.color_field "renderer[background_colour]", value: @user.renderer_settings["background_colour"], class: "form-control" %>
+        <%= form.color_field "renderer_settings[background_colour]", value: @user.renderer_settings["background_colour"], class: "form-control" %>
       </div>
     </div>
     <div class="row">
-      <%= form.label nil, t(".render_style.label"), for: "renderer[render_style]", class: "col col-form-label" %>
+      <%= form.label nil, t(".render_style.label"), for: "renderer_settings[render_style]", class: "col col-form-label" %>
       <div class="col">
-        <%= form.select "renderer[render_style]", [[t(".render_style.normals"), "normals"], [t(".render_style.lambert"), "lambert"]], {selected: @user.renderer_settings["render_style"]}, {class: "form-select"} %>
+        <%= form.select "renderer_settings[render_style]", [[t(".render_style.normals"), "normals"], [t(".render_style.lambert"), "lambert"]], {selected: @user.renderer_settings["render_style"]}, {class: "form-select"} %>
       </div>
     </div>
     <div class="row">
-      <%= form.label nil, t(".object_colour.label"), for: "renderer[object_colour]", class: "col col-form-label" %>
+      <%= form.label nil, t(".object_colour.label"), for: "renderer_settings[object_colour]", class: "col col-form-label" %>
       <div class="col">
-        <%= form.color_field "renderer[object_colour]", value: @user.renderer_settings["object_colour"], class: "form-control" %>
+        <%= form.color_field "renderer_settings[object_colour]", value: @user.renderer_settings["object_colour"], class: "form-control" %>
       </div>
     </div>
   </div>