From 7c1d738a1db78dd8943f903ceedfbc0f8a0f0b24 Mon Sep 17 00:00:00 2001 From: Simon Reed Date: Thu, 29 Aug 2019 11:45:28 +0100 Subject: [PATCH] Validations on Map to check for resolved topic conflicts #238 --- .../javascripts/lib/data_manager.coffee | 17 +++++++++++++ .../data_manager/constructs/questions.coffee | 2 +- app/assets/javascripts/lib/resource.coffee | 2 +- .../datasets/modules/show/index.coffee | 11 ++++---- .../javascripts/sections/mapping/index.coffee | 14 +++++------ .../templates/partials/datasets/show.html | 8 +++++- .../templates/partials/instruments/map.html | 5 ++++ app/controllers/cc_questions_controller.rb | 25 ++++++++++++------- app/controllers/instruments_controller.rb | 2 +- app/controllers/variables_controller.rb | 14 ++++++++--- app/models/map.rb | 8 ++++++ app/models/variable.rb | 4 +-- app/views/variables/index.json.jbuilder | 2 ++ app/views/variables/show.json.jbuilder | 2 ++ config/locales/en.yml | 4 +++ lib/linkable.rb | 8 +++--- 16 files changed, 91 insertions(+), 37 deletions(-) diff --git a/app/assets/javascripts/lib/data_manager.coffee b/app/assets/javascripts/lib/data_manager.coffee index 3b547fbd..3586ae56 100755 --- a/app/assets/javascripts/lib/data_manager.coffee +++ b/app/assets/javascripts/lib/data_manager.coffee @@ -360,6 +360,23 @@ data_manager.factory( delete model.suggested_topic model.$update_topic({topic_id: if Number.isInteger(topic_id) then topic_id else null }) + DataManager.addSources = (model, new_sources, x, y)-> + console.log(model) + model.$add_mapping { + sources: + id: new_sources + x: x + y: y + } + + DataManager.addVariables = (model, variables)-> + console.log(model) + model.$add_mapping { + variable_names: variables + x: null + y: null + } + DataManager.getInstrumentStats = (id, cb)-> DataManager.Data.InstrumentStats[id] = {$resolved: false} DataManager.Data.InstrumentStats[id].$promise = InstrumentStats(id) diff --git a/app/assets/javascripts/lib/data_manager/constructs/questions.coffee b/app/assets/javascripts/lib/data_manager/constructs/questions.coffee index d0dfd5ed..d7737704 100755 --- a/app/assets/javascripts/lib/data_manager/constructs/questions.coffee +++ b/app/assets/javascripts/lib/data_manager/constructs/questions.coffee @@ -39,4 +39,4 @@ questions.factory( grid.clearCache() if grid? } ] -) \ No newline at end of file +) diff --git a/app/assets/javascripts/lib/resource.coffee b/app/assets/javascripts/lib/resource.coffee index 23c5c5fd..c5867f2c 100755 --- a/app/assets/javascripts/lib/resource.coffee +++ b/app/assets/javascripts/lib/resource.coffee @@ -82,4 +82,4 @@ resource.factory('GetResource', [ ) sub_promise.then(cb) if typeof cb is 'function' rsrc -]) \ No newline at end of file +]) diff --git a/app/assets/javascripts/sections/datasets/modules/show/index.coffee b/app/assets/javascripts/sections/datasets/modules/show/index.coffee index 5f6d6694..11fa2915 100755 --- a/app/assets/javascripts/sections/datasets/modules/show/index.coffee +++ b/app/assets/javascripts/sections/datasets/modules/show/index.coffee @@ -73,12 +73,11 @@ show.controller( $scope.detectKey = (event, variable, x = null, y = null)-> if event.keyCode == 13 new_sources = event.target.value.split ',' - variable.$add_mapping { - sources: - id: new_sources - x: x - y: y - } + DataManager.addSources(variable, new_sources, x, y).then(-> + $scope.model.orig_topic = $scope.model.topic + , (reason)-> + variable.errors = reason.data.message + ) console.log variable console.log $scope diff --git a/app/assets/javascripts/sections/mapping/index.coffee b/app/assets/javascripts/sections/mapping/index.coffee index d0a34628..d0f343b7 100755 --- a/app/assets/javascripts/sections/mapping/index.coffee +++ b/app/assets/javascripts/sections/mapping/index.coffee @@ -52,13 +52,11 @@ mapping.controller( $scope.detectKey = (event, question, x = null, y = null)-> if event.keyCode == 13 variables = event.target.value.split ',' - question.$add_mapping { - variable_names: variables - x: null - y: null - } - , -> - DataManager.resolveQuestions() + DataManager.addVariables(question, variables).then(-> + $scope.model.orig_topic = $scope.model.topic + , (reason)-> + question.errors = reason.data.message + ) console.log question # $scope.detectKey = (event,question_id)-> @@ -137,7 +135,7 @@ mapping.directive( $scope.model.orig_topic = $scope.model.topic , (reason)-> $scope.model.topic = $scope.model.orig_topic - Flash.add('danger', reason.data.message) + $scope.model.errors = reason.data.message ).finally(-> bsLoadingOverlayService.stop() ) diff --git a/app/assets/javascripts/templates/partials/datasets/show.html b/app/assets/javascripts/templates/partials/datasets/show.html index a8719966..f5dedea9 100755 --- a/app/assets/javascripts/templates/partials/datasets/show.html +++ b/app/assets/javascripts/templates/partials/datasets/show.html @@ -34,8 +34,13 @@

{{dataset.study}} - {{dataset.name}}

- + +
+ +
@@ -85,6 +90,7 @@

{{dataset.study}} - {{dataset.name}}

+ Resolved Topic - {{ variable.resolved_topic.name }}
+
+ +
{{obj.label}}
diff --git a/app/controllers/cc_questions_controller.rb b/app/controllers/cc_questions_controller.rb index 3561f9f6..60bdee32 100755 --- a/app/controllers/cc_questions_controller.rb +++ b/app/controllers/cc_questions_controller.rb @@ -24,17 +24,24 @@ def add_variables variables = @object.instrument.variables.where(name: variable_names) variables.to_a.compact! - variables.each do |variable| - unless @object.variables.find_by_id(variable.id) - if params.has_key?(:x) && params.has_key?(:y) - @object.map.create(variable: variable, x: params[:x].to_i, y: params[:y].to_i) - else - @object.variables << variable + begin + ActiveRecord::Base.transaction do + variables.each do |variable| + unless @object.variables.find_by_id(variable.id) + if params.has_key?(:x) && params.has_key?(:y) + @object.map.create!(variable: variable, x: params[:x].to_i, y: params[:y].to_i) + else + @object.variables << variable + end + end end + @object.save end - end - respond_to do |format| - format.json { render 'show' } + respond_to do |format| + format.json { render 'show' } + end + rescue => e + render json: {message: e.message}, status: :conflict end end diff --git a/app/controllers/instruments_controller.rb b/app/controllers/instruments_controller.rb index 64f81095..5dffe8fe 100755 --- a/app/controllers/instruments_controller.rb +++ b/app/controllers/instruments_controller.rb @@ -78,7 +78,7 @@ def export end def variables - @collection = @object.variables + @collection = @object.variables.includes(:topic, :questions, :question_topics) render 'variables/index' end diff --git a/app/controllers/variables_controller.rb b/app/controllers/variables_controller.rb index fc6899d5..deca3acf 100755 --- a/app/controllers/variables_controller.rb +++ b/app/controllers/variables_controller.rb @@ -19,9 +19,15 @@ def add_sources params[:sources] = JSON.parse(params[:sources]) - @object.add_sources(params[:sources][:id], params[:sources][:x], params[:sources][:y]) - - render 'variables/show' + begin + ActiveRecord::Base.transaction do + @object.add_sources(params[:sources][:id], params[:sources][:x], params[:sources][:y]) + @object.reload + end + render 'variables/show' + rescue => e + render json: {message: e.message}, status: :conflict + end end def remove_source @@ -46,4 +52,4 @@ def collection def set_dataset @dataset = policy_scope(Dataset).includes(variables: [:questions, :src_variables, :der_variables, :topic] ).find(params[:dataset_id]) end -end \ No newline at end of file +end diff --git a/app/models/map.rb b/app/models/map.rb index c497e033..5efeb814 100755 --- a/app/models/map.rb +++ b/app/models/map.rb @@ -12,4 +12,12 @@ class Map < ApplicationRecord belongs_to :source, polymorphic: true # Every mapping has a destination {Variable} belongs_to :variable + + # Validate to stop topic conflict between Variable(s) and CcQuestion + validate :resolved_topic_conflict + + def resolved_topic_conflict + return if source.resolved_topic.nil? || variable.resolved_topic.nil? + errors.add(:topic, I18n.t('activerecord.errors.models.map.attributes.resolved_topic.variables_conflict', variable: variable, source: source, variable_topic: variable.resolved_topic, source_topic: source.resolved_topic)) + end end diff --git a/app/models/variable.rb b/app/models/variable.rb index ab8e5077..8ffb23c4 100755 --- a/app/models/variable.rb +++ b/app/models/variable.rb @@ -72,14 +72,12 @@ def resolved_topic def add_sources(source_labels, x = nil, y = nil) sources = self.var_type == 'Normal' ? find_by_label_from_possible_questions(source_labels) : self.dataset.variables.find_by_name(source_labels) [*sources].compact.each do |source| - if self.maps.create ({ + self.maps.create! ({ variable: self, source: source, x: x, y: y }) - self.strand + source.strand - end end end diff --git a/app/views/variables/index.json.jbuilder b/app/views/variables/index.json.jbuilder index f784c9e0..6d528dbe 100755 --- a/app/views/variables/index.json.jbuilder +++ b/app/views/variables/index.json.jbuilder @@ -7,4 +7,6 @@ json.array!(@collection) do |variable| end json.used_bys variable.der_variables, :id, :name, :label, :var_type json.topic variable.topic, :id, :code, :name, :parent_id unless variable.topic.nil? + json.errors variable.errors.full_messages.to_sentence unless variable.valid? + json.resolved_topic variable.resolved_topic end diff --git a/app/views/variables/show.json.jbuilder b/app/views/variables/show.json.jbuilder index 6edcc137..883db048 100755 --- a/app/views/variables/show.json.jbuilder +++ b/app/views/variables/show.json.jbuilder @@ -7,3 +7,5 @@ else end json.used_bys @object.der_variables, :id, :name, :label, :var_type json.topic @object.topic, :id, :code, :name, :parent_id unless @object.topic.nil? +json.resolved_topic @object.resolved_topic +json.errors @object.errors.full_messages.to_sentence unless @object.valid? diff --git a/config/locales/en.yml b/config/locales/en.yml index 04963d47..d23c8837 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -34,3 +34,7 @@ en: conflict: "conflict, already associated to topic %{topics} through variables %{variables}" resolved_topic: variables_conflict: "conflict, you cannot assign variable '%{new_variables}'' to this question because it has a topic of '%{new_topics}'' and your existing variables are using topic '%{existing_topic}'" + map: + attributes: + resolved_topic: + variables_conflict: "conflict, you cannot assign variable '%{variable}' to '%{source}' because '%{variable}' has a topic of '%{variable_topic}'' while '%{source}' is using topic '%{source_topic}'" diff --git a/lib/linkable.rb b/lib/linkable.rb index 5f93851b..2cd93118 100644 --- a/lib/linkable.rb +++ b/lib/linkable.rb @@ -25,8 +25,10 @@ def set_topic topic = Topic.find_by_id params[:topic_id] begin - @object.topic = topic - @object.save! + ActiveRecord::Base.transaction do + @object.topic = topic + @object.save! + end yield if block_given? @@ -34,7 +36,7 @@ def set_topic rescue Exceptions::TopicConflictError render json: {message: 'Could not set topic as it would cause a conflict.'}, status: :conflict rescue => e - render json: e, status: :bad_request + render json: {message: e.message}, status: :conflict end end