From b12f6f1b92c36b145366dc6aff43d4c9a535406c Mon Sep 17 00:00:00 2001 From: zmagod Date: Mon, 9 Apr 2018 12:50:25 +0200 Subject: [PATCH 1/5] set limit on maximum number of dropdown list items [fixes SCI-2259] --- app/models/repository_list_item.rb | 2 ++ app/validators/repository_list_item_validator.rb | 9 +++++++++ app/views/repositories/_parse_records_modal.html.erb | 4 ++-- config/initializers/constants.rb | 2 ++ config/locales/en.yml | 2 +- spec/models/repository_list_item_spec.rb | 4 ++-- 6 files changed, 18 insertions(+), 5 deletions(-) create mode 100644 app/validators/repository_list_item_validator.rb diff --git a/app/models/repository_list_item.rb b/app/models/repository_list_item.rb index e3dbeeb5f..99f66c597 100644 --- a/app/models/repository_list_item.rb +++ b/app/models/repository_list_item.rb @@ -1,4 +1,5 @@ class RepositoryListItem < ApplicationRecord + include ActiveModel::Validations has_many :repository_list_values, inverse_of: :repository_list_item belongs_to :repository, inverse_of: :repository_list_items belongs_to :repository_column, inverse_of: :repository_list_items @@ -12,4 +13,5 @@ class RepositoryListItem < ApplicationRecord presence: true, uniqueness: { scope: :repository_column, case_sensitive: false }, length: { maximum: Constants::TEXT_MAX_LENGTH } + validates_with RepositoryListItemValidator end diff --git a/app/validators/repository_list_item_validator.rb b/app/validators/repository_list_item_validator.rb new file mode 100644 index 000000000..740bb9afc --- /dev/null +++ b/app/validators/repository_list_item_validator.rb @@ -0,0 +1,9 @@ +class RepositoryListItemValidator < ActiveModel::Validator + def validate(record) + return unless record.repository_column + items_length = record.repository_column.repository_list_items.size + if items_length >= Constants::REPOSITORY_LIST_ITEMS_PER_COLUMN + record.errors[:items_limit] << 'Maximum number of list items reached!' + end + end +end diff --git a/app/views/repositories/_parse_records_modal.html.erb b/app/views/repositories/_parse_records_modal.html.erb index 44c398a16..14536ce18 100644 --- a/app/views/repositories/_parse_records_modal.html.erb +++ b/app/views/repositories/_parse_records_modal.html.erb @@ -64,8 +64,8 @@ diff --git a/config/initializers/constants.rb b/config/initializers/constants.rb index 54a793cc1..bface4e3e 100644 --- a/config/initializers/constants.rb +++ b/config/initializers/constants.rb @@ -868,6 +868,8 @@ class Constants EXPORTABLE_ZIP_EXPIRATION_DAYS = 7 + REPOSITORY_LIST_ITEMS_PER_COLUMN = 500 + # Very basic regex to check for validity of emails BASIC_EMAIL_REGEX = URI::MailTo::EMAIL_REGEXP diff --git a/config/locales/en.yml b/config/locales/en.yml index 31847fa5c..e5410189b 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -966,7 +966,7 @@ en: delete: "Delete column" modal_parse: title: 'Import items' - warning_1: 'Be careful when importing into Dropdown column/s! Each new unique cell value from the file will create a new Dropdown option. This could result in large amounts of Dropdown options.' + warning_1: 'Be careful when importing into Dropdown column/s! Each new unique cell value from the file will create a new Dropdown option. Maximum nr. of Dropdown options is %{limit}.' warning_2: 'Importing into file columns is not supported.' modal_import: title: 'Import items' diff --git a/spec/models/repository_list_item_spec.rb b/spec/models/repository_list_item_spec.rb index dde1ae856..40f9937e8 100644 --- a/spec/models/repository_list_item_spec.rb +++ b/spec/models/repository_list_item_spec.rb @@ -22,14 +22,14 @@ RSpec.describe RepositoryListItem, type: :model do end describe 'Validations' do + let!(:user) { create :user } + let!(:repository_one) { create :repository } it { should validate_presence_of(:data) } it do should validate_length_of(:data).is_at_most(Constants::TEXT_MAX_LENGTH) end context 'has a uniq data scoped on repository column' do - let!(:user) { create :user } - let!(:repository_one) { create :repository } let!(:repository_column) do create :repository_column, name: 'My column', repository: repository_one end From e4b49e62a338d2a430039abb93963898258fb08c Mon Sep 17 00:00:00 2001 From: zmagod Date: Tue, 10 Apr 2018 15:45:51 +0200 Subject: [PATCH 2/5] fixes import limit --- .../repository_columns_controller.rb | 6 ++++ app/models/repository_list_item.rb | 2 -- .../repository_import_parser/importer.rb | 33 +++++++++++++++++-- .../list_items_column.rb | 10 ++++++ .../repository_cell_value_resolver.rb | 24 +++++++++----- .../repository_list_item_validator.rb | 9 ----- .../repository_cell_value_resolver_spec.rb | 4 +-- 7 files changed, 65 insertions(+), 23 deletions(-) create mode 100644 app/utilities/repository_import_parser/list_items_column.rb delete mode 100644 app/validators/repository_list_item_validator.rb diff --git a/app/controllers/repository_columns_controller.rb b/app/controllers/repository_columns_controller.rb index 6c02dc3cf..216d7312f 100644 --- a/app/controllers/repository_columns_controller.rb +++ b/app/controllers/repository_columns_controller.rb @@ -162,7 +162,9 @@ class RepositoryColumnsController < ApplicationController def generate_repository_list_items(item_names) return unless @repository_column.data_type == 'RepositoryListValue' + column_items = @repository_column.repository_list_items.size item_names.split(',').uniq.each do |name| + next if column_items >= Constants::REPOSITORY_LIST_ITEMS_PER_COLUMN RepositoryListItem.create( repository: @repository, repository_column: @repository_column, @@ -170,11 +172,13 @@ class RepositoryColumnsController < ApplicationController created_by: current_user, last_modified_by: current_user ) + column_items += 1 end end def update_repository_list_items(item_names) return unless @repository_column.data_type == 'RepositoryListValue' + column_items = @repository_column.repository_list_items.size items_list = item_names.split(',').uniq existing = @repository_column.repository_list_items.pluck(:data) existing.each do |name| @@ -191,6 +195,7 @@ class RepositoryColumnsController < ApplicationController end items_list.each do |name| next if @repository_column.repository_list_items.find_by_data(name) + next if column_items >= Constants::REPOSITORY_LIST_ITEMS_PER_COLUMN RepositoryListItem.create( repository: @repository, repository_column: @repository_column, @@ -198,6 +203,7 @@ class RepositoryColumnsController < ApplicationController created_by: current_user, last_modified_by: current_user ) + column_items += 1 end end end diff --git a/app/models/repository_list_item.rb b/app/models/repository_list_item.rb index 99f66c597..e3dbeeb5f 100644 --- a/app/models/repository_list_item.rb +++ b/app/models/repository_list_item.rb @@ -1,5 +1,4 @@ class RepositoryListItem < ApplicationRecord - include ActiveModel::Validations has_many :repository_list_values, inverse_of: :repository_list_item belongs_to :repository, inverse_of: :repository_list_items belongs_to :repository_column, inverse_of: :repository_list_items @@ -13,5 +12,4 @@ class RepositoryListItem < ApplicationRecord presence: true, uniqueness: { scope: :repository_column, case_sensitive: false }, length: { maximum: Constants::TEXT_MAX_LENGTH } - validates_with RepositoryListItemValidator end diff --git a/app/utilities/repository_import_parser/importer.rb b/app/utilities/repository_import_parser/importer.rb index c3123f9a5..e244fc80f 100644 --- a/app/utilities/repository_import_parser/importer.rb +++ b/app/utilities/repository_import_parser/importer.rb @@ -53,6 +53,7 @@ module RepositoryImportParser def import_rows! errors = false + column_items = [] @rows.each do |row| # Skip empty rows next if row.empty? @@ -71,16 +72,28 @@ module RepositoryImportParser end row_cell_values = [] - row.each.with_index do |value, index| column = @columns[index] + size = 0 if column && value.present? + if column.data_type == 'RepositoryListValue' + current_items_column = get_items_column(column_items, column) + size = current_items_column.list_items_number + end # uses RepositoryCellValueResolver to retrieve the correct value cell_value_resolver = RepositoryImportParser::RepositoryCellValueResolver.new( - column, @user, @repository + column, + @user, + @repository, + size ) cell_value = cell_value_resolver.get_value(value, record_row) + if column.data_type == 'RepositoryListValue' + current_items_column.list_items_number = + cell_value_resolver.column_list_items_size + end + next if cell_value.nil? # checks the case if we reach items limit unless cell_value.valid? errors = true raise ActiveRecord::Rollback @@ -125,5 +138,21 @@ module RepositoryImportParser ).failed_instances.any? true end + + def get_items_column(list, column) + current_column = nil + list.each do |element| + current_column = element if element.column == column + end + unless current_column + new_column = RepositoryImportParser::ListItemsColumn.new( + column, + column.repository_list_items.size + ) + list << new_column + return new_column + end + current_column + end end end diff --git a/app/utilities/repository_import_parser/list_items_column.rb b/app/utilities/repository_import_parser/list_items_column.rb new file mode 100644 index 000000000..dc9a0a76b --- /dev/null +++ b/app/utilities/repository_import_parser/list_items_column.rb @@ -0,0 +1,10 @@ +module RepositoryImportParser + class ListItemsColumn + attr_accessor :column, :list_items_number + + def initialize(column, list_items_number) + @column = column + @list_items_number = list_items_number + end + end +end diff --git a/app/utilities/repository_import_parser/repository_cell_value_resolver.rb b/app/utilities/repository_import_parser/repository_cell_value_resolver.rb index 2ae63e988..6cb6c9530 100644 --- a/app/utilities/repository_import_parser/repository_cell_value_resolver.rb +++ b/app/utilities/repository_import_parser/repository_cell_value_resolver.rb @@ -4,10 +4,12 @@ # it to the repository_row module RepositoryImportParser class RepositoryCellValueResolver - def initialize(column, user, repository) + attr_reader :column_list_items_size + def initialize(column, user, repository, size) @column = column @user = user @repository = repository + @column_list_items_size = size end def get_value(value, record_row) @@ -30,6 +32,7 @@ module RepositoryImportParser def new_repository_list_value(value, record_row) list_item = @column.repository_list_items.find_by_data(value) list_item ||= create_repository_list_item(value) + return unless list_item RepositoryListValue.new( created_by: @user, last_modified_by: @user, @@ -42,13 +45,18 @@ module RepositoryImportParser end def create_repository_list_item(value) - RepositoryListItem.create( - data: value, - created_by: @user, - last_modified_by: @user, - repository_column: @column, - repository: @repository - ) + if @column_list_items_size >= Constants::REPOSITORY_LIST_ITEMS_PER_COLUMN + return + end + item = RepositoryListItem.new(data: value, + created_by: @user, + last_modified_by: @user, + repository_column: @column, + repository: @repository) + if item.save + @column_list_items_size += 1 + return item + end end end end diff --git a/app/validators/repository_list_item_validator.rb b/app/validators/repository_list_item_validator.rb deleted file mode 100644 index 740bb9afc..000000000 --- a/app/validators/repository_list_item_validator.rb +++ /dev/null @@ -1,9 +0,0 @@ -class RepositoryListItemValidator < ActiveModel::Validator - def validate(record) - return unless record.repository_column - items_length = record.repository_column.repository_list_items.size - if items_length >= Constants::REPOSITORY_LIST_ITEMS_PER_COLUMN - record.errors[:items_limit] << 'Maximum number of list items reached!' - end - end -end diff --git a/spec/utilities/repository_import_parser/repository_cell_value_resolver_spec.rb b/spec/utilities/repository_import_parser/repository_cell_value_resolver_spec.rb index e213002f2..85abfa928 100644 --- a/spec/utilities/repository_import_parser/repository_cell_value_resolver_spec.rb +++ b/spec/utilities/repository_import_parser/repository_cell_value_resolver_spec.rb @@ -28,7 +28,7 @@ describe RepositoryImportParser::RepositoryCellValueResolver do context 'RepositoryListValue' do let(:subject) do RepositoryImportParser::RepositoryCellValueResolver.new( - sample_group_column, user, repository + sample_group_column, user, repository, 0 ) end @@ -51,7 +51,7 @@ describe RepositoryImportParser::RepositoryCellValueResolver do context 'RepositoryTextValue' do let(:subject) do RepositoryImportParser::RepositoryCellValueResolver.new( - custom_column, user, repository + custom_column, user, repository, 0 ) it 'returns a valid RepositoryTextValue object' do From 23653d1d95f5152a15a0fa5812ebe4d3a6bfae99 Mon Sep 17 00:00:00 2001 From: zmagod Date: Wed, 11 Apr 2018 09:29:08 +0200 Subject: [PATCH 3/5] refactor --- app/utilities/repository_import_parser/importer.rb | 2 +- app/utilities/repository_import_parser/list_items_column.rb | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/app/utilities/repository_import_parser/importer.rb b/app/utilities/repository_import_parser/importer.rb index e244fc80f..0274ec295 100644 --- a/app/utilities/repository_import_parser/importer.rb +++ b/app/utilities/repository_import_parser/importer.rb @@ -142,7 +142,7 @@ module RepositoryImportParser def get_items_column(list, column) current_column = nil list.each do |element| - current_column = element if element.column == column + current_column = element if element.has_column? column end unless current_column new_column = RepositoryImportParser::ListItemsColumn.new( diff --git a/app/utilities/repository_import_parser/list_items_column.rb b/app/utilities/repository_import_parser/list_items_column.rb index dc9a0a76b..b7bd1167b 100644 --- a/app/utilities/repository_import_parser/list_items_column.rb +++ b/app/utilities/repository_import_parser/list_items_column.rb @@ -6,5 +6,9 @@ module RepositoryImportParser @column = column @list_items_number = list_items_number end + + def has_column?(column) + @column == column + end end end From ce52eb851b2e01d0302c27a221516ab360264f42 Mon Sep 17 00:00:00 2001 From: zmagod Date: Thu, 19 Apr 2018 14:02:42 +0200 Subject: [PATCH 4/5] add validation to dropdown items --- .../repository_columns/index.js.erb | 12 ++- .../repository_columns_controller.rb | 88 ++++++++++++------- config/locales/en.yml | 1 + 3 files changed, 68 insertions(+), 33 deletions(-) diff --git a/app/assets/javascripts/repository_columns/index.js.erb b/app/assets/javascripts/repository_columns/index.js.erb index 474c4141b..89c8fdcf8 100644 --- a/app/assets/javascripts/repository_columns/index.js.erb +++ b/app/assets/javascripts/repository_columns/index.js.erb @@ -189,8 +189,16 @@ }).on('ajax:error', function(e, xhr) { animateSpinner(null, false); if (modalID) { - var field = { "name": xhr.responseJSON.message } - $(form).renderFormErrors('repository_column', field, true, e); + if(xhr.responseJSON.message.hasOwnProperty('repository_list_items')) { + var message = xhr.responseJSON.message['repository_list_items']; + $('.dnd-error').remove(); + $('.bootstrap-tagsinput').after( + "" + message + "" + ); + } else { + var field = { "name": xhr.responseJSON.message } + $(form).renderFormErrors('repository_column', field, true, e); + } } else { HelperModule.flashAlertMsg(xhr.responseJSON.message, 'danger'); } diff --git a/app/controllers/repository_columns_controller.rb b/app/controllers/repository_columns_controller.rb index 216d7312f..d4d5cb241 100644 --- a/app/controllers/repository_columns_controller.rb +++ b/app/controllers/repository_columns_controller.rb @@ -28,28 +28,35 @@ class RepositoryColumnsController < ApplicationController @repository_column.created_by = current_user respond_to do |format| - if @repository_column.save - generate_repository_list_items(params[:list_items]) - format.json do - render json: { - id: @repository_column.id, - name: escape_input(@repository_column.name), - message: t('libraries.repository_columns.create.success_flash', - name: @repository_column.name), - edit_url: - edit_repository_repository_column_path(@repository, - @repository_column), - update_url: - repository_repository_column_path(@repository, - @repository_column), - destroy_html_url: - repository_columns_destroy_html_path(@repository, - @repository_column) - }, - status: :ok - end - else - format.json do + format.json do + if @repository_column.save + if generate_repository_list_items(params[:list_items]) + render json: { + id: @repository_column.id, + name: escape_input(@repository_column.name), + message: t('libraries.repository_columns.create.success_flash', + name: @repository_column.name), + edit_url: + edit_repository_repository_column_path(@repository, + @repository_column), + update_url: + repository_repository_column_path(@repository, + @repository_column), + destroy_html_url: + repository_columns_destroy_html_path(@repository, + @repository_column) + }, + status: :ok + else + render json: { + message: { + repository_list_items: + t('libraries.repository_columns.repository_list_items_limit', + limit: Constants::REPOSITORY_LIST_ITEMS_PER_COLUMN) + } + }, status: :unprocessable_entity + end + else render json: { message: @repository_column.errors.full_messages }, status: :unprocessable_entity end @@ -74,13 +81,22 @@ class RepositoryColumnsController < ApplicationController format.json do @repository_column.update_attributes(repository_column_params) if @repository_column.save - update_repository_list_items(params[:list_items]) - render json: { - id: @repository_column.id, - name: escape_input(@repository_column.name), - message: t('libraries.repository_columns.update.success_flash', - name: @repository_column.name) - }, status: :ok + if update_repository_list_items(params[:list_items]) + render json: { + id: @repository_column.id, + name: escape_input(@repository_column.name), + message: t('libraries.repository_columns.update.success_flash', + name: @repository_column.name) + }, status: :ok + else + render json: { + message: { + repository_list_items: + t('libraries.repository_columns.repository_list_items_limit', + limit: Constants::REPOSITORY_LIST_ITEMS_PER_COLUMN) + } + }, status: :unprocessable_entity + end else render json: { message: @repository_column.errors.full_messages }, status: :unprocessable_entity @@ -163,8 +179,12 @@ class RepositoryColumnsController < ApplicationController def generate_repository_list_items(item_names) return unless @repository_column.data_type == 'RepositoryListValue' column_items = @repository_column.repository_list_items.size + success = true item_names.split(',').uniq.each do |name| - next if column_items >= Constants::REPOSITORY_LIST_ITEMS_PER_COLUMN + if column_items >= Constants::REPOSITORY_LIST_ITEMS_PER_COLUMN + success = false + next + end RepositoryListItem.create( repository: @repository, repository_column: @repository_column, @@ -174,6 +194,7 @@ class RepositoryColumnsController < ApplicationController ) column_items += 1 end + success end def update_repository_list_items(item_names) @@ -193,9 +214,13 @@ class RepositoryColumnsController < ApplicationController list_item_id ).destroy_all end + success = true items_list.each do |name| next if @repository_column.repository_list_items.find_by_data(name) - next if column_items >= Constants::REPOSITORY_LIST_ITEMS_PER_COLUMN + if column_items >= Constants::REPOSITORY_LIST_ITEMS_PER_COLUMN + success = false + next + end RepositoryListItem.create( repository: @repository, repository_column: @repository_column, @@ -205,5 +230,6 @@ class RepositoryColumnsController < ApplicationController ) column_items += 1 end + success end end diff --git a/config/locales/en.yml b/config/locales/en.yml index e5410189b..bb5b31c05 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -1007,6 +1007,7 @@ en: repository_columns: head_title: '%{repository} | Manage Columns' + repository_list_items_limit: "Dropdown items limit reached max. %{limit}" update: success_flash: "Column %{name} was successfully updated." create: From f555e8cea4eec627cc08345cc281aa73f822441f Mon Sep 17 00:00:00 2001 From: zmagod Date: Thu, 19 Apr 2018 15:32:14 +0200 Subject: [PATCH 5/5] fix error message --- app/assets/javascripts/repository_columns/index.js.erb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/assets/javascripts/repository_columns/index.js.erb b/app/assets/javascripts/repository_columns/index.js.erb index 89c8fdcf8..34f53915b 100644 --- a/app/assets/javascripts/repository_columns/index.js.erb +++ b/app/assets/javascripts/repository_columns/index.js.erb @@ -192,7 +192,7 @@ if(xhr.responseJSON.message.hasOwnProperty('repository_list_items')) { var message = xhr.responseJSON.message['repository_list_items']; $('.dnd-error').remove(); - $('.bootstrap-tagsinput').after( + $('#manageRepositoryColumn ').find('.bootstrap-tagsinput').after( "" + message + "" ); } else {