Merge branch 'fix-file-tmpls-return-same-content-for-different-tmpls' into 'master'

Fix file templates return same content for different templates

See merge request gitlab-org/gitlab!52332

app/assets/javascripts/blob/file_template_selector.js

@@ -66,6 +66,8 @@ export default class FileTemplateSelector {
   reportSelectionName(options) {
     const opts = options;
     opts.query = options.selectedObj.name;
+    opts.data = options.selectedObj;
+    opts.data.source_template_project_id = options.selectedObj.project_id;
 
     this.reportSelection(opts);
   }

app/assets/javascripts/blob/template_selectors/license_selector.js

@@ -30,6 +30,7 @@ export default class BlobLicenseSelector extends FileTemplateSelector {
         const data = {
           project: this.$dropdown.data('project'),
           fullname: this.$dropdown.data('fullname'),
+          source_template_project_id: query.project_id,
         };
 
         this.reportSelection({

app/assets/javascripts/deprecated_jquery_dropdown/gl_dropdown.js

@@ -437,6 +437,7 @@ export class GitLabDropdown {
       groupName = el.data('group');
       if (groupName) {
         selectedIndex = el.data('index');
+        this.selectedIndex = selectedIndex;
         selectedObject = this.renderedData[groupName][selectedIndex];
       } else {
         selectedIndex = el.closest('li').index();

app/assets/javascripts/issue_show/components/app.vue

@@ -132,6 +132,10 @@ export default {
       type: String,
       required: true,
     },
+    projectId: {
+      type: Number,
+      required: true,
+    },
     projectNamespace: {
       type: String,
       required: true,
@@ -419,6 +423,7 @@ export default {
         :markdown-docs-path="markdownDocsPath"
         :markdown-preview-path="markdownPreviewPath"
         :project-path="projectPath"
+        :project-id="projectId"
         :project-namespace="projectNamespace"
         :show-delete-button="showDeleteButton"
         :can-attach-file="canAttachFile"

app/assets/javascripts/issue_show/components/fields/description_template.vue

@@ -21,6 +21,10 @@ export default {
       type: String,
       required: true,
     },
+    projectId: {
+      type: Number,
+      required: true,
+    },
     projectNamespace: {
       type: String,
       required: true,
@@ -49,11 +53,12 @@ export default {
 </script>
 
 <template>
-  <div class="dropdown js-issuable-selector-wrap" data-issuable-type="issue">
+  <div class="dropdown js-issuable-selector-wrap" data-issuable-type="issues">
     <button
       ref="toggle"
       :data-namespace-path="projectNamespace"
       :data-project-path="projectPath"
+      :data-project-id="projectId"
       :data-data="issuableTemplatesJson"
       class="dropdown-menu-toggle js-issuable-selector"
       type="button"

app/assets/javascripts/issue_show/components/form.vue

@@ -46,6 +46,10 @@ export default {
       type: String,
       required: true,
     },
+    projectId: {
+      type: Number,
+      required: true,
+    },
     projectNamespace: {
       type: String,
       required: true,
@@ -127,6 +131,7 @@ export default {
           :form-state="formState"
           :issuable-templates="issuableTemplates"
           :project-path="projectPath"
+          :project-id="projectId"
           :project-namespace="projectNamespace"
         />
       </div>

app/assets/javascripts/issue_show/issue.js

@@ -54,6 +54,7 @@ export function initIssueHeaderActions(store) {
       issueType: el.dataset.issueType,
       newIssuePath: el.dataset.newIssuePath,
       projectPath: el.dataset.projectPath,
+      projectId: el.dataset.projectId,
       reportAbusePath: el.dataset.reportAbusePath,
       submitAsSpamPath: el.dataset.submitAsSpamPath,
     },

app/assets/javascripts/templates/issuable_template_selector.js

@@ -9,8 +9,7 @@ export default class IssuableTemplateSelector extends TemplateSelector {
   constructor(...args) {
     super(...args);
 
-    this.projectPath = this.dropdown.data('projectPath');
-    this.namespacePath = this.dropdown.data('namespacePath');
+    this.projectId = this.dropdown.data('projectId');
     this.issuableType = this.$dropdownContainer.data('issuableType');
     this.titleInput = $(`#${this.issuableType}_title`);
     this.templateWarningEl = $('.js-issuable-template-warning');
@@ -81,21 +80,21 @@ export default class IssuableTemplateSelector extends TemplateSelector {
   }
 
   requestFile(query) {
+    const callback = (currentTemplate) => {
+      this.currentTemplate = currentTemplate;
+      this.stopLoadingSpinner();
+      this.setInputValueToTemplateContent();
+    };
+
     this.startLoadingSpinner();
 
-    Api.issueTemplate(
-      this.namespacePath,
-      this.projectPath,
-      query.name,
+    Api.projectTemplate(
+      this.projectId,
       this.issuableType,
-      (err, currentTemplate) => {
-        this.currentTemplate = currentTemplate;
-        this.stopLoadingSpinner();
-        if (err) return; // Error handled by global AJAX error handler
-        this.setInputValueToTemplateContent();
-      },
+      query.name,
+      { source_template_project_id: query.project_id },
+      callback,
     );
-    return;
   }
 
   setInputValueToTemplateContent() {

app/finders/license_template_finder.rb

@@ -40,6 +40,7 @@ class LicenseTemplateFinder
         LicenseTemplate.new(
           key: license.key,
           name: license.name,
+          project: project,
           nickname: license.nickname,
           category: (license.featured? ? :Popular : :Other),
           content: license.content,

app/helpers/issuables_description_templates_helper.rb

@@ -16,9 +16,7 @@ module IssuablesDescriptionTemplatesHelper
         data: issuable_templates(ref_project, issuable.to_ability_name),
         field_name: 'issuable_template',
         selected: selected_template(issuable),
-        project_id: ref_project.id,
-        project_path: ref_project.path,
-        namespace_path: ref_project.namespace.full_path
+        project_id: ref_project.id
       }
     }
 

app/models/license_template.rb

@@ -12,11 +12,12 @@ class LicenseTemplate
       (fullname|name\sof\s(author|copyright\sowner))
     [\>\}\]]}xi.freeze
 
-  attr_reader :key, :name, :category, :nickname, :url, :meta
+  attr_reader :key, :name, :project, :category, :nickname, :url, :meta
 
-  def initialize(key:, name:, category:, content:, nickname: nil, url: nil, meta: {})
+  def initialize(key:, name:, project:, category:, content:, nickname: nil, url: nil, meta: {})
     @key = key
     @name = name
+    @project = project
     @category = category
     @content = content
     @nickname = nickname
@@ -24,6 +25,10 @@ class LicenseTemplate
     @meta = meta
   end
 
+  def project_id
+    project&.id
+  end
+
   def popular?
     category == :Popular
   end

app/views/shared/issuable/form/_template_selector.html.haml

@@ -3,7 +3,7 @@
 - return unless issuable && issuable_templates(ref_project, issuable.to_ability_name).any?
 
 .issuable-form-select-holder.selectbox.form-group
-  .js-issuable-selector-wrap{ data: { issuable_type: issuable.to_ability_name, qa_selector: 'template_dropdown' } }
+  .js-issuable-selector-wrap{ data: { issuable_type: issuable.to_ability_name.pluralize, qa_selector: 'template_dropdown' } }
     = template_dropdown_tag(issuable) do
       %ul.dropdown-footer-list
         %li

doc/api/project_templates.md

@@ -94,14 +94,15 @@ Example response (licenses):
 ## Get one template of a particular type
 
 ```plaintext
-GET /projects/:id/templates/:type/:key
+GET /projects/:id/templates/:type/:name
 ```
 
 | Attribute  | Type   | Required | Description |
 | ---------- | ------ | -------- | ----------- |
 | `id`      | integer / string | yes      | The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) |
 | `type`     | string | yes| The type `(dockerfiles|gitignores|gitlab_ci_ymls|licenses|issues|merge_requests)` of the template |
-| `key`      | string | yes      | The key of the template, as obtained from the collection endpoint |
+| `name`     | string | yes      | The key of the template, as obtained from the collection endpoint |
+| `source_template_project_id`   | integer | no      | The project ID where a given template is being stored. This is useful when multiple templates from different projects have the same name. If multiple templates have the same name, the match from `closest ancestor` is returned if `source_template_project_id` is not specified |
 | `project`  | string | no       | The project name to use when expanding placeholders in the template. Only affects licenses |
 | `fullname` | string | no       | The full name of the copyright holder to use when expanding placeholders in the template. Only affects licenses |
 

ee/app/assets/javascripts/epic/components/epic_body.vue

@@ -45,6 +45,7 @@ export default {
         :endpoint="endpoint"
         :update-endpoint="updateEndpoint"
         :project-path="groupPath"
+        :project-id="0"
         :markdown-preview-path="markdownPreviewPath"
         :markdown-docs-path="markdownDocsPath"
         :can-update="canUpdate"

ee/app/finders/ee/license_template_finder.rb

@@ -19,7 +19,7 @@ module EE
       return super unless custom_templates?
 
       if params[:name]
-        custom_templates.find(params[:name]) || super
+        custom_templates.find(params[:name], params[:source_template_project_id]) || super
       else
         custom_templates.all + super
       end

ee/app/finders/ee/template_finder.rb

@@ -28,7 +28,7 @@ module EE
       return super if custom_templates.nil? || !custom_templates.enabled?
 
       if params[:name]
-        custom_templates.find(params[:name]) || super
+        custom_templates.find(params[:name], params[:source_template_project_id]) || super
       else
         custom_templates.all + super
       end

ee/changelogs/unreleased/fix-file-tmpls-return-same-content-for-different-tmpls.yml

+---
+title: Fix file templates return same content for templates with same name but from
+  different projects
+merge_request: 52332
+author:
+type: fixed

ee/lib/gitlab/custom_file_templates.rb

@@ -43,12 +43,16 @@ module Gitlab
       by_namespace + by_instance
     end
 
-    def find(name)
+    def find(name, source_template_project_id = nil)
       namespace_template_projects_hash.each do |namespace, project|
+        next if source_template_project_id && project&.id != source_template_project_id.to_i
+
         found = template_for(project, name, category_for(namespace))
         return found if found
       end
 
+      return if source_template_project_id && instance_template_project&.id != source_template_project_id.to_i
+
       template_for(instance_template_project, name, _('Instance'))
     end
 
@@ -78,7 +82,7 @@ module Gitlab
     # ordered from most-specific to least-specific
     def namespace_template_projects_hash
       strong_memoize(:namespace_template_projects_hash) do
-        next [] unless project.present?
+        next {} unless project.present?
 
         project
           .ancestors_upto(nil)
@@ -98,18 +102,18 @@ module Gitlab
     def templates_for(project, category)
       return [] unless project
 
-      finder.all(project).map { |template| translate(template, category: category) }
+      finder.all(project).map { |template| translate(template, project, category: category) }
     end
 
     def template_for(project, name, category)
       return unless project
 
-      translate(finder.find(name, project), category: category)
+      translate(finder.find(name, project), project, category: category)
     rescue ::Gitlab::Template::Finders::RepoTemplateFinder::FileNotFoundError
       nil
     end
 
-    def translate(template, category:)
+    def translate(template, project, category:)
       return unless template
 
       template.category = category
@@ -122,6 +126,7 @@ module Gitlab
       LicenseTemplate.new(
         key: template.key,
         name: template.name,
+        project: project,
         nickname: template.name,
         category: template.category,
         content: -> { template.content }

ee/spec/finders/license_template_finder_spec.rb

@@ -7,7 +7,7 @@ RSpec.describe LicenseTemplateFinder do
 
   let(:params) { {} }
   let(:fake_template_source) { double(::Gitlab::CustomFileTemplates) }
-  let(:custom_template) { ::LicenseTemplate.new(key: 'foo', name: 'foo', category: nil, content: 'Template') }
+  let(:custom_template) { ::LicenseTemplate.new(key: 'foo', name: 'foo', project: project, category: nil, content: 'Template') }
   let(:custom_templates) { [custom_template] }
 
   subject(:finder) { described_class.new(project, params) }
@@ -23,7 +23,7 @@ RSpec.describe LicenseTemplateFinder do
 
       allow(fake_template_source)
         .to receive(:find)
-        .with(custom_template.key)
+        .with(custom_template.key, nil)
         .and_return(custom_template)
 
       allow(fake_template_source)

ee/spec/finders/template_finder_spec.rb

@@ -33,7 +33,7 @@ RSpec.describe TemplateFinder do
 
         allow(fake_template_source)
           .to receive(:find)
-          .with(custom_template.key)
+          .with(custom_template.key, nil)
           .and_return(custom_template)
 
         allow(fake_template_source)

ee/spec/helpers/ee/blob_helper_spec.rb

@@ -29,10 +29,10 @@ RSpec.describe BlobHelper do
       expect(Gitlab::Template::CustomLicenseTemplate)
         .to receive(:all)
         .with(project)
-        .and_return([OpenStruct.new(key: 'name', name: 'Name')])
+        .and_return([OpenStruct.new(key: 'name', name: 'Name', project_id: project.id)])
 
       expect(categories).to contain_exactly(:Popular, :Other, group_category)
-      expect(by_group).to contain_exactly({ id: 'name', name: 'Name', key: 'name', project_id: nil })
+      expect(by_group).to contain_exactly({ id: 'name', name: 'Name', key: 'name', project_id: project.id })
       expect(by_popular).to be_present
       expect(by_other).to be_present
     end
@@ -43,10 +43,10 @@ RSpec.describe BlobHelper do
       expect(Gitlab::Template::CustomLicenseTemplate)
         .to receive(:all)
         .with(project)
-        .and_return([OpenStruct.new(key: 'name', name: 'Name')])
+        .and_return([OpenStruct.new(key: 'name', name: 'Name', project_id: project.id)])
 
       expect(categories).to contain_exactly(:Popular, :Other, 'Instance')
-      expect(by_instance).to contain_exactly({ id: 'name', name: 'Name', key: 'name', project_id: nil })
+      expect(by_instance).to contain_exactly({ id: 'name', name: 'Name', key: 'name', project_id: project.id })
       expect(by_popular).to be_present
       expect(by_other).to be_present
     end

ee/spec/lib/gitlab/custom_file_templates_spec.rb

@@ -180,6 +180,44 @@ RSpec.describe Gitlab::CustomFileTemplates do
           it 'returns nil for an unknown key' do
             expect(templates.find('unknown')).to be_nil
           end
+
+          context 'when subgroup template names overlap with ancestor' do
+            let(:subgroup_template_project) { create(:project, :custom_repo, namespace: subgroup, files: template_files('group')) }
+
+            before do
+              subgroup.update!(file_template_project: subgroup_template_project)
+            end
+
+            it 'returns a template from the subgroup' do
+              expect(templates.find(group_key)).to be_template(group_key, "Group #{subgroup.full_name}")
+            end
+
+            it 'finds a template from the parent group with specified project' do
+              expect(templates.find(group_key, group_template_project.id)).to be_template(group_key, "Group #{group.full_name}")
+            end
+          end
+        end
+
+        context 'when looking for template for a specific project' do
+          let(:target_project) { project }
+
+          let_it_be(:another_project) { create(:project, :custom_repo, namespace: group, files: template_files('group')) }
+
+          it 'finds a valid template when looking into group template project' do
+            templates_find = templates.find(group_key, group_template_project.id)
+
+            expect(templates_find).to be_template(group_key, "Group #{group.full_name}")
+          end
+
+          it 'finds a valid template when looking into instance template project' do
+            templates_find = templates.find(instance_key, instance_template_project.id)
+
+            expect(templates_find).to be_template(instance_key, "Instance")
+          end
+
+          it 'does not find a template when given project does not have the template' do
+            expect(templates.find(group_key, another_project.id)).to be_nil
+          end
         end
       end
     end

ee/spec/lib/gitlab/template/custom_templates_spec.rb

@@ -30,7 +30,7 @@ RSpec.describe "Custom file template classes" do
     'LICENSE/category/baz.txt'    => 'CustomLicenseTemplate category baz'
   }
 
-  let(:project) { create(:project, :custom_repo, files: files) }
+  let_it_be(:project) { create(:project, :custom_repo, files: files) }
 
   [
     ::Gitlab::Template::CustomDockerfileTemplate,

lib/api/project_templates.rb

@@ -36,16 +36,22 @@ module API
       end
       params do
         requires :name, type: String, desc: 'The name of the template'
-
+        optional :source_template_project_id, type: Integer,
+                 desc: 'The project id where a given template is being stored. This is useful when multiple templates from different projects have the same name'
         optional :project, type: String, desc: 'The project name to use when expanding placeholders in the template. Only affects licenses'
         optional :fullname, type: String, desc: 'The full name of the copyright holder to use when expanding placeholders in the template. Only affects licenses'
       end
 
       get ':id/templates/:type/:name', requirements: TEMPLATE_NAMES_ENDPOINT_REQUIREMENTS do
         begin
-          template = TemplateFinder
-            .build(params[:type], user_project, name: params[:name])
-            .execute
+          template = TemplateFinder.build(
+            params[:type],
+            user_project,
+            {
+              name: params[:name],
+              source_template_project_id: params[:source_template_project_id]
+            }
+          ).execute
         rescue ::Gitlab::Template::Finders::RepoTemplateFinder::FileNotFoundError
           not_found!('Template')
         end

lib/gitlab/template/base_template.rb

@@ -8,6 +8,7 @@ module Gitlab
       def initialize(path, project = nil, category: nil)
         @path = path
         @category = category
+        @project = project
         @finder = self.class.finder(project)
       end
 
@@ -31,6 +32,10 @@ module Gitlab
         # override with a comment to be placed at the top of the blob.
       end
 
+      def project_id
+        @project&.id
+      end
+
       # Present for compatibility with license templates, which can replace text
       # like `[fullname]` with a user-specified string. This is a no-op for
       # other templates
@@ -76,7 +81,7 @@ module Gitlab
         end
 
         # Defines which strategy will be used to get templates files
-        # RepoTemplateFinder - Finds templates on project repository, templates are filtered perproject
+        # RepoTemplateFinder - Finds templates on project repository, templates are filtered per project
         # GlobalTemplateFinder - Finds templates on gitlab installation source, templates can be used in all projects
         def finder(project = nil)
           raise NotImplementedError

spec/frontend/issue_show/components/app_spec.js

@@ -423,7 +423,7 @@ describe('Issuable output', () => {
     });
 
     it('shows the form if template names request is successful', () => {
-      const mockData = [{ name: 'Bug' }];
+      const mockData = [{ name: 'test', id: 'test', project_path: '/', namespace_path: '/' }];
       mock.onGet('/issuable-templates-path').reply(() => Promise.resolve([200, mockData]));
 
       return wrapper.vm.requestTemplatesAndShowForm().then(() => {

spec/frontend/issue_show/components/fields/description_template_spec.js

@@ -14,8 +14,10 @@ describe('Issue description template component', () => {
     vm = new Component({
       propsData: {
         formState,
-        issuableTemplates: [{ name: 'test' }],
+        issuableTemplates: [{ name: 'test', id: 'test', project_path: '/', namespace_path: '/' }],
+        projectId: 1,
         projectPath: '/',
+        namespacePath: '/',
         projectNamespace: '/',
       },
     }).$mount();
@@ -23,7 +25,7 @@ describe('Issue description template component', () => {
 
   it('renders templates as JSON array in data attribute', () => {
     expect(vm.$el.querySelector('.js-issuable-selector').getAttribute('data-data')).toBe(
-      '[{"name":"test"}]',
+      '[{"name":"test","id":"test","project_path":"/","namespace_path":"/"}]',
     );
   });
 

spec/frontend/issue_show/components/form_spec.js

@@ -19,6 +19,7 @@ describe('Inline edit form component', () => {
     markdownPreviewPath: '/',
     markdownDocsPath: '/',
     projectPath: '/',
+    projectId: 1,
     projectNamespace: '/',
   };
 
@@ -42,7 +43,11 @@ describe('Inline edit form component', () => {
   });
 
   it('renders template selector when templates exists', () => {
-    createComponent({ issuableTemplates: ['test'] });
+    createComponent({
+      issuableTemplates: [
+        { name: 'test', id: 'test', project_path: 'test', namespace_path: 'test' },
+      ],
+    });
 
     expect(vm.$el.querySelector('.js-issuable-selector-wrap')).not.toBeNull();
   });

spec/frontend/issue_show/mock_data.js

@@ -52,6 +52,7 @@ export const appProps = {
   markdownDocsPath: '/',
   projectNamespace: '/',
   projectPath: '/',
+  projectId: 1,
   issuableTemplateNamesPath: '/issuable-templates-path',
   zoomMeetingUrl,
   publishedIncidentUrl,

spec/helpers/issuables_helper_spec.rb

@@ -209,6 +209,7 @@ RSpec.describe IssuablesHelper do
         markdownDocsPath: '/help/user/markdown',
         lockVersion: issue.lock_version,
         projectPath: @project.path,
+        projectId: @project.id,
         projectNamespace: @project.namespace.path,
         initialTitleHtml: issue.title,
         initialTitleText: issue.title,

spec/models/license_template_spec.rb

@@ -57,6 +57,6 @@ RSpec.describe LicenseTemplate do
   end
 
   def build_template(content)
-    described_class.new(key: 'foo', name: 'foo', category: :Other, content: content)
+    described_class.new(key: 'foo', name: 'foo', project: nil, category: :Other, content: content)
   end
 end

spec/requests/api/project_templates_spec.rb

@@ -131,7 +131,7 @@ RSpec.describe API::ProjectTemplates do
     end
   end
 
-  describe 'GET /projects/:id/templates/:type/:key' do
+  describe 'GET /projects/:id/templates/:type/:name' do
     it 'returns a specific dockerfile' do
       get api("/projects/#{public_project.id}/templates/dockerfiles/Binary")
 

spec/support/shared_examples/helpers/issuable_description_templates_shared_examples.rb

@@ -23,11 +23,11 @@ RSpec.shared_examples 'project issuable templates' do
     end
 
     it 'returns only md files as issue templates' do
-      expect(helper.issuable_templates(project, 'issue')).to eq(templates('issue', nil))
+      expect(helper.issuable_templates(project, 'issue')).to eq(templates('issue', project))
     end
 
     it 'returns only md files as merge_request templates' do
-      expect(helper.issuable_templates(project, 'merge_request')).to eq(templates('merge_request', nil))
+      expect(helper.issuable_templates(project, 'merge_request')).to eq(templates('merge_request', project))
     end
   end