From 519642af7720349f5aec19dc9b620bbb287cc526 Mon Sep 17 00:00:00 2001 From: barthc Date: Tue, 14 Jun 2016 18:57:50 +0100 Subject: fix clickable code search results --- app/views/search/results/_blob.html.haml | 5 +++-- app/views/shared/_file_highlight.html.haml | 3 ++- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/app/views/search/results/_blob.html.haml b/app/views/search/results/_blob.html.haml index 0fe8a3b490a..b28482f5db1 100644 --- a/app/views/search/results/_blob.html.haml +++ b/app/views/search/results/_blob.html.haml @@ -2,9 +2,10 @@ .blob-result .file-holder .file-title - = link_to namespace_project_blob_path(@project.namespace, @project, tree_join(blob.ref, blob.filename), :anchor => "L" + blob.startline.to_s) do + - blob_path = namespace_project_blob_path(@project.namespace, @project, tree_join(blob.ref, blob.filename)) + = link_to blob_path do %i.fa.fa-file %strong = blob.filename .file-content.code.term - = render 'shared/file_highlight', blob: blob, first_line_number: blob.startline + = render 'shared/file_highlight', blob: blob, first_line_number: blob.startline, blob_path: blob_path diff --git a/app/views/shared/_file_highlight.html.haml b/app/views/shared/_file_highlight.html.haml index 37dcf39c062..d9e9078cc1e 100644 --- a/app/views/shared/_file_highlight.html.haml +++ b/app/views/shared/_file_highlight.html.haml @@ -2,11 +2,12 @@ .line-numbers - if blob.data.present? - link_icon = icon('link') + - path = blob_path if defined?(blob_path) - blob.data.each_line.each_with_index do |_, index| - offset = defined?(first_line_number) ? first_line_number : 1 - i = index + offset -# We're not using `link_to` because it is too slow once we get to thousands of lines. - %a.diff-line-num{href: "#L#{i}", id: "L#{i}", 'data-line-number' => i} + %a.diff-line-num{href: "#{path}#L#{i}", id: "L#{i}", 'data-line-number' => i} = link_icon = i .blob-content{data: {blob_id: blob.id}} -- cgit v1.2.1 From 76aea978c6b2d8c69881836408c4947d816d2db2 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 9 Jun 2016 14:48:40 +0200 Subject: Add class that encapsulates error in new Ci config --- lib/gitlab/ci/config.rb | 10 ++++++++-- lib/gitlab/ci/config/node/configurable.rb | 4 ++-- lib/gitlab/ci/config/node/entry.rb | 9 +++++++++ lib/gitlab/ci/config/node/error.rb | 26 ++++++++++++++++++++++++++ lib/gitlab/ci/config/node/factory.rb | 1 + lib/gitlab/ci/config/node/script.rb | 2 +- spec/lib/ci/gitlab_ci_yaml_processor_spec.rb | 2 +- spec/lib/gitlab/ci/config/node/error_spec.rb | 23 +++++++++++++++++++++++ spec/lib/gitlab/ci/config/node/factory_spec.rb | 10 ++++++++++ spec/lib/gitlab/ci/config/node/global_spec.rb | 8 +++++++- spec/lib/gitlab/ci/config/node/script_spec.rb | 2 +- spec/lib/gitlab/ci/config_spec.rb | 6 ++++++ 12 files changed, 95 insertions(+), 8 deletions(-) create mode 100644 lib/gitlab/ci/config/node/error.rb create mode 100644 spec/lib/gitlab/ci/config/node/error_spec.rb diff --git a/lib/gitlab/ci/config.rb b/lib/gitlab/ci/config.rb index b48d3592f16..26028547fa0 100644 --- a/lib/gitlab/ci/config.rb +++ b/lib/gitlab/ci/config.rb @@ -4,8 +4,6 @@ module Gitlab # Base GitLab CI Configuration facade # class Config - delegate :valid?, :errors, to: :@global - ## # Temporary delegations that should be removed after refactoring # @@ -18,6 +16,14 @@ module Gitlab @global.process! end + def valid? + errors.none? + end + + def errors + @global.errors.map(&:to_s) + end + def to_hash @config end diff --git a/lib/gitlab/ci/config/node/configurable.rb b/lib/gitlab/ci/config/node/configurable.rb index d60f87f3f94..2f4abbf9ffe 100644 --- a/lib/gitlab/ci/config/node/configurable.rb +++ b/lib/gitlab/ci/config/node/configurable.rb @@ -24,12 +24,12 @@ module Gitlab def prevalidate! unless @value.is_a?(Hash) - @errors << 'should be a configuration entry with hash value' + add_error('should be a configuration entry with hash value') end end def create_node(key, factory) - factory.with(value: @value[key]) + factory.with(value: @value[key], key: key) factory.nullify! unless @value.has_key?(key) factory.create! end diff --git a/lib/gitlab/ci/config/node/entry.rb b/lib/gitlab/ci/config/node/entry.rb index 52758a962f3..f41e191d611 100644 --- a/lib/gitlab/ci/config/node/entry.rb +++ b/lib/gitlab/ci/config/node/entry.rb @@ -8,6 +8,7 @@ module Gitlab class Entry class InvalidError < StandardError; end + attr_writer :key attr_accessor :description def initialize(value) @@ -40,10 +41,18 @@ module Gitlab allowed_nodes.none? end + def key + @key || self.class.name.demodulize.underscore + end + def errors @errors + nodes.map(&:errors).flatten end + def add_error(message) + @errors << Error.new(message, self) + end + def allowed_nodes {} end diff --git a/lib/gitlab/ci/config/node/error.rb b/lib/gitlab/ci/config/node/error.rb new file mode 100644 index 00000000000..5b8d0288e4e --- /dev/null +++ b/lib/gitlab/ci/config/node/error.rb @@ -0,0 +1,26 @@ +module Gitlab + module Ci + class Config + module Node + class Error + def initialize(message, parent) + @message = message + @parent = parent + end + + def key + @parent.key + end + + def to_s + "#{key}: #{@message}" + end + + def ==(other) + other.to_s == to_s + end + end + end + end + end +end diff --git a/lib/gitlab/ci/config/node/factory.rb b/lib/gitlab/ci/config/node/factory.rb index 787ca006f5a..025ae40ef94 100644 --- a/lib/gitlab/ci/config/node/factory.rb +++ b/lib/gitlab/ci/config/node/factory.rb @@ -30,6 +30,7 @@ module Gitlab @entry_class.new(@attributes[:value]).tap do |entry| entry.description = @attributes[:description] + entry.key = @attributes[:key] end end end diff --git a/lib/gitlab/ci/config/node/script.rb b/lib/gitlab/ci/config/node/script.rb index 5072bf0db7d..314877a035c 100644 --- a/lib/gitlab/ci/config/node/script.rb +++ b/lib/gitlab/ci/config/node/script.rb @@ -19,7 +19,7 @@ module Gitlab def validate! unless validate_array_of_strings(@value) - @errors << 'before_script should be an array of strings' + add_error('should be an array of strings') end end end diff --git a/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb b/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb index 5e1d2b8e4f5..40ee1eb64ca 100644 --- a/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb +++ b/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb @@ -820,7 +820,7 @@ EOT config = YAML.dump({ before_script: "bundle update", rspec: { script: "test" } }) expect do GitlabCiYamlProcessor.new(config, path) - end.to raise_error(GitlabCiYamlProcessor::ValidationError, "before_script should be an array of strings") + end.to raise_error(GitlabCiYamlProcessor::ValidationError, "before_script: should be an array of strings") end it "returns errors if job before_script parameter is not an array of strings" do diff --git a/spec/lib/gitlab/ci/config/node/error_spec.rb b/spec/lib/gitlab/ci/config/node/error_spec.rb new file mode 100644 index 00000000000..764fdfdb821 --- /dev/null +++ b/spec/lib/gitlab/ci/config/node/error_spec.rb @@ -0,0 +1,23 @@ +require 'spec_helper' + +describe Gitlab::Ci::Config::Node::Error do + let(:error) { described_class.new(message, parent) } + let(:message) { 'some error' } + let(:parent) { spy('parent') } + + before do + allow(parent).to receive(:key).and_return('parent_key') + end + + describe '#key' do + it 'returns underscored class name' do + expect(error.key).to eq 'parent_key' + end + end + + describe '#to_s' do + it 'returns valid error message' do + expect(error.to_s).to eq 'parent_key: some error' + end + end +end diff --git a/spec/lib/gitlab/ci/config/node/factory_spec.rb b/spec/lib/gitlab/ci/config/node/factory_spec.rb index d681aa32456..01a707a6bd4 100644 --- a/spec/lib/gitlab/ci/config/node/factory_spec.rb +++ b/spec/lib/gitlab/ci/config/node/factory_spec.rb @@ -25,6 +25,16 @@ describe Gitlab::Ci::Config::Node::Factory do expect(entry.description).to eq 'test description' end end + + context 'when setting key' do + it 'creates entry with custom key' do + entry = factory + .with(value: ['ls', 'pwd'], key: 'test key') + .create! + + expect(entry.key).to eq 'test key' + end + end end context 'when not setting value' do diff --git a/spec/lib/gitlab/ci/config/node/global_spec.rb b/spec/lib/gitlab/ci/config/node/global_spec.rb index b1972172435..89a7183b655 100644 --- a/spec/lib/gitlab/ci/config/node/global_spec.rb +++ b/spec/lib/gitlab/ci/config/node/global_spec.rb @@ -13,6 +13,12 @@ describe Gitlab::Ci::Config::Node::Global do end end + describe '#key' do + it 'returns underscored class name' do + expect(global.key).to eq 'global' + end + end + context 'when hash is valid' do let(:hash) do { before_script: ['ls', 'pwd'] } @@ -79,7 +85,7 @@ describe Gitlab::Ci::Config::Node::Global do describe '#errors' do it 'reports errors from child nodes' do expect(global.errors) - .to include 'before_script should be an array of strings' + .to include 'before_script: should be an array of strings' end end diff --git a/spec/lib/gitlab/ci/config/node/script_spec.rb b/spec/lib/gitlab/ci/config/node/script_spec.rb index e4d6481f8a5..ff7016046cc 100644 --- a/spec/lib/gitlab/ci/config/node/script_spec.rb +++ b/spec/lib/gitlab/ci/config/node/script_spec.rb @@ -34,7 +34,7 @@ describe Gitlab::Ci::Config::Node::Script do describe '#errors' do it 'saves errors' do expect(entry.errors) - .to include /should be an array of strings/ + .to include 'script: should be an array of strings' end end diff --git a/spec/lib/gitlab/ci/config_spec.rb b/spec/lib/gitlab/ci/config_spec.rb index 3871d939feb..2a5d132db7b 100644 --- a/spec/lib/gitlab/ci/config_spec.rb +++ b/spec/lib/gitlab/ci/config_spec.rb @@ -67,6 +67,12 @@ describe Gitlab::Ci::Config do expect(config.errors).not_to be_empty end end + + describe '#errors' do + it 'returns an array of strings' do + expect(config.errors).to all(be_an_instance_of(String)) + end + end end end end -- cgit v1.2.1 From 95520dfc72770955ae99c73f90e02037f3b1d4b5 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 16 Jun 2016 14:16:33 +0200 Subject: Add prototype of CI config node validator This makes use of `ActiveModel::Validations` encapsulated in a separate class that is accessible from a node object. --- lib/gitlab/ci/config.rb | 4 +-- lib/gitlab/ci/config/node/configurable.rb | 23 +++++++----- lib/gitlab/ci/config/node/entry.rb | 41 +++++++++------------- lib/gitlab/ci/config/node/error.rb | 26 -------------- lib/gitlab/ci/config/node/script.rb | 18 ++++++---- lib/gitlab/ci/config/node/validatable.rb | 30 ++++++++++++++++ lib/gitlab/ci/config/node/validator.rb | 25 +++++++++++++ spec/lib/ci/gitlab_ci_yaml_processor_spec.rb | 2 +- .../lib/gitlab/ci/config/node/configurable_spec.rb | 1 + spec/lib/gitlab/ci/config/node/error_spec.rb | 23 ------------ spec/lib/gitlab/ci/config/node/global_spec.rb | 2 +- spec/lib/gitlab/ci/config/node/script_spec.rb | 14 ++++---- 12 files changed, 108 insertions(+), 101 deletions(-) delete mode 100644 lib/gitlab/ci/config/node/error.rb create mode 100644 lib/gitlab/ci/config/node/validatable.rb create mode 100644 lib/gitlab/ci/config/node/validator.rb delete mode 100644 spec/lib/gitlab/ci/config/node/error_spec.rb diff --git a/lib/gitlab/ci/config.rb b/lib/gitlab/ci/config.rb index 26028547fa0..adfd097736e 100644 --- a/lib/gitlab/ci/config.rb +++ b/lib/gitlab/ci/config.rb @@ -17,11 +17,11 @@ module Gitlab end def valid? - errors.none? + @global.valid? end def errors - @global.errors.map(&:to_s) + @global.errors end def to_hash diff --git a/lib/gitlab/ci/config/node/configurable.rb b/lib/gitlab/ci/config/node/configurable.rb index 2f4abbf9ffe..23d3f9f3277 100644 --- a/lib/gitlab/ci/config/node/configurable.rb +++ b/lib/gitlab/ci/config/node/configurable.rb @@ -16,21 +16,27 @@ module Gitlab module Configurable extend ActiveSupport::Concern + included do + validations do + validate :hash_config_value + + def hash_config_value + unless self.config.is_a?(Hash) + errors.add(:config, 'should be a configuration entry hash') + end + end + end + end + def allowed_nodes self.class.allowed_nodes || {} end private - def prevalidate! - unless @value.is_a?(Hash) - add_error('should be a configuration entry with hash value') - end - end - def create_node(key, factory) - factory.with(value: @value[key], key: key) - factory.nullify! unless @value.has_key?(key) + factory.with(value: @config[key], key: key) + factory.nullify! unless @config.has_key?(key) factory.create! end @@ -47,7 +53,6 @@ module Gitlab define_method(symbol) do raise Entry::InvalidError unless valid? - @nodes[symbol].try(:value) end diff --git a/lib/gitlab/ci/config/node/entry.rb b/lib/gitlab/ci/config/node/entry.rb index f41e191d611..823c8bb5d13 100644 --- a/lib/gitlab/ci/config/node/entry.rb +++ b/lib/gitlab/ci/config/node/entry.rb @@ -7,16 +7,15 @@ module Gitlab # class Entry class InvalidError < StandardError; end + include Validatable - attr_writer :key - attr_accessor :description + attr_reader :config + attr_accessor :key, :description - def initialize(value) - @value = value + def initialize(config) + @config = config @nodes = {} - @errors = [] - - prevalidate! + @validator = self.class.validator.new(self) end def process! @@ -24,19 +23,13 @@ module Gitlab return unless valid? compose! - - nodes.each(&:process!) - nodes.each(&:validate!) + process_nodes! end def nodes @nodes.values end - def valid? - errors.none? - end - def leaf? allowed_nodes.none? end @@ -45,37 +38,35 @@ module Gitlab @key || self.class.name.demodulize.underscore end - def errors - @errors + nodes.map(&:errors).flatten + def valid? + errors.none? end - def add_error(message) - @errors << Error.new(message, self) + def errors + @validator.full_errors + + nodes.map(&:errors).flatten end def allowed_nodes {} end - def validate! - raise NotImplementedError - end - def value raise NotImplementedError end private - def prevalidate! - end - def compose! allowed_nodes.each do |key, essence| @nodes[key] = create_node(key, essence) end end + def process_nodes! + nodes.each(&:process!) + end + def create_node(key, essence) raise NotImplementedError end diff --git a/lib/gitlab/ci/config/node/error.rb b/lib/gitlab/ci/config/node/error.rb deleted file mode 100644 index 5b8d0288e4e..00000000000 --- a/lib/gitlab/ci/config/node/error.rb +++ /dev/null @@ -1,26 +0,0 @@ -module Gitlab - module Ci - class Config - module Node - class Error - def initialize(message, parent) - @message = message - @parent = parent - end - - def key - @parent.key - end - - def to_s - "#{key}: #{@message}" - end - - def ==(other) - other.to_s == to_s - end - end - end - end - end -end diff --git a/lib/gitlab/ci/config/node/script.rb b/lib/gitlab/ci/config/node/script.rb index 314877a035c..18592acdac6 100644 --- a/lib/gitlab/ci/config/node/script.rb +++ b/lib/gitlab/ci/config/node/script.rb @@ -11,17 +11,21 @@ module Gitlab # implementation in Runner. # class Script < Entry - include ValidationHelpers + validations do + include ValidationHelpers - def value - @value.join("\n") - end + validate :array_of_strings - def validate! - unless validate_array_of_strings(@value) - add_error('should be an array of strings') + def array_of_strings + unless validate_array_of_strings(self.config) + errors.add(:config, 'should be an array of strings') + end end end + + def value + @config.join("\n") + end end end end diff --git a/lib/gitlab/ci/config/node/validatable.rb b/lib/gitlab/ci/config/node/validatable.rb new file mode 100644 index 00000000000..a0617d21ad8 --- /dev/null +++ b/lib/gitlab/ci/config/node/validatable.rb @@ -0,0 +1,30 @@ +module Gitlab + module Ci + class Config + module Node + module Validatable + extend ActiveSupport::Concern + + class_methods do + def validator + validator = Class.new(Node::Validator) + validator.include(ActiveModel::Validations) + + if defined?(@validations) + @validations.each { |rules| validator.class_eval(&rules) } + end + + validator + end + + private + + def validations(&block) + (@validations ||= []).append(block) + end + end + end + end + end + end +end diff --git a/lib/gitlab/ci/config/node/validator.rb b/lib/gitlab/ci/config/node/validator.rb new file mode 100644 index 00000000000..891b19c9743 --- /dev/null +++ b/lib/gitlab/ci/config/node/validator.rb @@ -0,0 +1,25 @@ +module Gitlab + module Ci + class Config + module Node + class Validator < SimpleDelegator + def initialize(node) + @node = node + super(node) + validate + end + + def full_errors + errors.full_messages.map do |error| + "#{@node.key} #{error}".humanize + end + end + + def self.name + 'Validator' + end + end + end + end + end +end diff --git a/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb b/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb index 40ee1eb64ca..42cbb555694 100644 --- a/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb +++ b/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb @@ -820,7 +820,7 @@ EOT config = YAML.dump({ before_script: "bundle update", rspec: { script: "test" } }) expect do GitlabCiYamlProcessor.new(config, path) - end.to raise_error(GitlabCiYamlProcessor::ValidationError, "before_script: should be an array of strings") + end.to raise_error(GitlabCiYamlProcessor::ValidationError, "Before script config should be an array of strings") end it "returns errors if job before_script parameter is not an array of strings" do diff --git a/spec/lib/gitlab/ci/config/node/configurable_spec.rb b/spec/lib/gitlab/ci/config/node/configurable_spec.rb index 47c68f96dc8..8ec7919f4d4 100644 --- a/spec/lib/gitlab/ci/config/node/configurable_spec.rb +++ b/spec/lib/gitlab/ci/config/node/configurable_spec.rb @@ -4,6 +4,7 @@ describe Gitlab::Ci::Config::Node::Configurable do let(:node) { Class.new } before do + node.include(Gitlab::Ci::Config::Node::Validatable) node.include(described_class) end diff --git a/spec/lib/gitlab/ci/config/node/error_spec.rb b/spec/lib/gitlab/ci/config/node/error_spec.rb deleted file mode 100644 index 764fdfdb821..00000000000 --- a/spec/lib/gitlab/ci/config/node/error_spec.rb +++ /dev/null @@ -1,23 +0,0 @@ -require 'spec_helper' - -describe Gitlab::Ci::Config::Node::Error do - let(:error) { described_class.new(message, parent) } - let(:message) { 'some error' } - let(:parent) { spy('parent') } - - before do - allow(parent).to receive(:key).and_return('parent_key') - end - - describe '#key' do - it 'returns underscored class name' do - expect(error.key).to eq 'parent_key' - end - end - - describe '#to_s' do - it 'returns valid error message' do - expect(error.to_s).to eq 'parent_key: some error' - end - end -end diff --git a/spec/lib/gitlab/ci/config/node/global_spec.rb b/spec/lib/gitlab/ci/config/node/global_spec.rb index 89a7183b655..8bb76cf920a 100644 --- a/spec/lib/gitlab/ci/config/node/global_spec.rb +++ b/spec/lib/gitlab/ci/config/node/global_spec.rb @@ -85,7 +85,7 @@ describe Gitlab::Ci::Config::Node::Global do describe '#errors' do it 'reports errors from child nodes' do expect(global.errors) - .to include 'before_script: should be an array of strings' + .to include 'Before script config should be an array of strings' end end diff --git a/spec/lib/gitlab/ci/config/node/script_spec.rb b/spec/lib/gitlab/ci/config/node/script_spec.rb index ff7016046cc..6af6aa15eef 100644 --- a/spec/lib/gitlab/ci/config/node/script_spec.rb +++ b/spec/lib/gitlab/ci/config/node/script_spec.rb @@ -1,13 +1,13 @@ require 'spec_helper' describe Gitlab::Ci::Config::Node::Script do - let(:entry) { described_class.new(value) } + let(:entry) { described_class.new(config) } - describe '#validate!' do - before { entry.validate! } + describe '#process!' do + before { entry.process! } - context 'when entry value is correct' do - let(:value) { ['ls', 'pwd'] } + context 'when entry config value is correct' do + let(:config) { ['ls', 'pwd'] } describe '#value' do it 'returns concatenated command' do @@ -29,12 +29,12 @@ describe Gitlab::Ci::Config::Node::Script do end context 'when entry value is not correct' do - let(:value) { 'ls' } + let(:config) { 'ls' } describe '#errors' do it 'saves errors' do expect(entry.errors) - .to include 'script: should be an array of strings' + .to include 'Script config should be an array of strings' end end -- cgit v1.2.1 From 002e6ed1f042852bef78c3a749e13567d7a0ee5a Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Fri, 17 Jun 2016 09:33:32 +0200 Subject: Improve CI config entries validations prototype --- lib/gitlab/ci/config/node/configurable.rb | 7 +-- lib/gitlab/ci/config/node/entry.rb | 16 ++++-- lib/gitlab/ci/config/node/script.rb | 2 + lib/gitlab/ci/config/node/validatable.rb | 1 - lib/gitlab/ci/config/node/validator.rb | 5 +- .../lib/gitlab/ci/config/node/configurable_spec.rb | 15 +++-- spec/lib/gitlab/ci/config/node/global_spec.rb | 6 +- spec/lib/gitlab/ci/config/node/validatable_spec.rb | 50 ++++++++++++++++ spec/lib/gitlab/ci/config/node/validator_spec.rb | 67 ++++++++++++++++++++++ 9 files changed, 144 insertions(+), 25 deletions(-) create mode 100644 spec/lib/gitlab/ci/config/node/validatable_spec.rb create mode 100644 spec/lib/gitlab/ci/config/node/validator_spec.rb diff --git a/lib/gitlab/ci/config/node/configurable.rb b/lib/gitlab/ci/config/node/configurable.rb index 23d3f9f3277..7b428c1362c 100644 --- a/lib/gitlab/ci/config/node/configurable.rb +++ b/lib/gitlab/ci/config/node/configurable.rb @@ -15,6 +15,7 @@ module Gitlab # module Configurable extend ActiveSupport::Concern + include Validatable included do validations do @@ -28,10 +29,6 @@ module Gitlab end end - def allowed_nodes - self.class.allowed_nodes || {} - end - private def create_node(key, factory) @@ -41,7 +38,7 @@ module Gitlab end class_methods do - def allowed_nodes + def nodes Hash[@allowed_nodes.map { |key, factory| [key, factory.dup] }] end diff --git a/lib/gitlab/ci/config/node/entry.rb b/lib/gitlab/ci/config/node/entry.rb index 823c8bb5d13..f044ef965e9 100644 --- a/lib/gitlab/ci/config/node/entry.rb +++ b/lib/gitlab/ci/config/node/entry.rb @@ -7,7 +7,6 @@ module Gitlab # class Entry class InvalidError < StandardError; end - include Validatable attr_reader :config attr_accessor :key, :description @@ -16,6 +15,7 @@ module Gitlab @config = config @nodes = {} @validator = self.class.validator.new(self) + @validator.validate end def process! @@ -31,7 +31,7 @@ module Gitlab end def leaf? - allowed_nodes.none? + self.class.nodes.none? end def key @@ -47,18 +47,22 @@ module Gitlab nodes.map(&:errors).flatten end - def allowed_nodes + def value + raise NotImplementedError + end + + def self.nodes {} end - def value - raise NotImplementedError + def self.validator + Validator end private def compose! - allowed_nodes.each do |key, essence| + self.class.nodes.each do |key, essence| @nodes[key] = create_node(key, essence) end end diff --git a/lib/gitlab/ci/config/node/script.rb b/lib/gitlab/ci/config/node/script.rb index 18592acdac6..059650d8a52 100644 --- a/lib/gitlab/ci/config/node/script.rb +++ b/lib/gitlab/ci/config/node/script.rb @@ -11,6 +11,8 @@ module Gitlab # implementation in Runner. # class Script < Entry + include Validatable + validations do include ValidationHelpers diff --git a/lib/gitlab/ci/config/node/validatable.rb b/lib/gitlab/ci/config/node/validatable.rb index a0617d21ad8..f6e2896dfb2 100644 --- a/lib/gitlab/ci/config/node/validatable.rb +++ b/lib/gitlab/ci/config/node/validatable.rb @@ -8,7 +8,6 @@ module Gitlab class_methods do def validator validator = Class.new(Node::Validator) - validator.include(ActiveModel::Validations) if defined?(@validations) @validations.each { |rules| validator.class_eval(&rules) } diff --git a/lib/gitlab/ci/config/node/validator.rb b/lib/gitlab/ci/config/node/validator.rb index 891b19c9743..2454cc6d957 100644 --- a/lib/gitlab/ci/config/node/validator.rb +++ b/lib/gitlab/ci/config/node/validator.rb @@ -3,10 +3,11 @@ module Gitlab class Config module Node class Validator < SimpleDelegator + include ActiveModel::Validations + def initialize(node) - @node = node super(node) - validate + @node = node end def full_errors diff --git a/spec/lib/gitlab/ci/config/node/configurable_spec.rb b/spec/lib/gitlab/ci/config/node/configurable_spec.rb index 8ec7919f4d4..9bbda6e7396 100644 --- a/spec/lib/gitlab/ci/config/node/configurable_spec.rb +++ b/spec/lib/gitlab/ci/config/node/configurable_spec.rb @@ -4,30 +4,29 @@ describe Gitlab::Ci::Config::Node::Configurable do let(:node) { Class.new } before do - node.include(Gitlab::Ci::Config::Node::Validatable) node.include(described_class) end - describe 'allowed nodes' do + describe 'configured nodes' do before do node.class_eval do allow_node :object, Object, description: 'test object' end end - describe '#allowed_nodes' do - it 'has valid allowed nodes' do - expect(node.allowed_nodes).to include :object + describe '.nodes' do + it 'has valid nodes' do + expect(node.nodes).to include :object end it 'creates a node factory' do - expect(node.allowed_nodes[:object]) + expect(node.nodes[:object]) .to be_an_instance_of Gitlab::Ci::Config::Node::Factory end it 'returns a duplicated factory object' do - first_factory = node.allowed_nodes[:object] - second_factory = node.allowed_nodes[:object] + first_factory = node.nodes[:object] + second_factory = node.nodes[:object] expect(first_factory).not_to be_equal(second_factory) end diff --git a/spec/lib/gitlab/ci/config/node/global_spec.rb b/spec/lib/gitlab/ci/config/node/global_spec.rb index 8bb76cf920a..fddd53a2b57 100644 --- a/spec/lib/gitlab/ci/config/node/global_spec.rb +++ b/spec/lib/gitlab/ci/config/node/global_spec.rb @@ -3,13 +3,13 @@ require 'spec_helper' describe Gitlab::Ci::Config::Node::Global do let(:global) { described_class.new(hash) } - describe '#allowed_nodes' do + describe '.nodes' do it 'can contain global config keys' do - expect(global.allowed_nodes).to include :before_script + expect(described_class.nodes).to include :before_script end it 'returns a hash' do - expect(global.allowed_nodes).to be_a Hash + expect(described_class.nodes).to be_a Hash end end diff --git a/spec/lib/gitlab/ci/config/node/validatable_spec.rb b/spec/lib/gitlab/ci/config/node/validatable_spec.rb new file mode 100644 index 00000000000..10cd01afcd1 --- /dev/null +++ b/spec/lib/gitlab/ci/config/node/validatable_spec.rb @@ -0,0 +1,50 @@ +require 'spec_helper' + +describe Gitlab::Ci::Config::Node::Validatable do + let(:node) { Class.new } + + before do + node.include(described_class) + end + + describe '.validator' do + before do + node.class_eval do + attr_accessor :test_attribute + + validations do + validates :test_attribute, presence: true + end + end + end + + it 'returns validator' do + expect(node.validator.superclass) + .to be Gitlab::Ci::Config::Node::Validator + end + + context 'when validating node instance' do + let(:node_instance) { node.new } + + context 'when attribute is valid' do + before do + node_instance.test_attribute = 'valid' + end + + it 'instance of validator is valid' do + expect(node.validator.new(node_instance)).to be_valid + end + end + + context 'when attribute is not valid' do + before do + node_instance.test_attribute = nil + end + + it 'instance of validator is invalid' do + expect(node.validator.new(node_instance)).to be_invalid + end + end + end + end +end diff --git a/spec/lib/gitlab/ci/config/node/validator_spec.rb b/spec/lib/gitlab/ci/config/node/validator_spec.rb new file mode 100644 index 00000000000..ad875d55384 --- /dev/null +++ b/spec/lib/gitlab/ci/config/node/validator_spec.rb @@ -0,0 +1,67 @@ +require 'spec_helper' + +describe Gitlab::Ci::Config::Node::Validator do + let(:validator) { Class.new(described_class) } + let(:validator_instance) { validator.new(node) } + let(:node) { spy('node') } + + shared_examples 'delegated validator' do + context 'when node is valid' do + before do + allow(node).to receive(:test_attribute).and_return('valid value') + end + + it 'validates attribute in node' do + expect(node).to receive(:test_attribute) + expect(validator_instance).to be_valid + end + + it 'returns no errors' do + validator_instance.validate + + expect(validator_instance.full_errors).to be_empty + end + end + + context 'when node is invalid' do + before do + allow(node).to receive(:test_attribute).and_return(nil) + end + + it 'validates attribute in node' do + expect(node).to receive(:test_attribute) + expect(validator_instance).to be_invalid + end + + it 'returns errors' do + validator_instance.validate + + expect(validator_instance.full_errors).not_to be_empty + end + end + end + + describe 'attributes validations' do + before do + validator.class_eval do + validates :test_attribute, presence: true + end + end + + it_behaves_like 'delegated validator' + end + + describe 'interface validations' do + before do + validator.class_eval do + validate do + unless @node.test_attribute == 'valid value' + errors.add(:test_attribute, 'invalid value') + end + end + end + end + + it_behaves_like 'delegated validator' + end +end -- cgit v1.2.1 From a9bd16bd0a44332133d0f9fd859d9ffaba9e262f Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Fri, 17 Jun 2016 11:55:55 +0200 Subject: Rename legacy validation helpers for new ci config --- lib/ci/gitlab_ci_yaml_processor.rb | 2 +- .../ci/config/node/legacy_validation_helpers.rb | 34 ++++++++++++++++++++++ lib/gitlab/ci/config/node/script.rb | 2 +- lib/gitlab/ci/config/node/validation_helpers.rb | 34 ---------------------- 4 files changed, 36 insertions(+), 36 deletions(-) create mode 100644 lib/gitlab/ci/config/node/legacy_validation_helpers.rb delete mode 100644 lib/gitlab/ci/config/node/validation_helpers.rb diff --git a/lib/ci/gitlab_ci_yaml_processor.rb b/lib/ci/gitlab_ci_yaml_processor.rb index e0b89cead06..1a5be9dbc46 100644 --- a/lib/ci/gitlab_ci_yaml_processor.rb +++ b/lib/ci/gitlab_ci_yaml_processor.rb @@ -2,7 +2,7 @@ module Ci class GitlabCiYamlProcessor class ValidationError < StandardError; end - include Gitlab::Ci::Config::Node::ValidationHelpers + include Gitlab::Ci::Config::Node::LegacyValidationHelpers DEFAULT_STAGES = %w(build test deploy) DEFAULT_STAGE = 'test' diff --git a/lib/gitlab/ci/config/node/legacy_validation_helpers.rb b/lib/gitlab/ci/config/node/legacy_validation_helpers.rb new file mode 100644 index 00000000000..970763670c5 --- /dev/null +++ b/lib/gitlab/ci/config/node/legacy_validation_helpers.rb @@ -0,0 +1,34 @@ +module Gitlab + module Ci + class Config + module Node + module LegacyValidationHelpers + private + + def validate_duration(value) + value.is_a?(String) && ChronicDuration.parse(value) + rescue ChronicDuration::DurationParseError + false + end + + def validate_array_of_strings(values) + values.is_a?(Array) && values.all? { |value| validate_string(value) } + end + + def validate_variables(variables) + variables.is_a?(Hash) && + variables.all? { |key, value| validate_string(key) && validate_string(value) } + end + + def validate_string(value) + value.is_a?(String) || value.is_a?(Symbol) + end + + def validate_boolean(value) + value.in?([true, false]) + end + end + end + end + end +end diff --git a/lib/gitlab/ci/config/node/script.rb b/lib/gitlab/ci/config/node/script.rb index 059650d8a52..44490096f28 100644 --- a/lib/gitlab/ci/config/node/script.rb +++ b/lib/gitlab/ci/config/node/script.rb @@ -14,7 +14,7 @@ module Gitlab include Validatable validations do - include ValidationHelpers + include LegacyValidationHelpers validate :array_of_strings diff --git a/lib/gitlab/ci/config/node/validation_helpers.rb b/lib/gitlab/ci/config/node/validation_helpers.rb deleted file mode 100644 index 42ef60244ba..00000000000 --- a/lib/gitlab/ci/config/node/validation_helpers.rb +++ /dev/null @@ -1,34 +0,0 @@ -module Gitlab - module Ci - class Config - module Node - module ValidationHelpers - private - - def validate_duration(value) - value.is_a?(String) && ChronicDuration.parse(value) - rescue ChronicDuration::DurationParseError - false - end - - def validate_array_of_strings(values) - values.is_a?(Array) && values.all? { |value| validate_string(value) } - end - - def validate_variables(variables) - variables.is_a?(Hash) && - variables.all? { |key, value| validate_string(key) && validate_string(value) } - end - - def validate_string(value) - value.is_a?(String) || value.is_a?(Symbol) - end - - def validate_boolean(value) - value.in?([true, false]) - end - end - end - end - end -end -- cgit v1.2.1 From d9ca84015c04d8836c09c3ebb70a8240262b60e8 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Fri, 17 Jun 2016 12:06:48 +0200 Subject: Add first custom validator for new ci config This follows a standard `ActiveModel` pattern of creating a custom validators. We use `ActiveModel::EachValidator` here that reuses methods provided by `LegacyValidationHelpers`. We will remove `LegacyValidationHelpers` on some point in the future, at the later stages of CI configuration refactoring. It may be possible to rewrite custom validators to use format like: `validates :config, array_of: String` --- lib/gitlab/ci/config/node/script.rb | 10 +--------- lib/gitlab/ci/config/node/validator.rb | 1 + lib/gitlab/ci/config/node/validators.rb | 19 +++++++++++++++++++ .../validators/array_of_strings_validator_spec.rb | 0 4 files changed, 21 insertions(+), 9 deletions(-) create mode 100644 lib/gitlab/ci/config/node/validators.rb create mode 100644 spec/lib/gitlab/ci/config/node/validators/array_of_strings_validator_spec.rb diff --git a/lib/gitlab/ci/config/node/script.rb b/lib/gitlab/ci/config/node/script.rb index 44490096f28..c044f5c5e71 100644 --- a/lib/gitlab/ci/config/node/script.rb +++ b/lib/gitlab/ci/config/node/script.rb @@ -14,15 +14,7 @@ module Gitlab include Validatable validations do - include LegacyValidationHelpers - - validate :array_of_strings - - def array_of_strings - unless validate_array_of_strings(self.config) - errors.add(:config, 'should be an array of strings') - end - end + validates :config, array_of_strings: true end def value diff --git a/lib/gitlab/ci/config/node/validator.rb b/lib/gitlab/ci/config/node/validator.rb index 2454cc6d957..02edc9219c3 100644 --- a/lib/gitlab/ci/config/node/validator.rb +++ b/lib/gitlab/ci/config/node/validator.rb @@ -4,6 +4,7 @@ module Gitlab module Node class Validator < SimpleDelegator include ActiveModel::Validations + include Node::Validators def initialize(node) super(node) diff --git a/lib/gitlab/ci/config/node/validators.rb b/lib/gitlab/ci/config/node/validators.rb new file mode 100644 index 00000000000..36d48394a8c --- /dev/null +++ b/lib/gitlab/ci/config/node/validators.rb @@ -0,0 +1,19 @@ +module Gitlab + module Ci + class Config + module Node + module Validators + class ArrayOfStringsValidator < ActiveModel::EachValidator + include LegacyValidationHelpers + + def validate_each(record, attribute, value) + unless validate_array_of_strings(value) + record.errors.add(attribute, 'should be an array of strings') + end + end + end + end + end + end + end +end diff --git a/spec/lib/gitlab/ci/config/node/validators/array_of_strings_validator_spec.rb b/spec/lib/gitlab/ci/config/node/validators/array_of_strings_validator_spec.rb new file mode 100644 index 00000000000..e69de29bb2d -- cgit v1.2.1 From 44b00a1ebbfeaa095343f55f6c12dcbc65b85924 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Fri, 17 Jun 2016 14:51:39 +0200 Subject: Extract CI entry config hash validation to validator --- lib/gitlab/ci/config/node/configurable.rb | 8 +------- lib/gitlab/ci/config/node/validators.rb | 8 ++++++++ .../ci/config/node/validators/array_of_strings_validator_spec.rb | 0 3 files changed, 9 insertions(+), 7 deletions(-) delete mode 100644 spec/lib/gitlab/ci/config/node/validators/array_of_strings_validator_spec.rb diff --git a/lib/gitlab/ci/config/node/configurable.rb b/lib/gitlab/ci/config/node/configurable.rb index 7b428c1362c..374ff71d0f5 100644 --- a/lib/gitlab/ci/config/node/configurable.rb +++ b/lib/gitlab/ci/config/node/configurable.rb @@ -19,13 +19,7 @@ module Gitlab included do validations do - validate :hash_config_value - - def hash_config_value - unless self.config.is_a?(Hash) - errors.add(:config, 'should be a configuration entry hash') - end - end + validates :config, hash: true end end diff --git a/lib/gitlab/ci/config/node/validators.rb b/lib/gitlab/ci/config/node/validators.rb index 36d48394a8c..dc9cdb9a220 100644 --- a/lib/gitlab/ci/config/node/validators.rb +++ b/lib/gitlab/ci/config/node/validators.rb @@ -12,6 +12,14 @@ module Gitlab end end end + + class HashValidator < ActiveModel::EachValidator + def validate_each(record, attribute, value) + unless value.is_a?(Hash) + record.errors.add(attribute, 'should be a configuration entry hash') + end + end + end end end end diff --git a/spec/lib/gitlab/ci/config/node/validators/array_of_strings_validator_spec.rb b/spec/lib/gitlab/ci/config/node/validators/array_of_strings_validator_spec.rb deleted file mode 100644 index e69de29bb2d..00000000000 -- cgit v1.2.1 From 9b777f4187436aed84c52625ca3dbc224f8e3b7a Mon Sep 17 00:00:00 2001 From: Alfredo Sumaran Date: Fri, 17 Jun 2016 19:05:26 -0500 Subject: Pass dropdown instance to toggleLabel callback --- app/assets/javascripts/gl_dropdown.js.coffee | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/app/assets/javascripts/gl_dropdown.js.coffee b/app/assets/javascripts/gl_dropdown.js.coffee index b49bd4565a7..3d8c971b447 100644 --- a/app/assets/javascripts/gl_dropdown.js.coffee +++ b/app/assets/javascripts/gl_dropdown.js.coffee @@ -454,7 +454,7 @@ class GitLabDropdown # Toggle the dropdown label if @options.toggleLabel - @updateLabel() + @updateLabel(selectedObject, el, @) else selectedObject else if el.hasClass(INDETERMINATE_CLASS) @@ -481,7 +481,7 @@ class GitLabDropdown # Toggle the dropdown label if @options.toggleLabel - @updateLabel(selectedObject, el) + @updateLabel(selectedObject, el, @) if value? if !field.length and fieldName @addInput(fieldName, value) @@ -580,8 +580,8 @@ class GitLabDropdown # Scroll the dropdown content up $dropdownContent.scrollTop(listItemTop - dropdownContentTop) - updateLabel: (selected = null, el = null) => - $(@el).find(".dropdown-toggle-text").text @options.toggleLabel(selected, el) + updateLabel: (selected = null, el = null, instance = null) => + $(@el).find(".dropdown-toggle-text").text @options.toggleLabel(selected, el, instance) $.fn.glDropdown = (opts) -> return @.each -> -- cgit v1.2.1 From fcfe87510bc6add78a6be291400df7504c84de54 Mon Sep 17 00:00:00 2001 From: Alfredo Sumaran Date: Fri, 17 Jun 2016 19:06:04 -0500 Subject: Update dropdown text accordingly to selected item --- app/assets/javascripts/issue_status_select.js.coffee | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/app/assets/javascripts/issue_status_select.js.coffee b/app/assets/javascripts/issue_status_select.js.coffee index c5740f27ddd..ed50e2e698f 100644 --- a/app/assets/javascripts/issue_status_select.js.coffee +++ b/app/assets/javascripts/issue_status_select.js.coffee @@ -6,6 +6,13 @@ class @IssueStatusSelect $(el).glDropdown( selectable: true fieldName: fieldName + toggleLabel: (selected, el, instance) => + label = 'Author' + $item = instance.dropdown.find('.is-active') + label = $item.text() if $item.length + label + clicked: (item, $el, e)-> + e.preventDefault() id: (obj, el) -> $(el).data("id") ) -- cgit v1.2.1 From 03a4d6e9a2eef52181a70a68847aa27a2f82dcb2 Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Fri, 17 Jun 2016 12:58:09 +0100 Subject: Made the search bar on emoji menu sticky --- app/assets/javascripts/awards_handler.coffee | 6 ++++-- app/assets/stylesheets/pages/awards.scss | 21 ++++++++++----------- app/views/emojis/index.html.haml | 2 +- 3 files changed, 15 insertions(+), 14 deletions(-) diff --git a/app/assets/javascripts/awards_handler.coffee b/app/assets/javascripts/awards_handler.coffee index 030f1564862..37d0adaa625 100644 --- a/app/assets/javascripts/awards_handler.coffee +++ b/app/assets/javascripts/awards_handler.coffee @@ -341,7 +341,9 @@ class @AwardsHandler for emoji in frequentlyUsedEmojis $(".emoji-menu-content [data-emoji='#{emoji}']").closest('li').clone().appendTo(ul) - $('input.emoji-search').after(ul).after($('
').text('Frequently used')) + $('.emoji-menu-content') + .prepend(ul) + .prepend($('
').text('Frequently used')) @frequentEmojiBlockRendered = true @@ -356,7 +358,7 @@ class @AwardsHandler if term # Generate a search result block - h5 = $('
').text('Search results').addClass('emoji-search') + h5 = $('
').text('Search results') found_emojis = @searchEmojis(term).show() ul = $('