Fix project bound global note template deletion (agileware-jp#65)

* fix: enabling global note template deletion

* test: add test case for global note template

* chore: fix spell

* fix: use Migration version 5.2

* fix: remove unnecessary `!`

* Update spec/models/global_note_template_spec.rb

Co-authored-by: Yuya.Nishida. <[email protected]>

* Update spec/models/global_note_template_spec.rb

Co-authored-by: Yuya.Nishida. <[email protected]>

* fix: display accurate strings

* fix: hide disabled note templates

* fix: switch the display depending on whether the user has editing permissions for the note template

* feat: add tests for template edit form visibility

* feat: display appropriate validation attribute names

* fix tests

---------

Co-authored-by: Yuya.Nishida. <[email protected]>

app/models/concerns/attribute_name_mapper.rb

@@ -0,0 +1,21 @@
+# frozen_string_literal: true
+
+# A mixin to display the appropriate field name when displaying a validation error message.
+module AttributeNameMapper
+  extend ActiveSupport::Concern
+
+  module ClassMethods
+    def attribute_map
+      {}
+    end
+
+    def human_attribute_name(attr, options = {})
+      map = ActiveSupport::HashWithIndifferentAccess.new(attribute_map)
+      if map.has_key?(attr)
+        l(map[attr])
+      else
+        super(attr, options)
+      end
+    end
+  end
+end

app/models/global_issue_template.rb

@@ -3,6 +3,7 @@
 class GlobalIssueTemplate < ActiveRecord::Base
   include Redmine::SafeAttributes
   include IssueTemplateCommon
+  include AttributeNameMapper
   validates :title, uniqueness: { scope: :tracker_id }
   has_and_belongs_to_many :projects
 
@@ -51,5 +52,12 @@ def get_templates_for_project_tracker(project_id, tracker_id = nil)
                          .enabled
                          .sorted
     end
+
+    def attribute_map
+      {
+        description: :issue_description,
+        title: :issue_template_name,
+      }
+    end
   end
 end

app/models/global_note_template.rb

@@ -2,6 +2,7 @@
 
 class GlobalNoteTemplate < ActiveRecord::Base
   include Redmine::SafeAttributes
+  include AttributeNameMapper
 
   # author and project should be stable.
   safe_attributes 'name',
@@ -19,8 +20,7 @@ class GlobalNoteTemplate < ActiveRecord::Base
   belongs_to :author, class_name: 'User', inverse_of: false, foreign_key: 'author_id'
   belongs_to :tracker
 
-  has_many :global_note_template_projects, dependent: :nullify
-  has_many :projects, through: :global_note_template_projects
+  has_and_belongs_to_many :projects
 
   has_many :global_note_visible_roles, dependent: :nullify
   has_many :roles, through: :global_note_visible_roles
@@ -131,7 +131,7 @@ def visible_note_templates_condition(user_id:, project_id:, tracker_id:)
 
       # return uniq ids
       ids = open_ids | role_ids
-      GlobalNoteTemplate.where(id: ids).includes(:global_note_visible_roles)
+      GlobalNoteTemplate.where(id: ids).enabled.includes(:global_note_visible_roles)
     end
 
     def get_templates_for_project_tracker(project_id, tracker_id = nil)
@@ -148,5 +148,13 @@ def plugin_setting
     def apply_all_projects?
       plugin_setting['apply_global_template_to_all_projects'].to_s == 'true'
     end
+
+    def attribute_map
+      {
+        description: :label_comment,
+        name: :issue_template_name,
+        role_ids: :field_template_visibility,
+      }
+    end
   end
 end

app/models/issue_template.rb

@@ -3,6 +3,7 @@
 class IssueTemplate < ActiveRecord::Base
   include Redmine::SafeAttributes
   include IssueTemplateCommon
+  include AttributeNameMapper
   belongs_to :project
   validates :project_id, presence: true
   validates :title, uniqueness: { scope: :project_id }
@@ -70,5 +71,12 @@ def get_templates_for_project_tracker(project_id, tracker_id = nil)
                    .enabled
                    .sorted
     end
+
+    def attribute_map
+      {
+        description: :issue_description,
+        title: :issue_template_name,
+      }
+    end
   end
 end

app/models/note_template.rb

@@ -2,6 +2,7 @@
 
 class NoteTemplate < ActiveRecord::Base
   include Redmine::SafeAttributes
+  include AttributeNameMapper
 
   class NoteTemplateError < StandardError; end
 
@@ -127,7 +128,14 @@ def visible_note_templates_condition(user_id:, project_id:, tracker_id:)
 
       # return uniq ids
       ids = open_ids | mine_ids | role_ids
-      NoteTemplate.where(id: ids).includes(:note_visible_roles)
+      NoteTemplate.where(id: ids).enabled.includes(:note_visible_roles)
+    end
+
+    def attribute_map
+      {
+        description: :label_comment,
+        name: :issue_template_name,
+      }
     end
   end
 end

app/views/global_note_templates/index.html.erb

@@ -30,7 +30,7 @@
           <th><%= l(:label_preview) %></th>
           <th class='hideable'><%= l(:field_tracker) %></th>
           <th class='hideable'><%= l(:field_author) %></th>
-          <th class='hideable'><%= l(:field_updated_at) %></th>
+          <th class='hideable'><%= l(:field_updated_on) %></th>
           <th><%= l(:label_enabled) %></th>
           <th><%=l(:button_sort)%></th>
         </tr>

app/views/note_templates/show.html.erb

@@ -41,10 +41,10 @@
     <% end %>
     <hr/>
   </div>
-<% end %>
+<% else %>
 
 
-<div class='box tabular box-white'>
+<div id='view-note_template' class='box tabular box-white'>
   <p>
     <label><%= l(:issue_template_name) %></label>
     <%= h note_template.name %>
@@ -85,6 +85,6 @@
   </p>
 
 </div>
+<% end %>
 
 <%= render partial: 'common/template_links' %>
-

config/locales/ja.yml

@@ -38,6 +38,7 @@ ja:
   label_should_replaced_help_message: "オプションをチェックすると、テンプレートを適用する際に、入力済みの本文とタイトルを上書きします。（デフォルトはOFFです）"
   global_issue_templates: "グローバル チケットテンプレート"
   no_issue_templates_for_this_redmine: "このRedmineにはグローバルチケットテンプレートは定義されていません。"
+  no_note_templates_for_this_redmine: "このRedmineにはグローバルコメント用テンプレートは定義されていません。"
   only_admin_can_associate_global_template: "このプロジェクトでグローバルチケットテンプレートを利用するには、Redmine管理者権限が必要です。"
   text_no_tracker_enabled_for_global: "テンプレートはトラッカー毎に設定します。\nトラッカーの設定をお願いします。"
   display_and_filter_issue_templates_in_dialog: "テンプレートの内容を確認"

db/migrate/20230330055341_change_global_note_template_projects_table_name.rb

@@ -0,0 +1,9 @@
+class ChangeGlobalNoteTemplateProjectsTableName < ActiveRecord::Migration[5.2]
+  def up
+    rename_table :global_note_template_projects, :global_note_templates_projects
+  end
+
+  def down
+    rename_table :global_note_templates_projects, :global_note_template_projects
+  end
+end

spec/features/issue_template_spec.rb

@@ -97,7 +97,7 @@
       end
 
       scenario 'create template failed' do
-        expect(error_message).to have_content('Title cannot be blank')
+        expect(error_message).to have_content('Template name cannot be blank')
       end
     end
 

spec/features/update_issue_spec.rb

@@ -132,6 +132,31 @@
         end
       end
     end
+
+  end
+
+  context 'Have disabled note templates' do
+    background do
+      FactoryBot.rewind_sequences
+      FactoryBot.create_list(
+        :note_template, 3, project_id: project.id, tracker_id: tracker.id, visibility: :open, enabled: false
+      )
+    end
+
+    scenario 'Disabled Templates would not to be shown.' do
+      template_list = NoteTemplate.visible_note_templates_condition(
+        user_id: user.id, project_id: project.id, tracker_id: tracker.id
+      )
+      expect(template_list).to be_empty
+
+      visit_update_issue(user)
+      issue = Issue.last
+      visit "/issues/#{issue.id}"
+
+      page.find('#content > div:nth-child(1) > a.icon.icon-edit').click
+      sleep(0.2)
+      expect(page).to have_no_selector('a#link_template_issue_notes_dialog')
+    end
   end
 
   context 'Have global note template' do
@@ -222,6 +247,29 @@
     end
   end
 
+  context 'Have disabled global note templates' do
+    before do
+      Setting.send 'plugin_redmine_issue_templates=', 'apply_global_template_to_all_projects' => 'true'
+      create_list(
+        :global_note_template, 3, tracker_id: tracker.id, name: 'Global Note Template name', visibility: 2, enabled: false
+      )
+    end
+
+    scenario 'Disabled global note templates would not be show' do
+      template_list = GlobalNoteTemplate.visible_note_templates_condition(
+        user_id: user.id, project_id: project.id, tracker_id: tracker.id
+      )
+      expect(template_list).to be_empty
+
+      visit_update_issue(user)
+      issue = Issue.last
+      visit "/issues/#{issue.id}"
+      page.find('#content > div:nth-child(1) > a.icon.icon-edit').click
+      sleep(0.2)
+      expect(page).to have_no_selector('a#link_template_issue_notes_dialog')
+    end
+  end
+
   private
 
   def visit_update_issue(user)

spec/features/update_template_spec.rb

@@ -0,0 +1,88 @@
+# frozen_string_literal: true
+
+require_relative '../spec_helper'
+require_relative '../rails_helper'
+require_relative '../support/login_helper'
+
+RSpec.configure do |c|
+  c.include LoginHelper
+end
+
+feature 'Update template', js: true do
+  given(:user) { FactoryBot.create(:user, :password_same_login, login: 'test-manager', language: 'en', admin: false) }
+  given(:project) { FactoryBot.create(:project_with_enabled_modules) }
+  given(:tracker) { FactoryBot.create(:tracker, :with_default_status) }
+  given(:role) { FactoryBot.create(:role, :manager_role) }
+  given(:status) { IssueStatus.create(name: 'open', is_closed: false) }
+  given(:expected_note_description) { 'Note Template desctiption' }
+  given!(:template) {
+    NoteTemplate.create(project_id: project.id, tracker_id: tracker.id,
+      name: 'Note Template name', description: expected_note_description, enabled: true)
+  }
+
+  background(:all) do
+    Redmine::Plugin.register(:redmine_issue_templates) do
+      settings partial: 'settings/redmine_issue_templates',
+               default: { 'apply_global_template_to_all_projects' => 'false', 'apply_template_when_edit_issue' => 'true' }
+    end
+  end
+
+  background do
+    project.trackers << tracker
+
+    priority = IssuePriority.create(
+      name: 'Low',
+      position: 1, is_default: false, type: 'IssuePriority', active: true, project_id: nil, parent_id: nil,
+      position_name: 'lowest'
+    )
+
+    member = Member.new(project: project, user_id: user.id)
+    member.member_roles << MemberRole.new(role: role)
+    member.save
+
+    Issue.create(project_id: project.id, tracker_id: tracker.id,
+                 author_id: user.id,
+                 priority: priority,
+                 subject: 'test_create',
+                 status_id: status.id,
+                 description: 'IssueTest#test_create')
+  end
+
+  context 'Have show_issue_template permission' do
+
+    background do
+      assign_template_priv(role, add_permission: :show_issue_templates)
+    end
+
+    scenario 'Cannot edit the template, only view it' do
+      visit_log_user(user)
+      visit "/projects/#{project.identifier}/note_templates/#{template.id}"
+      sleep(0.2)
+      expect(page).to have_no_selector('div#edit-note_template')
+      expect(page).to have_selector('div#view-note_template')
+    end
+  end
+
+  context 'Have edit_issue_template permission' do
+
+    background do
+      assign_template_priv(role, add_permission: :edit_issue_templates)
+      assign_template_priv(role, add_permission: :show_issue_templates)
+    end
+
+    scenario 'Can edit the template, and view it' do
+      visit_log_user(user)
+      visit "/projects/#{project.identifier}/note_templates/#{template.id}"
+      sleep(0.2)
+      expect(page).to have_selector('div#edit-note_template')
+      expect(page).to have_no_selector('div#view-note_template')
+    end
+  end
+
+  private
+
+  def visit_log_user(user)
+    user.update_attribute(:admin, false)
+    log_user(user.login, user.login)
+  end
+end

spec/models/global_note_template_spec.rb

@@ -23,4 +23,13 @@
       expect(GlobalNoteTemplate.sorted).to eq [note_template2, note_template3, note_template]
     end
   end
+
+  it 'can be deleted even though some projects bound' do
+    note_template_with_project = create(:global_note_template, tracker_id: tracker.id, position: 4, enabled: false)
+    note_template_with_project.projects << create(:project) << create(:project) << create(:project)
+
+    expect(GlobalNoteTemplate.where(id: note_template_with_project.id)).not_to be_empty
+    note_template_with_project.destroy
+    expect(GlobalNoteTemplate.where(id: note_template_with_project.id)).to be_empty
+  end
 end

test/functional/global_issue_templates_controller_test.rb

@@ -59,7 +59,7 @@ def test_update_template_with_empty_title
     # render :show
     assert_select 'h2.global_issue_template', "#{l(:global_issue_templates)}: ##{global_issue_template.id}"
     # Error message should be displayed.
-    assert_select 'div#errorExplanation', { count: 1, text: /Title cannot be blank/ }, @response.body.to_s
+    assert_select 'div#errorExplanation', { count: 1, text: /Template name cannot be blank/ }, @response.body.to_s
   end
 
   def test_destroy_template
@@ -107,7 +107,7 @@ def test_create_template_fail
     # render :new
     assert_select 'h2', text: "#{l(:issue_templates)} / #{l(:button_add)}"
     # Error message should be displayed.
-    assert_select 'div#errorExplanation', { count: 1, text: /Title cannot be blank/ }, @response.body.to_s
+    assert_select 'div#errorExplanation', { count: 1, text: /Template name cannot be blank/ }, @response.body.to_s
   end
 
   def test_preview_template

test/functional/issue_templates_controller_test.rb

@@ -110,7 +110,7 @@ def test_create_template_with_empty_title
     # render :new
     assert_select 'h2', text: "#{l(:issue_templates)} / #{l(:button_add)}"
     # Error message should be displayed.
-    assert_select 'div#errorExplanation', { count: 1, text: /Title cannot be blank/ }, @response.body.to_s
+    assert_select 'div#errorExplanation', { count: 1, text: /Template name cannot be blank/ }, @response.body.to_s
   end
 
   def test_preview_template
@@ -150,7 +150,7 @@ def test_update_template_with_empty_title
     assert_select 'h2.issue_template', "#{l(:issue_templates)}: #2"
     assert_select 'div#edit-issue_template'
     # Error message should be displayed.
-    assert_select 'div#errorExplanation', { count: 1, text: /Title cannot be blank/ }, @response.body.to_s
+    assert_select 'div#errorExplanation', { count: 1, text: /Template name cannot be blank/ }, @response.body.to_s
   end
 
   def test_delete_template_fail_if_enabled