ANW-1474 refactor agent required field customization form

common/jsonmodel_i18n_mixin.rb

@@ -1,4 +1,3 @@
-
 module JSONModelI18nMixin
 
   def t(*args)
@@ -70,7 +69,7 @@ def translate_exception_message(msg, path = nil)
                when /Username '(.*)' is already in use/
                  [:username_already_in_use, {:username => $1}]
                else
-                 [msg.downcase.gsub(/[\s,':]/, '_')]
+                 [msg.to_s.downcase.gsub(/[\s,':]/, '_')]
                end
 
     key, vars = *msg_data

common/locales/en.yml

@@ -2404,6 +2404,7 @@ en:
     subject_must_be_unique: Subject records cannot be identical
     subject_heading_identifier_must_be_unique_within_source: Subject heading identifier must be unique within source
     missing_required_property: Property is required but was missing
+    missing_required_subrecord: Subrecord is required but was missing
     missing_property: Property was missing
     not_a_valid_date: Not a valid date
     must_not_be_before_begin: must not be before begin
@@ -3044,4 +3045,4 @@ en:
     title: "%{name} [%{depth}d, %{height}h, %{width}w %{dimension_units}] extent measured by %{extent_dimension}"
     new_title: New Container Profile
   location_profile:
-    new_title: New Location Profile
+    new_title: New Location Profile

frontend/Gemfile

@@ -31,6 +31,7 @@ gem 'atomic', '= 1.0.1'
 gem 'jruby-jars', '= 9.2.20.1'
 
 group :test do
+  gem 'rails-controller-testing'
   gem 'axe-core-rspec'
   gem 'dumb_delegator', '~> 0.8'
   gem 'rspec', '~> 3.6.0'

frontend/Gemfile.lock

@@ -212,6 +212,10 @@ GEM
       bundler (>= 1.3.0)
       railties (= 5.2.5)
       sprockets-rails (>= 2.0.0)
+    rails-controller-testing (1.0.5)
+      actionpack (>= 5.0.1.rc1)
+      actionview (>= 5.0.1.rc1)
+      activesupport (>= 5.0.1.rc1)
     rails-dom-testing (2.0.3)
       activesupport (>= 4.2.0)
       nokogiri (>= 1.6)
@@ -354,6 +358,7 @@ DEPENDENCIES
   pry-nav
   pry-rails
   rails (= 5.2.5)
+  rails-controller-testing
   rspec (~> 3.6.0)
   rspec-benchmark
   rspec-rails

frontend/app/controllers/agents_controller.rb

@@ -7,7 +7,6 @@ class AgentsController < ApplicationController
                       'manage_repository' => [:defaults, :update_defaults, :required, :update_required]
 
   before_action :assign_types
-  before_action :set_structured_date_type, only: [:create, :update]
   before_action :get_required, only: [:new, :create, :required]
 
   include ExportHelper
@@ -42,9 +41,7 @@ def new
     end
 
     begin
-      if @required.class == RequiredFields
-        @agent.update_concat(@required.values)
-      end
+      @agent.update_concat(@required.values)
     rescue Exception => e
       flash[:error] = e.message
       redirect_to controller: :agents, action: :required
@@ -66,20 +63,18 @@ def edit
   end
 
   def create
-    required_values = (@required.values if @required.class == RequiredFields)
+    required_values = @required.values
     handle_crud(instance: :agent,
                 model: JSONModel(@agent_type),
                 required: required_values,
                 find_opts: find_opts,
+                before_hooks: [method(:set_structured_date_type)],
                 on_invalid: lambda {
                   @required = RequiredFields.get @agent_type.to_s
-                  @required = {} if @required.nil?
-                  if @required.class == RequiredFields
-                    @agent.update_concat(@required.values)
-                  end
+                  @agent.update_concat(@required.values)
+
                   ensure_auth_and_display
                   return render_aspace_partial partial: 'agents/new' if inline?
-
                   return render action: :new
                 },
                 on_valid: lambda  { |id|
@@ -106,6 +101,7 @@ def update
     handle_crud(instance: :agent,
                 model: JSONModel(@agent_type),
                 obj: JSONModel(@agent_type).find(params[:id], find_opts),
+                before_hooks: [method(:set_structured_date_type)],
                 on_invalid: lambda {
                   if @agent.names.empty?
                     @agent.names = [@name_type.new._always_valid!]
@@ -191,22 +187,34 @@ def update_defaults
 
   def required
     @agent = JSONModel(@agent_type).new({ agent_type: @agent_type })._always_valid!
-
-    @agent.update(@required.form_values) if @required.class == RequiredFields
-
+    @agent.update(@required.form_values)
     render 'required'
   end
 
   def update_required
+    processed_params = cleanup_params_for_schema(
+      params['agent'],
+      JSONModel(@agent_type).schema
+    )
+
+    # this bears explanation: we want to infer subrecord requirement
+    # if a field on the subrecord is required. We look in the fields for
+    # the JSONModel type, which is what we now assign to any required field
+    # or to 'jsonmodel_type' to indicate the whole record ("REQ" is deprecated.)
+    # someday the required_fields table should be migrated to a more expressive
+    # format
+    processed_params.each do |key, defn|
+      next unless defn.is_a?(Array)
+      require_keys = defn.map { |h| h.keys }.flatten
+      likely_type = defn.map { |h| h.values }.flatten.reject { |v| v == "REQ"}.uniq
+      unless likely_type.empty? || require_keys.include?("jsonmodel_type")
+        defn << { "jsonmodel_type" => likely_type.first }
+      end
+    end
     RequiredFields.from_hash({
                                'record_type' => @agent_type.to_s,
                                'lock_version' => params['agent'].delete('lock_version'),
-                               'required' => cleanup_params_for_schema(
-                                 params['agent'],
-                                 JSONModel(@agent_type).schema
-                               )
-                             }).save
-
+                               'required' => processed_params}).save
     flash[:success] = I18n.t('required_fields.messages.required_fields_updated')
     redirect_to controller: :agents, action: :required
   rescue Exception => e
@@ -312,7 +320,6 @@ def name_type_for_agent_type(agent_type)
 
   def get_required
     @required = RequiredFields.get @agent_type.to_s
-    @required = {} if @required.nil?
   end
 
   def assign_types
@@ -323,8 +330,8 @@ def assign_types
     @name_type = name_type_for_agent_type(@agent_type)
   end
 
-  def set_structured_date_type
-    params['agent']['dates_of_existence']&.each do |_key, label|
+  def set_structured_date_type(agent_hash)
+    agent_hash['dates_of_existence']&.each do |label|
       if label['structured_date_single']
         label['date_type_structured'] = 'single'
       elsif label['structured_date_range']
@@ -333,11 +340,9 @@ def set_structured_date_type
         label['date_type_structured'] = 'Add or update either a single or ranged date subrecord to set'
       end
     end
-
-    params['agent']['names']&.each do |_key, name|
+    agent_hash['names']&.each do |name|
       next unless name['use_dates']
-
-      name['use_dates'].each do |_key, label|
+      name['use_dates'].each do |label|
         if label['structured_date_single']
           label['date_type_structured'] = 'single'
         elsif label['structured_date_range']
@@ -348,7 +353,7 @@ def set_structured_date_type
       end
     end
 
-    params['agent']['related_agents']&.each do |_key, rel|
+    agent_hash['related_agents']&.each do |rel|
       if rel['dates']
         if rel['dates']['structured_date_single']
           rel['dates']['date_type_structured'] = 'single'

frontend/app/controllers/application_controller.rb

@@ -101,6 +101,10 @@ def handle_crud(opts)
 
       instance = cleanup_params_for_schema(params[opts[:instance]], model.schema)
 
+      if opts[:before_hooks]
+        opts[:before_hooks].each { |hook| hook.call(instance) }
+      end
+
       if opts[:replace] || opts[:replace].nil?
         obj.replace(instance)
       elsif opts[:copy]
@@ -112,15 +116,18 @@ def handle_crud(opts)
 
       if opts[:required]
         required = opts[:required]
+        check_required_subrecords(required, obj)
         missing, min_items = compare(required, obj)
         #render :plain => missing
         if !missing.nil?
           missing.each do |field_name|
-            obj.add_error(field_name, "Property is required but was missing")
+            obj.add_error(field_name, :missing_required_property)
           end
         end
         if !min_items.nil?
           min_items.each do |item|
+            # Do not alter this! It mimics a built-in json-schema message and gets
+            # i18nized later on
             message = "At least #{item['num']} item(s) is required"
             obj.add_error(item['name'], message)
           end
@@ -290,10 +297,7 @@ def compare(required, obj)
     min_items = []
     required.keys.each do |key|
       if required[key].is_a? Array and obj[key].is_a? Array
-        if required[key].length > obj[key].length
-          min_items << {"name" => key, "num" => required[key].length}
-        elsif required[key].length === obj[key].length
-
+        if required[key].length === obj[key].length
           required[key].zip(obj[key]).each_with_index do |(required_a, obj_a), index|
             required_a.keys.each do |nested_key|
               if required_a[nested_key].is_a? Array and obj_a[nested_key].is_a? Array
@@ -690,7 +694,6 @@ def cleanup_params_for_schema(params_hash, schema)
       end
     end
 
-
     JSONSchemaUtils.map_hash_with_schema(params_hash,
                                          schema,
                                          [fix_arrays,
@@ -831,7 +834,15 @@ def controller_supports_current_record?
     self.method(:current_record).owner != ApplicationController
   end
 
+  def check_required_subrecords(required, obj)
+    required.each do |subrecord_field, requirements_defn|
+      next unless requirements_defn.is_a?(Array)
+      if obj[subrecord_field].empty?
+        obj.add_error(subrecord_field, :missing_required_subrecord)
+      end
+    end
+  end
+
   helper_method :current_record
   helper_method :'controller_supports_current_record?'
-
 end

frontend/app/helpers/aspace_form_helper.rb

@@ -254,7 +254,8 @@ def exceptions_for_js(exceptions)
 
 
     def id_for_javascript(name)
-      "#{form_top}#{name.split("/").collect {|a| "[#{a}]"}.join}".gsub(/[\[\]\/]/, "_")
+      path = name.split("/").collect {|a| "[#{a}]"}.join
+      "#{form_top}#{path}".gsub(/[\[\]\/]/, "_")
     end
 
 
@@ -335,12 +336,6 @@ def label_and_boolean(name, opts = {}, default = false, force_checked = false)
       label_with_field(name, checkbox(name, opts, default, force_checked), opts)
     end
 
-    def label_and_req_boolean(name, opts = {}, default = false, force_checked = false)
-      opts[:col_size] = 1
-      opts[:controls_class] = "req_checkbox"
-      label_with_field(name, req_checkbox(name, opts, default, force_checked), opts)
-    end
-
     def label_and_readonly(name, default = "", opts = {})
       value = obj[name]
       if !(value.is_a? String)
@@ -670,13 +665,6 @@ def oai_config_sponsor_set_names_field(set_json, opts = {})
       return html.html_safe
     end
 
-    def req_checkbox(name, opts = {}, default = true, force_checked = false)
-      options = {:id => "#{id_for(name)}", :type => "checkbox", :name => path(name), :value => "REQ"}
-      options[:checked] = "checked" if force_checked or (obj[name] === true) or (obj[name].is_a? String and obj[name].start_with?("true")) or (obj[name] === "REQ") or (obj[name].nil? and default)
-
-      @forms.tag("input", options.merge(opts), false, false)
-    end
-
     def merge_checkbox(name, opts = {}, default = false, force_checked = false)
       options = {:id => "#{id_for(name)}", :type => "checkbox", :name => path(name), :value => "REPLACE"}
       options[:checked] = "checked" if force_checked or (obj[name] === true) or (obj[name].is_a? String and obj[name].start_with?("true")) or (obj[name] === "REPLACE") or (obj[name].nil? and default)
@@ -750,7 +738,7 @@ def label_with_field(name, field_html, opts = {})
       end
 
       control_group_classes << "required" if required == true
-      control_group_classes << "required" if obj[name].is_a? String and obj[name].end_with?("REQ")
+
       control_group_classes << "conditionally-required" if required == :conditionally
 
       control_group_classes << "#{opts[:control_class]}" if opts.has_key? :control_class
@@ -911,7 +899,9 @@ def merge_victim_view(hash, opts = {})
         end
       end
       html << "<div class='form-group'>"
-      html << "<div class='control-label col-sm-2'>#{I18n.t("#{prefix}#{jsonmodel_type.to_s}.#{property}")}</div>"
+      html << "<div class='control-label col-sm-2'>"
+      html << I18n.t("#{prefix}#{jsonmodel_type.to_s}.#{property}")
+      html << "</div>"
       html << "<div class='label-only col-sm-8'>#{value}</div>"
       html << "</div>"
     end
@@ -1275,10 +1265,11 @@ def read_only_view(hash, opts = {})
       end
 
       html << "<div class='form-group'>"
-      html << "<div class='control-label col-sm-2'>#{I18n.t("#{prefix}#{jsonmodel_type.to_s}.#{property}")}</div>"
+      html << "<div class='control-label col-sm-2'>"
+      html << I18n.t("#{prefix}#{jsonmodel_type.to_s}.#{property}")
+      html << "</div>"
       html << "<div class='label-only col-sm-8'>#{value}</div>"
       html << "</div>"
-
     end
 
     html << "</div>"

frontend/app/models/required_fields.rb

@@ -1,13 +1,12 @@
 class RequiredFields
 
-
   def self.get(record_type)
     uri = "/repositories/#{JSONModel.repository}/required_fields/#{record_type}"
     result = JSONModel::HTTP.get_json(uri)
     if result
       self.new(JSONModel(:required_fields).from_hash(result))
     else
-      nil
+      self.new(JSONModel(:required_fields).from_hash(record_type: record_type))
     end
   end
 
@@ -18,6 +17,7 @@ def self.from_hash(hash)
 
 
   def initialize(json)
+    json.required ||= {}
     @json = json
   end
 
@@ -29,7 +29,7 @@ def form_values
     values.merge({:lock_version => @json.lock_version})
   end
 
-
+  # confusing to return a hash from :values method in ruby
   def values
     @json.required || {}
   end
@@ -47,4 +47,14 @@ def save
 
     response
   end
+
+  def required?(property, type, field = nil)
+    if field.nil?
+      @json.required.has_key?(property) && @json.required[property].any? { |hash| hash["jsonmodel_type"] == type.to_s }
+    else
+      @json.required.has_key?(property) && @json.required[property].any? {
+        |hash| hash.has_key?(field) && (hash[field] == "REQ" || hash[field] == type.to_s)
+      }
+    end
+  end
 end

frontend/app/views/agent_alternate_sets/_template.html.erb

@@ -41,17 +41,6 @@
   </div>
 <% end %>
 
-<% define_template "agent_alternate_set_required", jsonmodel_definition(:agent_alternate_set) do |form| %>
-  <% field_names = ["set_component", "descriptive_note", "file_uri", "file_version_xlink_actuate_attribute", "file_version_xlink_show_attribute", "xlink_title_attribute", "xlink_role_attribute", "xlink_arcrole_attribute", "last_verified_date"] %>
-  <% field_names.each do |field_name| %>
-    <% if form.required?(field_name) %>
-      <%= form.label_and_readonly field_name %>
-    <% else %>
-      <%= form.label_and_req_boolean field_name %>
-    <% end %>
-  <% end %>
-<% end %>
-
 <% define_template "agent_alternate_set_merge_target", jsonmodel_definition(:agent_alternate_set) do |form| %>
 
   <%= form.record_level_merge_controls(form, "agent_alternate_set", false) %>