Merge pull request #15583 from opf/feature/53672-hide-attachments-in-…

…files-tab

[#55089] [#55022] Improve File settings UI.

app/components/admin/attachments_settings_header_component.html.erb

@@ -30,10 +30,10 @@ See COPYRIGHT and LICENSE files for more details.
 <% helpers.html_title t(:label_administration), @title %>
 
 <%= render(Primer::OpenProject::PageHeader.new(border_bottom: 0)) do |header| %>
-  <% header.with_title { @title } %>
+  <% header.with_title { t(:"attributes.attachments") } %>
   <% header.with_breadcrumbs([{ href: admin_index_path, text: t("label_administration") },
                               { href: admin_settings_storages_path, text: t("project_module_storages") },
-                              @title]) %>
+                              t(:"attributes.attachments")]) %>
 <% end %>
 
 <%= render(Primer::Alpha::TabNav.new(label: "label")) do |component|

modules/storages/app/controllers/storages/admin/project_storages_controller.rb

@@ -26,40 +26,21 @@
 # See COPYRIGHT and LICENSE files for more details.
 #++
 
-# This controller manages the creation and deletion of ProjectStorage objects.
-# ProjectStorages belong to projects and indicate that the respective
-# Storage (i.e. a Nextcloud server) is enabled in the project.
-# Please see the standard Rails documentation on controllers:
-# https://guides.rubyonrails.org/action_controller_overview.html
-# Called by: Calls to the controller methods are initiated by user Web GUI
-# actions and mapped to this controller by storages/config/routes.rb.
 class Storages::Admin::ProjectStoragesController < Projects::SettingsController
-  # This is the resource handled in this controller.
-  # So the controller knows that the ID in params (URl) refer to instances of this model.
-  # This defines @object as the model instance.
   model_object Storages::ProjectStorage
 
   before_action :find_model_object, only: %i[oauth_access_grant edit update destroy destroy_info]
-  # No need to before_action :find_project_by_project_id as SettingsController already checks
-  # No need to check for before_action :authorize, as the SettingsController already checks this.
-
-  # This MenuController method defines the default menu item to be used (highlighted)
-  # when rendering the main menu in the left (part of the base layout).
-  # The menu item itself is registered in modules/storages/lib/open_project/storages/engine.rb
   menu_item :settings_project_storages
 
-  # Show a HTML page with the list of ProjectStorages
-  # Called by: Project -> Settings -> File Storages
-  def index
-    # Just get the list of ProjectStorages associated with the project
+  def external_file_storages
     @project_storages = Storages::ProjectStorage.where(project: @project).includes(:storage)
-    # Render the list storages using ViewComponents in the /app/components folder which defines
-    # the ways rows are rendered in a table layout.
-    render "/storages/project_settings/index"
+    render "/storages/project_settings/external_file_storages"
+  end
+
+  def attachments
+    render "/storages/project_settings/attachments"
   end
 
-  # Show a HTML page with a form in order to create a new ProjectStorage
-  # Called by: When a user clicks on the "+New" button in Project -> Settings -> File Storages
   def new
     @available_storages = available_storages
     project_folder_mode = project_folder_mode_from_params
@@ -74,8 +55,6 @@ def new
     render template: "/storages/project_settings/new"
   end
 
-  # Create a new ProjectStorage object.
-  # Called by: The new page above with form-data from that form.
   def create
     service_result = ::Storages::ProjectStorages::CreateService
                        .new(user: current_user)
@@ -98,11 +77,11 @@ def oauth_access_grant # rubocop:disable Metrics/AbcSize
                    .authorization_state(storage:, user: current_user)
 
     if auth_state == :connected
-      redirect_to(project_settings_project_storages_path)
+      redirect_to(external_file_storages_project_settings_project_storages_path)
     else
       nonce = SecureRandom.uuid
       cookies["oauth_state_#{nonce}"] = {
-        value: { href: project_settings_project_storages_url(project_id: @project_storage.project_id),
+        value: { href: external_file_storages_project_settings_project_storages_url(project_id: @project_storage.project_id),
                  storageId: @project_storage.storage_id }.to_json,
         expires: 1.hour
       }
@@ -111,13 +90,7 @@ def oauth_access_grant # rubocop:disable Metrics/AbcSize
     end
   end
 
-  # Edit page is very similar to new page, except that we don't need to set
-  # default attribute values because the object already exists
-  # Called by: Global app/config/routes.rb to serve Web page
   def edit
-    # Render existing ProjectStorage object
-    # @object was calculated in before_action :find_model_object (see comments above).
-    # @project_storage is used in the view in order to render the form for a new object
     @project_storage = @object
     @project_storage.project_folder_mode = project_folder_mode_from_params if project_folder_mode_from_params.present?
 
@@ -129,10 +102,6 @@ def edit
     render "/storages/project_settings/edit"
   end
 
-  # Update is similar to create above
-  # See also: create above
-  # See also: https://www.openproject.org/docs/development/concepts/contracted-services/
-  # Called by: Global app/config/routes.rb to serve Web page
   def update
     service_result = ::Storages::ProjectStorages::UpdateService
                        .new(user: current_user, model: @object)
@@ -148,19 +117,13 @@ def update
     end
   end
 
-  # Purpose: Destroy a ProjectStorage object
-  # Called by: By pressing a "Delete" icon in the Project's settings ProjectStorages page
-  # It redirects back to the list of ProjectStorages in the project
   def destroy
-    # The complex logic for deleting associated objects was moved into a service:
-    # https://dev.to/joker666/ruby-on-rails-pattern-service-objects-b19
     Storages::ProjectStorages::DeleteService
       .new(user: current_user, model: @object)
       .call
       .on_failure { |service_result| flash[:error] = service_result.errors.full_messages }
 
-    # Redirect the user to the URL of Projects -> Settings -> File Storages
-    redirect_to project_settings_project_storages_path
+    redirect_to external_file_storages_project_settings_project_storages_path
   end
 
   def destroy_info
@@ -171,10 +134,7 @@ def destroy_info
 
   private
 
-  # Define the list of permitted parameters for creating/updating a ProjectStorage.
-  # Called by create and update actions above.
   def permitted_storage_settings_params
-    # "params" is an instance of ActionController::Parameters
     params
       .require(:storages_project_storage)
       .permit("storage_id", "project_folder_mode", "project_folder_id")
@@ -197,7 +157,7 @@ def available_storages
 
   def redirect_to_project_storages_path_with_oauth_access_grant_confirmation
     if storage_oauth_access_granted?
-      redirect_to project_settings_project_storages_path
+      redirect_to external_file_storages_project_settings_project_storages_path
     else
       redirect_to_project_storages_path_with_nudge_modal
     end
@@ -210,7 +170,7 @@ def storage_oauth_access_granted?
 
   def redirect_to_project_storages_path_with_nudge_modal
     redirect_to(
-      project_settings_project_storages_path,
+      external_file_storages_project_settings_project_storages_path,
       flash: { modal: oauth_access_grant_nudge_modal }
     )
   end

modules/storages/app/views/storages/project_settings/_project_folder_form.html.erb

@@ -158,7 +158,7 @@ See COPYRIGHT and LICENSE files for more details.
     <%= spot_icon('checkmark') %>
     <%= content_tag :span, submit_button_label %>
   <% end %>
-  <%= link_to project_settings_project_storages_path(@project), class: 'button' do %>
+  <%= link_to external_file_storages_project_settings_project_storages_path(@project), class: 'button' do %>
     <%= spot_icon('cancel') %>
     <%= content_tag :span, t(:button_cancel) %>
   <% end %>

modules/storages/app/views/storages/project_settings/_storage_select_form.html.erb

@@ -48,7 +48,7 @@ See COPYRIGHT and LICENSE files for more details.
     <%= spot_icon('checkmark') %>
     <%= content_tag :span, submit_button_label %>
   <% end %>
-  <%= link_to project_settings_project_storages_path(@project), class: 'button' do %>
+  <%= link_to external_file_storages_project_settings_project_storages_path(@project), class: 'button' do %>
     <%= spot_icon('cancel') %>
     <%= content_tag :span, t(:button_cancel) %>
   <% end %>

modules/storages/app/views/storages/project_settings/index.html.erb → modules/storages/app/views/storages/project_settings/attachments.html.erb

@@ -47,42 +47,39 @@ See COPYRIGHT and LICENSE files for more details.
 end
 %>
 
-<%=  render(Primer::Alpha::TabPanels.new(label: "label", align: :left)) do |component|
-  component.with_tab(selected: true, id: "tab-1") do |tab|
+<%= render(Primer::Alpha::TabNav.new(label: nil)) do |component|
+  component.with_tab(selected: false, href: external_file_storages_project_settings_project_storages_path) do |tab|
     tab.with_text { t(:"external_file_storages") }
-    tab.with_panel {
-      render(::Storages::ProjectStorages::TableComponent.new(rows: @project_storages))
-    }
   end
-  component.with_tab(selected: false, id: "tab-2") do |tab|
+  component.with_tab(selected: true, href: attachments_project_settings_project_storages_path) do |tab|
     tab.with_text { t(:"attributes.attachments") }
-    tab.with_panel {
-      render(Primer::OpenProject::FlexLayout.new(border: true , border_radius: 2, p: 3)) do |l|
-        l.with_row(flex_layout: true, align_items: :center, justify_content: :space_between) do |row|
-          row.with_column do
-            render(Primer::Beta::Text.new(font_weight: :bold)) do
-              t("storages.show_attachments_toggle.label")
-            end
-          end
-          row.with_column do
-            checked = if @project.deactivate_work_package_attachments.nil?
-              Setting.show_work_package_attachments
-            else
-              !@project.deactivate_work_package_attachments
-            end
-            render(Primer::Alpha::ToggleSwitch.new(src: deactivate_work_package_attachments_project_path(@project),
-                                                   csrf_token: form_authenticity_token,
-                                                   status_label_position: :start,
-                                                   checked:))
+  end
+end
+%>
+
+<%= render(Primer::OpenProject::FlexLayout.new(p: 3)) do |l|
+      l.with_row(flex_layout: true, align_items: :center, justify_content: :space_between) do |row|
+        row.with_column do
+          render(Primer::Beta::Text.new(font_weight: :bold)) do
+            t("storages.show_attachments_toggle.label")
           end
         end
-        l.with_row do
-          render(Primer::Beta::Text.new(color: :muted)) do
-            t("storages.show_attachments_toggle.description")
+        row.with_column do
+          checked = if @project.deactivate_work_package_attachments.nil?
+            Setting.show_work_package_attachments
+          else
+            !@project.deactivate_work_package_attachments
           end
+          render(Primer::Alpha::ToggleSwitch.new(src: deactivate_work_package_attachments_project_path(@project),
+                                                  csrf_token: form_authenticity_token,
+                                                  status_label_position: :start,
+                                                  checked:))
         end
       end
-    }
-  end
-end
+      l.with_row do
+        render(Primer::Beta::Text.new(color: :muted)) do
+          t("storages.show_attachments_toggle.description")
+        end
+      end
+    end
 %>

modules/storages/app/views/storages/project_settings/destroy_info.html.erb

@@ -53,7 +53,7 @@ See COPYRIGHT and LICENSE files for more details.
            concat content_tag :i, '', class: 'button--icon icon-delete'
            concat content_tag :span, t(:button_delete), class: 'button--text'
            end %>
-      <%= link_to project_settings_project_storages_path(@project_storage_to_destroy.project),
+      <%= link_to external_file_storages_project_settings_project_storages_path(@project_storage_to_destroy.project),
           title: t(:button_cancel),
           class: 'button -with-icon icon-cancel' do %>
             <%= t(:button_cancel) %>

modules/storages/app/views/storages/project_settings/external_file_storages.html.erb

@@ -0,0 +1,60 @@
+<%#-- copyright
+OpenProject is an open source project management software.
+Copyright (C) 2012-2024 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-2013 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 COPYRIGHT and LICENSE files for more details.
+
+++#%>
+
+<% html_title t(:label_administration), t("project_module_storages") %>
+
+<%= render(Primer::OpenProject::PageHeader.new(border_bottom: 0)) do |header|
+  header.with_title { t("project_module_storages") }
+  header.with_breadcrumbs([{ href: project_settings_general_path, text: t("label_project_settings") },
+                           t("project_module_storages")])
+  header.with_action_button(tag: :a,
+                            href: new_project_settings_project_storage_path,
+                            scheme: :primary,
+                            mobile_icon: :plus,
+                            mobile_label: t(:'storages.label_storage'),
+                            size: :medium,
+                            aria: { label: t(:'storages.label_new_storage') },
+                            title: t(:'storages.label_new_storage')) do |button|
+    button.with_leading_visual_icon(icon: :plus)
+    t(:'storages.label_storage')
+  end
+end
+%>
+
+<%= render(Primer::Alpha::TabNav.new(label: nil)) do |component|
+  component.with_tab(selected: true, href: external_file_storages_project_settings_project_storages_path) do |tab|
+    tab.with_text { t(:"external_file_storages") }
+  end
+  component.with_tab(selected: false, href: attachments_project_settings_project_storages_path) do |tab|
+    tab.with_text { t(:"attributes.attachments") }
+  end
+end
+%>
+
+<%= render(::Storages::ProjectStorages::TableComponent.new(rows: @project_storages)) %>

modules/storages/app/views/storages/project_settings/project_storage_members/index.html.erb

@@ -31,7 +31,7 @@ See COPYRIGHT and LICENSE files for more details.
 <%
     breadcrumb_paths(
       ActionController::Base.helpers
-        .link_to(t("project_module_storages"), project_settings_project_storages_path(@project)),
+        .link_to(t("project_module_storages"), external_file_storages_project_settings_project_storages_path(@project)),
       default_breadcrumb
     )
 %>

modules/storages/config/routes.rb

@@ -65,7 +65,11 @@
 
   scope "projects/:project_id", as: "project" do
     namespace "settings" do
-      resources :project_storages, controller: "/storages/admin/project_storages", except: %i[show] do
+      resources :project_storages, controller: "/storages/admin/project_storages", except: %i[index show] do
+        collection do
+          get :external_file_storages
+          get :attachments
+        end
         member do
           get :oauth_access_grant
           # Destroy uses a get request to prompt the user before the actual DELETE request

modules/storages/lib/open_project/storages/engine.rb

@@ -129,9 +129,18 @@ def self.permissions
       # Independent of storages module (Disabling storages module does not revoke enabled permissions).
       project_module nil, order: 100 do
         permission :manage_storages_in_project,
-                   { "storages/admin/project_storages": %i[index members new
-                                                           edit update create oauth_access_grant
-                                                           destroy destroy_info set_permissions],
+                   { "storages/admin/project_storages": %i[external_file_storages
+                                                           attachments
+                                                           members
+                                                           index
+                                                           new
+                                                           edit
+                                                           update
+                                                           create
+                                                           oauth_access_grant
+                                                           destroy
+                                                           destroy_info
+                                                           set_permissions],
                      "storages/project_settings/project_storage_members": %i[index] },
                    permissible_on: :project,
                    dependencies: %i[]
@@ -159,9 +168,6 @@ def self.permissions
         end
       end
 
-      # Menu extensions
-      # Add a "storages_admin_settings" to the admin_menu with the specified link,
-      # condition ("if:"), caption and icon.
       menu :admin_menu,
            :files,
            { controller: "/storages/admin/storages", action: :index },
@@ -185,7 +191,7 @@ def self.permissions
 
       menu :project_menu,
            :settings_project_storages,
-           { controller: "/storages/admin/project_storages", action: "index" },
+           { controller: "/storages/admin/project_storages", action: "external_file_storages" },
            if: lambda { |project| User.current.allowed_in_project?(:manage_storages_in_project, project) },
            caption: :project_module_storages,
            parent: :settings