Commit 1f85284f authored by Alexandru Croitor's avatar Alexandru Croitor

Fix file templates returning same content

This change accounts for the project where a given file template is
being stored. This helps to fix an issue with file templates
returning the same content for templates with same name even if
templates are from different projects and have different contents.
parent 1e1edbaf
......@@ -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);
}
......
......@@ -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({
......
......@@ -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();
......
......@@ -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"
......
......@@ -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"
......
......@@ -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>
......
......@@ -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,
},
......
......@@ -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() {
......
......@@ -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,
......
......@@ -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
}
}
......
......@@ -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
......
......@@ -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
......
......@@ -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 |
......
......@@ -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"
......
......@@ -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
......
......@@ -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
......
---
title: Fix file templates return same content for templates with same name but from
different projects
merge_request: 52332
author:
type: fixed
......@@ -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 }
......
......@@ -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)
......
......@@ -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)
......
......@@ -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
......
......@@ -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
......
......@@ -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,
......
......@@ -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
......
......@@ -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
......
......@@ -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(() => {
......
......@@ -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":"/"}]',
);
});
......
......@@ -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();
});
......
......@@ -52,6 +52,7 @@ export const appProps = {
markdownDocsPath: '/',
projectNamespace: '/',
projectPath: '/',
projectId: 1,
issuableTemplateNamesPath: '/issuable-templates-path',
zoomMeetingUrl,
publishedIncidentUrl,
......
......@@ -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,
......
......@@ -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
......@@ -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")
......
......@@ -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
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment