diff --git a/.koppie/config/metadata_profiles/m3_profile.yaml b/.koppie/config/metadata_profiles/m3_profile.yaml index 2f33e63022..c8f77ec9fb 100644 --- a/.koppie/config/metadata_profiles/m3_profile.yaml +++ b/.koppie/config/metadata_profiles/m3_profile.yaml @@ -927,4 +927,154 @@ properties: display: true primary: false range: http://www.w3.org/2001/XMLSchema#string - property_uri: http://vocabulary.samvera.org/ns#transcriptIds \ No newline at end of file + property_uri: http://vocabulary.samvera.org/ns#transcriptIds + # Sample compound (hierarchical) metadata, demonstrating the Hyrax compound + # foundation in flexible mode. A compound is a `type: hash, multiple: true` + # parent; its members are declared as separate properties pointing back with + # `subproperty_of: `, each carrying its own `available_on`, indexing, + # and form placement. `subproperty_of` entries are not standalone resource + # attributes. See documentation/compound_fields.md. + participants: + available_on: + class: + - GenericWork + - CollectionResource + data_type: array + type: hash + range: http://www.w3.org/2001/XMLSchema#string + display_label: + default: Participants + form: + primary: true + property_uri: http://id.loc.gov/vocabulary/relators/asn + # Group metadata (label) for subproperties declaring `group: `. + groups: + identity: + label: Identity + view: + render_as: compound + html_dl: true + # Subproperties omit `available_on`/`range`/`display_label`: they inherit the + # parent compound's class scope and are folded into it by the schema loader. + participant_title: + type: string + subproperty_of: participants + group: identity + form: + cols: 4 + participant_name: + type: string + subproperty_of: participants + group: identity + required: true + indexing: [participant_name_sim, participant_name_tesim] + form: + cols: 4 + participant_role: + type: controlled + subproperty_of: participants + group: identity + required: true + values: [Author, Editor, Contributor, Creator, Data Collector, Funder, Publisher, Other] + indexing: [participant_role_sim, participant_role_tesim] + form: + cols: 4 + identifiers: + available_on: + class: + - GenericWork + - CollectionResource + data_type: array + type: hash + range: http://www.w3.org/2001/XMLSchema#string + display_label: + default: Identifiers + form: + primary: false + property_uri: http://purl.org/dc/terms/identifier + view: + render_as: compound + html_dl: true + identifier_value: + type: string + subproperty_of: identifiers + required: true + indexing: [identifier_value_sim, identifier_value_tesim] + form: + cols: 6 + identifier_type: + type: controlled + subproperty_of: identifiers + required: true + values: [DOI, Handle, ISBN, ISSN, URL, URN, ARK, Other] + indexing: [identifier_type_sim] + form: + cols: 6 + compound_rights: + available_on: + class: + - GenericWork + - CollectionResource + data_type: array + type: hash + range: http://www.w3.org/2001/XMLSchema#string + display_label: + default: Rights + form: + primary: false + property_uri: http://purl.org/dc/terms/rights + view: + render_as: compound + html_dl: true + compound_rights_statement: + type: controlled + subproperty_of: compound_rights + authority: rights_statements + indexing: [compound_rights_statement_sim, compound_rights_statement_tesim] + form: + cols: 6 + compound_rights_license: + type: controlled + subproperty_of: compound_rights + authority: licenses + indexing: [compound_rights_license_sim, compound_rights_license_tesim] + form: + cols: 6 + compound_rights_notes: + type: string + subproperty_of: compound_rights + indexing: [compound_rights_notes_tesim] + form: + cols: 12 + relationships: + available_on: + class: + - GenericWork + - CollectionResource + data_type: array + type: hash + range: http://www.w3.org/2001/XMLSchema#string + display_label: + default: Relationships + form: + primary: true + property_uri: http://purl.org/dc/terms/relation + view: + render_as: compound + html_dl: true + display: card + relationship_item: + type: work_or_url + subproperty_of: relationships + required: true + indexing: [relationships_item_ssim] + form: + cols: 6 + relationship_type: + type: controlled + subproperty_of: relationships + required: true + values: [References, Is Referenced By, Is Part Of, Has Part, Is Version Of, Replaces, Requires, Other] + indexing: [relationships_type_sim] + form: + cols: 6 \ No newline at end of file diff --git a/.rubocop.yml b/.rubocop.yml index 6cb6d839a4..3442a3b2e4 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -95,6 +95,7 @@ Rails/OutputSafety: - 'app/presenters/hyrax/fixity_status_presenter.rb' - 'app/presenters/hyrax/presents_attributes.rb' - 'app/renderers/hyrax/renderers/attribute_renderer.rb' + - 'app/renderers/hyrax/renderers/compound_attribute_renderer.rb' - 'spec/views/hyrax/my/works/_list_works.html.erb_spec.rb' RSpec/DescribeClass: diff --git a/app/assets/javascripts/hyrax.js b/app/assets/javascripts/hyrax.js index cb70a7135d..f76be1a0e6 100644 --- a/app/assets/javascripts/hyrax.js +++ b/app/assets/javascripts/hyrax.js @@ -46,6 +46,7 @@ //= require hyrax/single_use_links_manager //= require hyrax/copy_permalink_button //= require hyrax/redirects +//= require hyrax/compound_metadata //= require hyrax/dashboard_actions //= require hyrax/batch //= require hyrax/flot_stats diff --git a/app/assets/javascripts/hyrax/compound_metadata.js b/app/assets/javascripts/hyrax/compound_metadata.js new file mode 100644 index 0000000000..3f09bb7ed6 --- /dev/null +++ b/app/assets/javascripts/hyrax/compound_metadata.js @@ -0,0 +1,98 @@ +// Behavior for compound metadata fields on resource edit forms (add/remove row, +// work_or_url select2). Event-delegated so dynamically-added rows need no +// rebinding. The sentinel guards against the IIFE running twice (Turbolinks +// re-evaluates module scripts on some navigation paths), which would stack +// duplicate listeners. Mirrors hyrax/redirects.js. +(function() { + if (document.hyraxCompoundsBound) return; + document.hyraxCompoundsBound = true; + + // Bind select2 to a `work_or_url` sub-property. The v3 API (Hyrax bundles + // select2-rails 3.x) binds to a hidden input; createSearchChoice lets a + // typed external URL be selected as-is. + function bindWorkOrUrlInputs(root) { + if (typeof jQuery === 'undefined' || !jQuery.fn.select2) return; + jQuery(root).find('[data-hyrax-compound-work-input]').each(function() { + var $el = jQuery(this); + if ($el.hasClass('select2-offscreen') || $el.data('select2')) return; // already bound + $el.select2({ + width: '100%', + allowClear: true, + placeholder: 'Search for a work or enter a URL', + minimumInputLength: 2, + // Let a typed value (e.g. an external URL) be selected as-is. + createSearchChoice: function(term) { + return { id: term, text: term }; + }, + // Render the current value's label (work title or the URL) on load. + initSelection: function(element, callback) { + var val = element.val(); + if (!val) return; + callback({ id: val, text: element.data('label') || val }); + }, + ajax: { + url: $el.data('autocomplete-url'), + dataType: 'json', + quietMillis: 250, + data: function(term, page) { return { q: term }; }, + results: function(data, page) { + return { + results: data.map(function(obj) { + return { id: obj.id, text: [].concat(obj.label)[0] }; + }) + }; + } + } + }); + }); + } + + // Bind saved rows once the DOM is ready (at script-eval time the form + // inputs don't exist yet, so the select2 would never attach). Covers both a + // fresh load and Turbolinks navigation. + function bindAll() { bindWorkOrUrlInputs(document); } + if (document.readyState === 'loading') { + document.addEventListener('DOMContentLoaded', bindAll); + } else { + bindAll(); + } + document.addEventListener('turbolinks:load', bindAll); + + document.addEventListener('click', function(event) { + var removeButton = event.target.closest('[data-hyrax-compound-remove-row]'); + if (removeButton) { + var row = removeButton.closest('[data-hyrax-compound-row]'); + if (!row) return; + var destroyFlag = row.querySelector('[data-hyrax-compound-destroy-flag]'); + if (destroyFlag && destroyFlag.value !== undefined) { + // Persisted rows: flip the _destroy flag and hide so the + // populator drops the row server-side. + destroyFlag.value = '1'; + row.style.display = 'none'; + } else { + row.parentNode.removeChild(row); + } + return; + } + + var addButton = event.target.closest('[data-hyrax-compound-add-row]'); + if (!addButton) return; + var section = addButton.closest('[data-hyrax-compound]'); + if (!section) return; + var template = section.querySelector('[data-hyrax-compound-row-template]'); + var rowsHost = section.querySelector('[data-hyrax-compound-rows]'); + if (!template || !rowsHost) return; + + // Monotonic counter on the section; never recycle an index after a + // row is removed. Fallback to row count when the attribute is missing. + var nextIndex = parseInt(section.dataset.nextIndex, 10); + if (isNaN(nextIndex)) { + nextIndex = rowsHost.querySelectorAll('[data-hyrax-compound-row]').length; + } + var html = template.innerHTML.replace(/__INDEX__/g, nextIndex); + rowsHost.insertAdjacentHTML('beforeend', html); + // Bind select2 on any work_or_url inputs in the row just added. + bindWorkOrUrlInputs(rowsHost.lastElementChild || rowsHost); + section.dataset.nextIndex = String(nextIndex + 1); + }); +})(); diff --git a/app/assets/stylesheets/hyrax/_collections.scss b/app/assets/stylesheets/hyrax/_collections.scss index 3fe27c0bff..98edae4e35 100644 --- a/app/assets/stylesheets/hyrax/_collections.scss +++ b/app/assets/stylesheets/hyrax/_collections.scss @@ -610,11 +610,18 @@ button.branding-banner-remove:hover { dt { flex-basis: 115px; + // Hold the label column fixed so every value column (dd) starts at the + // same x; otherwise a long unbreakable value (e.g. a URL link) shrinks + // this column and shifts the value left. + flex-shrink: 0; font-weight: bold; } dd { flex-basis: auto; + // Let the value column shrink to its flex space so long URLs wrap within + // it rather than forcing the row wider. + min-width: 0; } } diff --git a/app/assets/stylesheets/hyrax/_compound_metadata.scss b/app/assets/stylesheets/hyrax/_compound_metadata.scss new file mode 100644 index 0000000000..ad1c9cdd06 --- /dev/null +++ b/app/assets/stylesheets/hyrax/_compound_metadata.scss @@ -0,0 +1,50 @@ +// Compound metadata display on work and collection show pages. +// See documentation/compound_fields.md. +// +// `!important` throughout: the collection metadata area styles nested divs with +// a descendant selector (`.hyc-metadata div { display: flex; border-bottom }` +// in _collections.scss) that would otherwise turn each entry into a bordered +// flex row. These rules override it regardless of source order. +.hyrax-compound-values { + display: block !important; + flex-direction: column; + border: 0 !important; + padding: 0 !important; + max-width: 100%; + + // Force block flow on the entries/sub-properties; the label/value spans stay + // inline so each sub-property reads as "Label: value" on one wrapping line. + .hyrax-compound-entry, + .hyrax-compound-subproperty { + display: block !important; + } + + .hyrax-compound-entry, + .hyrax-compound-subproperty, + .hyrax-compound-subproperty-label, + .hyrax-compound-subproperty-value { + border: 0 !important; + padding: 0 !important; + margin: 0; + background: none; + } + + // A light divider between entries. + .hyrax-compound-entry + .hyrax-compound-entry { + margin-top: 0.5rem; + padding-top: 0.5rem !important; + border-top: 1px solid $gray-200 !important; + } + + .hyrax-compound-subproperty { + display: block; + line-height: 1.4; + overflow-wrap: break-word; + word-break: break-word; + } + + .hyrax-compound-subproperty-label { + font-weight: 600; + margin-right: 0.25rem; + } +} diff --git a/app/assets/stylesheets/hyrax/_hyrax.scss b/app/assets/stylesheets/hyrax/_hyrax.scss index 18076b42f9..c4a1fc55d7 100644 --- a/app/assets/stylesheets/hyrax/_hyrax.scss +++ b/app/assets/stylesheets/hyrax/_hyrax.scss @@ -10,7 +10,7 @@ "hyrax/select_work_type", "hyrax/users", "hyrax/dashboard", "hyrax/sidebar", "hyrax/controlled_vocabulary", "hyrax/accessibility", "hyrax/recent", "hyrax/viewer", "hyrax/breadcrumbs", "hyrax/facets", "hyrax/card", - "hyrax/badge"; + "hyrax/badge", "hyrax/compound_metadata"; @import "hydra-editor/multi_value_fields"; @import "typeahead"; @import "sharing_buttons"; diff --git a/app/authorities/qa/authorities/compound_works.rb b/app/authorities/qa/authorities/compound_works.rb new file mode 100644 index 0000000000..3f84882291 --- /dev/null +++ b/app/authorities/qa/authorities/compound_works.rb @@ -0,0 +1,43 @@ +# frozen_string_literal: true + +module Qa::Authorities + ## + # Autocomplete authority for the compound `work_or_url` sub-property's work + # picker, mounted at `/authorities/search/compound_works`. Returns readable + # works matched by {Hyrax::CompoundWorkPickerBuilder}. + class CompoundWorks < Qa::Authorities::Base + class_attribute :search_builder_class + self.search_builder_class = Hyrax::CompoundWorkPickerBuilder + + def search(_q, controller) + return [] unless controller.current_user + + response, _docs = search_response(controller) + response.documents.map do |doc| + { id: doc.id, label: Array(doc.title).first || doc.id, value: doc.id } + end + end + + private + + def search_service(controller) + @search_service ||= Hyrax::SearchService.new( + config: controller.blacklight_config, + user_params: controller.params, + search_builder_class: search_builder_class, + scope: controller, + current_ability: controller.current_ability + ) + end + + def search_response(controller) + access = controller.params[:access] || 'read' + + search_service(controller).search_results do |builder| + builder.with({ q: controller.params[:q] }) + .with_access(access) + .rows(20) + end + end + end +end diff --git a/app/controllers/concerns/hyrax/works_controller_behavior.rb b/app/controllers/concerns/hyrax/works_controller_behavior.rb index c3fbe24728..1845904f62 100644 --- a/app/controllers/concerns/hyrax/works_controller_behavior.rb +++ b/app/controllers/concerns/hyrax/works_controller_behavior.rb @@ -431,8 +431,17 @@ def after_create_error(errors, original_input_params_for_form = nil) end # Creating a form object that can re-render most of the submitted parameters. - # Required for ActiveFedora::Base objects only. + # + # The +FailedSubmissionFormWrapper+ re-exposes submitted params for the +new+ + # view to re-render, and is required for +ActiveFedora::Base+ objects only. A + # Valkyrie +ChangeSet+ form already holds the submitted, validated state — it + # was populated by +form.validate(params)+ before the error — so +@form+ + # re-renders directly with the user's input and errors. The wrapper has no + # +permitted_params+ source for a ChangeSet, so wrapping it would raise. + # Mirrors the same +Hyrax::ChangeSet+ guard in {#after_update_error}. def rebuild_form(original_input_params_for_form) + return if @form.is_a?(Hyrax::ChangeSet) + build_form @form = Hyrax::Forms::FailedSubmissionFormWrapper .new(form: @form, diff --git a/app/controllers/hyrax/dashboard/collections_controller.rb b/app/controllers/hyrax/dashboard/collections_controller.rb index 80646f0f15..155f6f04ed 100644 --- a/app/controllers/hyrax/dashboard/collections_controller.rb +++ b/app/controllers/hyrax/dashboard/collections_controller.rb @@ -271,11 +271,11 @@ def valkyrie_destroy end def form_err_msg(form) - errmsg = [] - form.errors.messages.each do |fld, err| - errmsg << "#{fld} #{err.to_sentence}" - end - errmsg.to_sentence + # Use ActiveModel's full_messages: it humanizes the attribute name + # ("collection_type_gid" -> "Collection type gid") and renders :base + # errors (e.g. the compound-required messages) verbatim, instead of the + # raw "#{field_key} #{message}" we'd otherwise emit ("base ..."). + form.errors.full_messages.to_sentence end def default_collection_type diff --git a/app/forms/concerns/hyrax/compound_field_behavior.rb b/app/forms/concerns/hyrax/compound_field_behavior.rb new file mode 100644 index 0000000000..f018b9127b --- /dev/null +++ b/app/forms/concerns/hyrax/compound_field_behavior.rb @@ -0,0 +1,89 @@ +# frozen_string_literal: true + +module Hyrax + ## + # Generic, schema-driven form handling for compound metadata fields (see + # {Hyrax::CompoundSchema}). The generic equivalent of the hand-written + # {Hyrax::RedirectsFieldBehavior}: it discovers every compound on the form's + # model and wires each one's virtual `_attributes` property and + # populator from the schema, in both flex modes. + module CompoundFieldBehavior + include Hyrax::CompoundRowPlumbing + + # Register the virtual `_attributes` populator properties on the + # singleton class for every compound on the resource. Must run before Reform + # builds the deserialization schema; the flexible-mode init path calls this, + # while non-flexible mode wires the same properties at class load via + # {Hyrax::FormFields}. + # + # @param [Valkyrie::Resource] resource the form's resource (the form's own + # `model` may not be set yet when this runs). + def register_compound_fields!(resource) + Hyrax::CompoundSchema.for(resource).compound_names.each do |name| + next if singleton_class.method_defined?(:"#{name}_attributes=") + + singleton_class.property :"#{name}_attributes", + virtual: true, + populator: :compound_attributes_populator + end + rescue StandardError => e + Hyrax.logger.debug("CompoundFieldBehavior: register failed for #{self.class}: #{e.message}") + end + + # Strip each compound's renamed bare key so the `_attributes` + # populator is the single write entry point (the Field Behavior contract; + # see documentation/field_behaviors.md). + def deserialize!(params) + result = super + return result unless result.respond_to?(:delete) + + compound_field_names.each do |name| + result.delete(name.to_s) + result.delete(name.to_sym) + end + result + end + + private + + def compound_field_names + return [] unless respond_to?(:model) && model + Hyrax::CompoundSchema.for(model).compound_names + rescue StandardError => e + Hyrax.logger.debug("CompoundFieldBehavior: could not read compounds for #{self.class}: #{e.message}") + [] + end + + # One populator serves every compound (Reform passes the property name as + # `as:`). Builds the replacement array of plain hashes — declared + # sub-property keys only, dropping `_destroy` and all-blank rows. + def compound_attributes_populator(fragment:, as:, **_options) + name = as.to_s.delete_suffix('_attributes') + return unless respond_to?(name) + + allowed = Hyrax::CompoundSchema.for(model).subproperty_keys(name) + public_send(:"#{name}=", build_compound_rows(fragment, allowed)) + end + + def build_compound_rows(fragment, allowed_keys) + fragment_pairs(fragment) + .sort_by { |key, _row| key.to_i } + .map { |_key, row| compound_row_from(row, allowed_keys) } + .compact + end + + # Returns nil for a row marked for destruction or whose declared sub-properties + # are all blank, otherwise the persisted hash for that row. + def compound_row_from(row, allowed_keys) + row = row_hash(row) + return nil if %w[true 1].include?(row['_destroy'].to_s) + + entry = allowed_keys.each_with_object({}) do |key, memo| + value = row[key] + memo[key] = value.is_a?(String) ? value.strip : value + end + return nil if entry.values.all?(&:blank?) + entry + end + end +end diff --git a/app/forms/concerns/hyrax/compound_row_plumbing.rb b/app/forms/concerns/hyrax/compound_row_plumbing.rb new file mode 100644 index 0000000000..3be52ab776 --- /dev/null +++ b/app/forms/concerns/hyrax/compound_row_plumbing.rb @@ -0,0 +1,24 @@ +# frozen_string_literal: true + +module Hyrax + # Shared coercion of a nested-attributes fragment into plain Ruby hashes, + # used by both {Hyrax::CompoundFieldBehavior} and + # {Hyrax::RedirectsFieldBehavior}. It only normalizes shape (unwrapping + # `ActionController::Parameters` via `to_unsafe_h`); the row-drop rules + # (`_destroy`, blank paths, all-blank rows) and any value normalization stay + # in each populator. + module CompoundRowPlumbing + private + + # The submitted `_attributes` payload as a `{ index => row }` hash. + def fragment_pairs(fragment) + return {} if fragment.nil? + fragment.respond_to?(:to_unsafe_h) ? fragment.to_unsafe_h : fragment.to_h + end + + # A single row as a plain hash. + def row_hash(row) + row.respond_to?(:to_unsafe_h) ? row.to_unsafe_h : row.to_h + end + end +end diff --git a/app/forms/concerns/hyrax/flexible_form_behavior.rb b/app/forms/concerns/hyrax/flexible_form_behavior.rb index 0d1198e4c5..286ea53b28 100644 --- a/app/forms/concerns/hyrax/flexible_form_behavior.rb +++ b/app/forms/concerns/hyrax/flexible_form_behavior.rb @@ -13,6 +13,10 @@ def validate_flexible_required_fields required_fields = singleton_class.schema_definitions.select { |_, opts| opts[:required] }.keys required_fields.each do |field| + # Compound fields carry their own requiredness rules (per-row sub-property + # checks); Hyrax::CompoundEntryValidator owns them, so skip the generic + # blank check to avoid a duplicate "can't be blank" error. + next if compound_field?(field) value = send(field) errors.add(field, :blank) if value.blank? end @@ -25,6 +29,16 @@ def schema private + # Whether the field is a compound (declares `subproperties:`), looked up from + # the resource's compound schema — the flex form definitions don't carry + # `subproperties`. Memoized per form instance. + def compound_field?(field) + @compound_field_names ||= Hyrax::CompoundSchema.for(model).compound_names + @compound_field_names.include?(field.to_sym) + rescue StandardError + false + end + # OVERRIDE valkyrie 3.0.1 to make schema dynamic def field(field_name) singleton_class.schema_definitions.fetch(field_name.to_s) diff --git a/app/forms/concerns/hyrax/redirects_field_behavior.rb b/app/forms/concerns/hyrax/redirects_field_behavior.rb index dbe30d17f9..b55565c85d 100644 --- a/app/forms/concerns/hyrax/redirects_field_behavior.rb +++ b/app/forms/concerns/hyrax/redirects_field_behavior.rb @@ -28,6 +28,8 @@ module Hyrax # `Flipflop.redirects?` directly is unsafe when the config is off # because the feature isn't registered in that case. module RedirectsFieldBehavior + include Hyrax::CompoundRowPlumbing + def self.included(descendant) return unless Hyrax.config.redirects_enabled? # Declare the radio-group scalar before redirects_attributes so Reform @@ -66,19 +68,14 @@ def deserialize!(params) def redirects_attributes_populator(fragment:, **_options) return unless respond_to?(:redirects) return unless Hyrax.config.redirects_active? - pairs = redirects_fragment_pairs(fragment) + pairs = fragment_pairs(fragment) self.redirects = pairs.sort_by { |k, _row| k.to_i } .map { |k, row| redirects_entry_from(k, row) } .compact end - def redirects_fragment_pairs(fragment) - return {} if fragment.nil? - fragment.respond_to?(:to_unsafe_h) ? fragment.to_unsafe_h : fragment.to_h - end - def redirects_entry_from(key, row) - row = row.respond_to?(:to_unsafe_h) ? row.to_unsafe_h : row + row = row_hash(row) return nil if row['_destroy'].to_s == 'true' || row['path'].to_s.strip.empty? { 'path' => Hyrax::RedirectPathNormalizer.call(row['path']), 'is_display_url' => redirects_is_display_url_flag_for(key, row) } diff --git a/app/forms/hyrax/forms.rb b/app/forms/hyrax/forms.rb index 10575e9c21..cf249437e3 100644 --- a/app/forms/hyrax/forms.rb +++ b/app/forms/hyrax/forms.rb @@ -14,6 +14,13 @@ def self.PcdmObjectForm(work_class) # rubocop:disable Naming/MethodName Class.new(Hyrax::Forms::PcdmObjectForm) do self.model_class = work_class include Hyrax::FormFields(:core_metadata) if work_class.ancestors.detect { |k| k.inspect == "Hyrax::Schema(core_metadata)" } + # Wire the compound form properties only for works whose model includes + # the compound_metadata schema (non-flexible mode); the m3 loader + # supplies them at init in flexible mode. + if !Hyrax.config.flexible? && + work_class.ancestors.detect { |k| k.inspect == "Hyrax::Schema(compound_metadata)" } + include Hyrax::FormFields(:compound_metadata) + end ## # @return [String] diff --git a/app/forms/hyrax/forms/pcdm_collection_form.rb b/app/forms/hyrax/forms/pcdm_collection_form.rb index 542829651d..e8355706c4 100644 --- a/app/forms/hyrax/forms/pcdm_collection_form.rb +++ b/app/forms/hyrax/forms/pcdm_collection_form.rb @@ -7,6 +7,12 @@ module Forms # @see https://github.com/samvera/valkyrie/wiki/ChangeSets-and-Dirty-Tracking class PcdmCollectionForm < Hyrax::Forms::ResourceForm # rubocop:disable Metrics/ClassLength include Hyrax::FormFields(:core_metadata) if Hyrax.config.collection_include_metadata? + # Wire the compound form properties (non-flexible mode), mirroring the + # work-side wiring in the PcdmObjectForm factory. + if !Hyrax.config.flexible? && + Hyrax::PcdmCollection.ancestors.detect { |k| k.inspect == "Hyrax::Schema(compound_metadata)" } + include Hyrax::FormFields(:compound_metadata) + end check_if_flexible(Hyrax::PcdmCollection) BannerInfoPrepopulator = lambda do |**_options| @@ -65,7 +71,7 @@ def required_fields def primary_terms terms = _form_field_definitions .select { |_, definition| definition[:primary] } - .keys.map(&:to_sym) + .keys.map(&:to_sym) - compound_terms terms = [:schema_version] + terms if model.flexible? terms @@ -76,7 +82,7 @@ def primary_terms def secondary_terms _form_field_definitions .select { |_, definition| definition[:display] && !definition[:primary] } - .keys.map(&:to_sym) + .keys.map(&:to_sym) - compound_terms end ## diff --git a/app/forms/hyrax/forms/resource_form.rb b/app/forms/hyrax/forms/resource_form.rb index 386764d2a8..2d7a65d572 100644 --- a/app/forms/hyrax/forms/resource_form.rb +++ b/app/forms/hyrax/forms/resource_form.rb @@ -28,6 +28,7 @@ class ResourceForm < Hyrax::ChangeSet # rubocop:disable Metrics/ClassLength include BasedNearFieldBehavior include RedirectsFieldBehavior + include CompoundFieldBehavior include Hyrax::FormFields(:redirects) if Hyrax.config.redirects_enabled? && Hyrax.config.work_include_metadata? class_attribute :model_class @@ -63,6 +64,15 @@ class ResourceForm < Hyrax::ChangeSet # rubocop:disable Metrics/ClassLength end end + # Required-compound / required-sub-property validation. Wired through a + # `validation { ... }` block (not a bare `validates_with`) because these + # are Reform/Disposable forms — a bare `validates_with` does not hook into + # Reform's `validate`, so it would never run. Record-level (no + # `attributes:`), so it is replay-safe. + validation(name: :default, inherit: true) do + validates_with Hyrax::CompoundEntryValidator + end + ## # @api public # @@ -79,6 +89,10 @@ def initialize(deprecated_resource = nil, resource: nil) current_schema_fields.each do |field_name, options| singleton_class.property field_name.to_sym, options.merge(display: options.fetch(:display, true), default: []) end + # Register the virtual `_attributes` populators on the + # singleton before `super`, so they are part of Reform's schema for + # this instance (non-flexible mode wires them at class load). + register_compound_fields!(r) if respond_to?(:register_compound_fields!) end if resource.nil? @@ -122,6 +136,7 @@ def inherited(subclass) # subclass's model. subclass.prepend(BasedNearFieldBehavior) subclass.prepend(RedirectsFieldBehavior) + subclass.prepend(CompoundFieldBehavior) super end @@ -226,7 +241,7 @@ def model_class # rubocop:disable Rails/Delegate def primary_terms terms = _form_field_definitions .select { |_, definition| definition[:primary] } - .keys.map(&:to_sym) + .keys.map(&:to_sym) - compound_terms terms = [:schema_version, :contexts] + terms if model.flexible? terms @@ -237,19 +252,55 @@ def primary_terms def secondary_terms _form_field_definitions .select { |_, definition| definition[:display] && !definition[:primary] } - .keys.map(&:to_sym) + .keys.map(&:to_sym) - compound_terms + end + + ## + # @return [Array] compound attribute terms, rendered by the + # compound form partials rather than as scalar fields (and so excluded + # from {#primary_terms} / {#secondary_terms}). + def compound_terms + return [] unless respond_to?(:model) && model + Hyrax::CompoundSchema.for(model).compound_names + rescue StandardError + [] + end + + ## + # @return [Array] compounds whose `form: { primary: true }`, shown + # in the primary form section alongside the primary scalar terms. + def primary_compound_terms + compound_terms.select { |term| compound_primary?(term) } + end + + ## + # @return [Array] compounds that are not primary (the default), + # shown in the "Additional fields" section. + def secondary_compound_terms + compound_terms.reject { |term| compound_primary?(term) } end ## # @return [Boolean] whether there are terms to display 'below-the-fold' + # (secondary scalar terms or non-primary compounds) def display_additional_fields? - secondary_terms.any? + secondary_terms.any? || secondary_compound_terms.any? end delegate :flexible?, to: :model private + # Whether a compound declares `form: { primary: true }`. Defaults to false + # (compounds render in "Additional fields" unless explicitly primary), + # mirroring how secondary scalar terms are derived. Read from the compound + # definition, which carries the flag in both flex modes. + def compound_primary?(term) + Hyrax::CompoundSchema.for(model).primary?(term) + rescue StandardError + false + end + def _form_field_definitions if model.flexible? singleton_class.schema_definitions diff --git a/app/helpers/hyrax/collections_helper.rb b/app/helpers/hyrax/collections_helper.rb index 09374db983..ba90fdb738 100644 --- a/app/helpers/hyrax/collections_helper.rb +++ b/app/helpers/hyrax/collections_helper.rb @@ -30,6 +30,11 @@ def available_parent_collections_data(collection:) # @since 3.0.0 # @return [#to_s] def collection_metadata_label(collection, field) + if collection.respond_to?(:compound_term?) && collection.compound_term?(field) + definition = compound_schema_for(collection).definition_for(field) + return compound_field_label(field, display_label: definition&.fetch(:display_label, nil)) + end + Hyrax::PresenterRenderer.new(collection, self).label(field) end @@ -37,6 +42,17 @@ def collection_metadata_label(collection, field) # @since 3.0.0 # @return [#to_s] def collection_metadata_value(collection, field) + # Compounds render through the shared CompoundAttributeRenderer, resolving + # the definition from the presenter's document (correct in flexible mode). + # Collection scalar values render as bare spans (no `ul.tabular`), so the + # entries render flush to match. See documentation/compound_fields.md. + if collection.respond_to?(:compound_term?) && collection.compound_term?(field) + definition = compound_schema_for(collection).definition_for(field) + return Hyrax::Renderers::CompoundAttributeRenderer + .new(field, collection[field], subproperties: definition&.fetch(:subproperties, nil)) + .render_value + end + Hyrax::PresenterRenderer.new(collection, self).value(field) end diff --git a/app/helpers/hyrax/compound_fields_helper.rb b/app/helpers/hyrax/compound_fields_helper.rb new file mode 100644 index 0000000000..effc34590e --- /dev/null +++ b/app/helpers/hyrax/compound_fields_helper.rb @@ -0,0 +1,150 @@ +# frozen_string_literal: true + +module Hyrax + # View helpers for rendering compound (hierarchical) metadata fields on forms + # and show pages. See documentation/compound_fields.md. + module CompoundFieldsHelper + ## + # Renders one compound section (a repeatable stack of sub-property rows) for the + # given attribute via the `hyrax/compounds/*` partials. + # + # @return [String, nil] rendered HTML, or nil when the attribute is not a + # declared compound on the model. + def render_compound_field(f, compound_name) + schema = Hyrax::CompoundSchema.for(f.object.model) + definition = schema.definition_for(compound_name) + return nil if definition.nil? + + render 'hyrax/compounds/compound_section', + f: f, + compound_name: compound_name.to_sym, + definition: definition, + display_label: compound_field_label(compound_name, display_label: definition[:display_label]) + end + + ## + # @return [Boolean] whether the field is a card-display compound + # (`view: { display: card }`). Show views use this to skip card compounds + # in the inline metadata list. + def compound_card_field?(presenter, field) + compound_schema_for(presenter).card?(field) + rescue StandardError + false + end + + ## + # Render every card-display compound that has a value as its own titled card + # (matching the relationships/items cards). + # + # @param [Object] presenter a work or collection show presenter + # @return [ActiveSupport::SafeBuffer] + def render_compound_cards(presenter) + safe_join(compound_schema_for(presenter).card_compound_names.map do |name| + next ''.html_safe unless presenter.respond_to?(name) && presenter.public_send(name).present? + + render 'hyrax/compounds/compound_card', presenter: presenter, field: name + end) + rescue StandardError => e + Hyrax.logger.debug("render_compound_cards: #{e.message}") + ''.html_safe + end + + # The compound schema for a show presenter, resolved from the backing Solr + # document so it works in flexible mode (where the class carries no + # compounds). Memoized per request. See {Hyrax::CompoundSchema.for_solr_document}. + def compound_schema_for(presenter) + @compound_schemas ||= {} + @compound_schemas[presenter.object_id] ||= + if presenter.respond_to?(:solr_document) && presenter.solr_document.respond_to?(:hydra_model) + Hyrax::CompoundSchema.for_solr_document(presenter.solr_document) + elsif presenter.is_a?(Hyrax::CollectionPresenter) + Hyrax::CompoundSchema.for(Hyrax.config.collection_class) + else + Hyrax::CompoundSchema.new + end + end + + ## + # Options for a `controlled` sub-property's `` dropdown | Options come from either an inline `values:` list or a QA local authority named by `authority:` (see below). The row's stored value is preserved even if it is no longer offered, matching the `include_current_value` convention of the ordinary controlled-field partials. | + +A `controlled` sub-property sources its options one of two ways: + +```yaml +# (a) inline list — no authority file needed +participant_role: + type: controlled + subproperty_of: participants + values: [Author, Editor, Contributor] +# (a') inline list with distinct id and label +participant_status: + type: controlled + subproperty_of: participants + values: + - { id: pub, label: Published } + - { id: dft, label: Draft } +# (b) an existing QA local authority (config/authorities/.yml) +identifier_type: + type: controlled + subproperty_of: identifiers + authority: identifier_type +``` + +Inline `values:` is convenient for small, stable lists and keeps the whole +declaration in one file. `authority:` reuses an existing QA local authority — +the same `config/authorities/*.yml` files ordinary controlled fields use. When +both are given, `values:` wins. Authority options are read through +`Hyrax::TolerantSelectService`, so an authority file that omits the `active:` +key behaves the same as it does for ordinary fields (terms default to active). + +A `work_or_url` sub-property's work search is broader than the stock "my works" +picker: `Hyrax::CompoundWorkPickerBuilder` matches a typed term against any +indexed query field **or** a partial/prefix title (so "jour" matches "Journal +of …"), restricted to works. It subclasses `Hyrax::SearchBuilder`, so the +catalog's read-permission filtering is retained — a user only sees works they +can read. The picker is mounted as the `compound_works` QA authority at +`/authorities/search/compound_works`. + +`db_table` (a typeahead backed by an ActiveRecord lookup table, with +add-new) and `geocode` (a Geonames/coordinate lookup) are planned additional +sub-property types; the form's row partial has an explicit extension point for +them. `geocode` generalizes the existing single-value controlled-URI location +field — see `Hyrax::BasedNearFieldBehavior` and +[`field_behaviors.md`](field_behaviors.md), which `geocode` is intended to +replace once it lands. + +### Grouping (`group:` + `groups:`, optional) + +Sub-properties can be clustered into labeled groups within each entry's card. +Membership is declared inline on each sub-property with `group: `; the +parent compound declares the groups' display metadata in a `groups:` block keyed +by that key (label only — no field lists): + +```yaml +participants: + type: hash + multiple: true + groups: + identity: { label: Identity } + role: { label: Role } + +participant_name: + type: string + subproperty_of: participants + group: identity + form: { cols: 4 } +``` + +A sub-property with no `group:` falls in a single leading unlabeled group. +Groups appear in the order their first member is declared; sub-properties appear +in document order within their group. Per-field width is `form: { cols: }` (out +of 12), not a group-level setting. + +### Indexing sub-properties (`index_keys:` / `indexing:`) + +Indexing is declared **per sub-property**, exactly as for scalar fields: list the +literal Solr field names the sub-property's value should be written to, under +`index_keys:` (non-flexible mode) or `indexing:` (flexible mode). The Solr +suffix on each name chooses the behavior — `_tesim` (stored + searchable, +multi-valued), `_sim` (facetable, not stored), `_ssim` (stored + facetable), +etc. — the same conventions scalar fields use. The author picks both the field +names and the suffixes. + +A sub-property with no `index_keys:`/`indexing:` is **not** written to a Solr field +of its own; it can still appear on the show page (see below). As with scalar +fields, the non-field control tokens (`facetable`, `stored_searchable`, +`admin_only`, `editor_only`) are filtered out if present. + +Indexing a sub-property makes it **searchable/facetable** in Solr; it does **not** +make it appear as a column on the catalog/search-results page. The catalog +renders only the fields explicitly registered with Blacklight +(`config.add_index_field`) — compound sub-properties are not registered by default. +To show a sub-property in search results, register its Solr field name in your +`CatalogController` (e.g. +`config.add_index_field 'participant_name_tesim', label: 'Participant'`). The +show page and edit form render compounds regardless. + +### `display:` (optional, default true) + +Controls whether a sub-property is included in the `_json_ss` blob that +the show page renders from. Combined with indexing, each sub-property can be: + +- **display + searchable** — has `index_keys:` and `display` not false +- **display-only** — no `index_keys:` (renders on show, not separately searchable) +- **searchable-only** — has `index_keys:` and `display: false` (indexed, hidden on show) +- **neither** — no `index_keys:` and `display: false` + +### `required:` (optional, default false) + +Marks a sub-property as required within each populated row, or — when set on the +compound itself — marks the whole compound as required. See +[Required sub-properties](#required-sub-properties) below. + +## Persisted shape + +A compound persists as an array of plain string-keyed hashes — one key per +declared sub-property: + +```ruby +work.contributors +# => [{ "given_name" => "Ada", "family_name" => "Lovelace", "role_label" => "author" }, +# { "given_name" => "Alan", "family_name" => "Turing", "role_label" => "author" }] +``` + +This shape round-trips cleanly through Postgres JSONB in both flex modes. (Use +this plain-hash shape rather than nesting a `Valkyrie::Resource`; nested +resources round-trip poorly and lack form-layer support — see +[`field_behaviors.md`](field_behaviors.md).) + +## What the foundation provides + +Adding a compound to the schema is enough to get all of the following, with no +per-compound code: + +- **Form rendering** — `Hyrax::Forms::ResourceForm#compound_terms` lists the + compounds, and the work form renders each via the + `hyrax/compounds/_compound_section` partials (a repeatable card stack with + add/remove-row controls). The add/remove client behavior is the engine asset + `app/assets/javascripts/hyrax/compound_metadata.js` (required via + `hyrax.js`). Compounds are excluded from `primary_terms` / `secondary_terms` + so they are not also rendered as scalar inputs. +- **Form → storage conversion** — `Hyrax::CompoundFieldBehavior` registers a + virtual `_attributes` property per compound and a shared populator that + builds the persisted array, dropping `_destroy` and all-blank rows and + keeping only declared sub-property keys. +- **Read-path defense** — `Hyrax::CompoundNormalization`, included on the + resource, guards against Valkyrie's single-element-array key-splay quirk on + reload. +- **Indexing** — `Hyrax::Indexers::CompoundIndexer`, included on the indexer, + writes each sub-property's value to the Solr field names it declares + (`index_keys:`/`indexing:`) and stores the displayable sub-properties as a + `_json_ss` blob for the show page. + +A resource and its indexer opt in with one include each: + +```ruby +class GenericWorkResource < Hyrax::Work + include Hyrax::Schema(:generic_work_resource) + include Hyrax::CompoundNormalization +end + +class GenericWorkResourceIndexer < Hyrax::ValkyrieWorkIndexer + include Hyrax::Indexer(:generic_work_resource) + include Hyrax::Indexers::CompoundIndexer +end +``` + +## Sample compounds shipped with Hyrax + +Hyrax ships sample compounds on **works and collections** by default, +demonstrating the supported sub-property types: + +- **`participants`** — `title`, `participant_name`, and a controlled + `participant_role` (inline `values:`). A person or organization associated + with the work or collection. +- **`identifiers`** — an open-entry `identifier` plus a controlled + `identifier_type` (inline `values:`). +- **`compound_rights`** — controlled `rights_statement` and `license` backed by + *existing* QA local authorities (`authority:`), plus an open `rights_notes`. +- **`relationships`** — a `work_or_url` `related_item` (search an internal work + or enter an external URL; linked on show) plus a controlled + `relationship_type`. + +They are declared in `config/metadata/compound_metadata.yaml` (non-flexible +mode) and in the default m3 profile (`config/metadata_profiles/m3_profile.yaml`, +flexible mode, `available_on` both `Hyrax::Work` and the collection class). +The engine base `Hyrax::Work` and `Hyrax::PcdmCollection` include them, and the +base work and collection indexers flatten them. Applications can add their own +compounds the same way and remove or override the samples in their own schema. + +## Show-page rendering + +Because the show page renders from Solr (not the live resource), the indexer +also stores each compound's ordered rows (limited to sub-properties with `display:` +not false) as a single JSON field (`_json_ss`). The SolrDocument +defines a `` reader for each such blob it carries — so an +application's own compounds work with no per-document declaration — that +parses it back into an array of hashes, and the generic +`Hyrax::Renderers::CompoundAttributeRenderer` (selected by `view: render_as: +compound`) renders each entry's populated sub-properties as a small definition +list. Works render compounds through the standard `attribute_to_html` / +`render_as` path; collections render them through the same shared renderer, +invoked from the collection show view for terms the presenter reports as +compounds. Sub-property labels come from the `hyrax.compound_fields..` +i18n keys (humanized fallback). + +For a `controlled` sub-property, the show page displays the authority/value-list +**term**, not the stored id — `Hyrax::CompoundSubpropertyLabeler` resolves the id +through the sub-property's inline `values:` list or its QA `authority:` (falling +back to the id when no term matches), the same way scalar controlled fields +display their term. + +### Inline vs. card display (`view: { display: card }`) + +By default a compound renders **inline** in the metadata list, like any other +attribute. Declaring `view: { display: card }` on the compound instead renders +it as its **own titled card** on the show page — matching the Relationships and +Items cards — for compounds that read better as a standalone block: + +```yaml +relationships: + type: hash + multiple: true + view: + render_as: compound + display: card + +relationship_item: + type: work_or_url + subproperty_of: relationships +relationship_type: + type: controlled + subproperty_of: relationships + authority: relationship_type +``` + +`Hyrax::CompoundSchema` reports each compound's `display_mode` (`:inline` or +`:card`); `card_compound_names` lists the card compounds. The view helper +`render_compound_cards(presenter)` renders every card compound that has a value: + +- On a **work** show page, the cards render above the Relationships and Items + cards. +- On a **collection** show page, the cards render after the search-within bar + and before the works list. + +Inline compounds are still listed by the presenter's `terms`; card compounds are +excluded from the inline list (`inline_compound_names`) so they appear only as +their card. A collection presenter delegates the card compound's reader to its +SolrDocument so the same `attribute_to_html(:, render_as: :compound)` +path resolves on a collection as on a work. + +## Profile validation (flexible mode) + +In flexible mode, `Hyrax::FlexibleSchemaValidators::CompoundValidator` checks +compound declarations when an m3 profile is saved/uploaded, so a +misconfiguration fails with a clear message instead of producing dead Solr +fields or unrenderable values. It enforces: a `subproperty_of:` points at a +declared `type: hash` compound property; a `type: controlled` sub-property +declares an option source (`authority:` or `values:`); and a compound parent +does **not** carry a top-level `indexing:` (indexing is declared per +sub-property — a top-level `indexing:` would point the catalog at a +`_tesim` field the indexer never writes). + +## Required sub-properties + +A compound declares requiredness at two levels: + +- **Sub-property level** — `required: true` on a sub-property means every populated + row must fill it. A row that fills some-but-not-all of its required + sub-properties blocks save (e.g. a relationship row with a related item but no + type). +- **Compound level** — `required: true` on the compound (or, in flexible mode, + a minimum cardinality of 1) means at least one row must be present to save. + An optional compound with no rows is always valid; the sub-property rules only + apply to rows the user actually adds. + +```yaml +relationships: + type: hash + multiple: true + required: true # the work must have at least one relationship + +relationship_item: + type: work_or_url + subproperty_of: relationships + required: true +relationship_type: + type: controlled + subproperty_of: relationships + authority: relationship_type + required: true +relationship_note: + type: string + subproperty_of: relationships # optional +``` + +Required sub-properties and required compounds render a `*` marker on the form +label. On save, `Hyrax::CompoundEntryValidator` (wired on the resource form) +adds one error per compound, keyed on the compound's attribute name, so the +failure is reported against that field. The rules are the same in both flex +modes. + +The decision logic lives in `Hyrax::CompoundEntryValidation`, a plain object +decoupled from ActiveModel and Reform — given a compound's definition and its +rows it returns the violations. That seam keeps the rules reusable (e.g. a +future Bulkrax-side check) and unit-testable without a form. Note: Bulkrax +import does not currently run this validation, so a required sub-property can be +left empty by an import. + +## Validation + +Sub-property format and controlled-vocabulary correctness belong in an +`ActiveModel::EachValidator` wired through the form's `validation` block — not +in the populator, whose only job is shape conversion. This mirrors the +`field_behaviors.md` guidance. + +## Bulkrax + +A compound round-trips through Bulkrax import and export with no additional +code, using Bulkrax's declarative `nested_attributes: true` field-mapping flag +(Bulkrax v9.4.3+). Declare the flag on every sibling mapping that shares the +compound's `object:` value, and keep each mapping key equal to the sub-property +key. See `field_behaviors.md` for the column conventions. Note: Bulkrax's +controlled-URI sanitization does not currently descend into compound +sub-properties. diff --git a/documentation/forms/field_behaviors.md b/documentation/field_behaviors.md similarity index 81% rename from documentation/forms/field_behaviors.md rename to documentation/field_behaviors.md index 30dd68b630..7c711d70c0 100644 --- a/documentation/forms/field_behaviors.md +++ b/documentation/field_behaviors.md @@ -5,7 +5,7 @@ A **Field Behavior** is a Ruby module mixed into `Hyrax::Forms::ResourceForm` (a Use a Field Behavior when you have a property that: - Is rendered through `accepts_nested_attributes_for` semantics (`_attributes` payload from the form). -- Persists in a shape the view can't render directly (a hash of sub-fields, a URI string the view wants as a `ControlledVocabulary` instance, a triple of values, etc.). +- Persists in a shape the view can't render directly (a hash of sub-properties, a URI string the view wants as a `ControlledVocabulary` instance, a triple of values, etc.). - Needs the same wiring on every form subclass. Two examples ship with Hyrax: @@ -15,6 +15,16 @@ Two examples ship with Hyrax: This document covers the contract a Field Behavior must satisfy, the decision points for a new behavior, and the worked examples. +> **For a field whose entries are a hash of named sub-properties, prefer a +> [compound](compound_fields.md) instead** — it satisfies this same contract +> from the schema alone, with no per-field Ruby or ERB. +> [`compound_fields.md`](compound_fields.md) covers when to use a compound vs. a +> hand-written Field Behavior. The behaviors documented below are the right tool +> for the cases a compound does not cover: a single controlled-URI value per +> entry wrapped in a presenter (`based_near`), or bespoke behavior such as a +> radio-group selection, write-time normalization, global-uniqueness validation, +> or feature gating (`redirects`). + ## Why this pattern exists Reform's `FormBuilderMethods#deserialize!` rewrites the submitted `_attributes` key to `` before `from_hash` runs. If the form *also* has a property named `` (e.g. from an `include Hyrax::Schema(:foo)`), Reform's `from_hash` writes the raw fragment hash onto the property — bypassing the populator entirely. @@ -127,12 +137,12 @@ When adding a Field Behavior, work through these: ### 1. What does each entry look like? - **Single value per entry** (URI string, scalar): see `BasedNearFieldBehavior`. -- **Multiple sub-fields per entry** (path + is_display_url; or label + value + lang): see `RedirectsFieldBehavior`. +- **Multiple sub-properties per entry** (path + is_display_url; or label + value + lang): see `RedirectsFieldBehavior`. ### 2. Persisted as what? - **Plain string / scalar** — fine for single-value entries. -- **Plain hash** with string keys — for multi-field entries. Add the `hash` shortcut to your YAML schema (`type: hash, multiple: true`). Use this *instead* of nesting a Valkyrie::Resource subclass; nested resources round-trip badly through Postgres JSONB (sub-fields strip, parent fields leak). +- **Plain hash** with string keys — for multi-field entries. Add the `hash` shortcut to your YAML schema (`type: hash, multiple: true`). Use this *instead* of nesting a Valkyrie::Resource subclass; nested resources round-trip badly through Postgres JSONB (sub-properties strip, parent fields leak). ### 3. Does the view need a presenter? @@ -194,6 +204,21 @@ end - **View-side shape:** array of `Hyrax::ControlledVocabularies::Location` instances. - **Diff from a plain `_attributes` setup:** `deserialize!` strips `based_near` after the rename so `from_hash` doesn't overwrite the property with raw fragment hashes; the populator merges adds/removes onto the existing `model.based_near` so partial form submissions are non-destructive. +`based_near` is **not** a [compound](compound_fields.md), for two reasons that +make a compound the wrong fit rather than just an unused option: + +- Its persisted shape is a **bare URI string per entry**, where a compound + persists a hash of named sub-properties per entry. Declaring it as a compound + would change the stored shape from `["uri"]` to `[{ "key" => "uri" }]` — a data + migration, not a refactor. +- Its populator **merges** (`(model.based_near + adds) - deletes`) rather than + replacing the whole array, so partial submissions are non-destructive; the + generic compound populator always writes a full replacement array. + +It is the reference case for the planned `geocode` compound sub-property type +(see [`compound_fields.md`](compound_fields.md)), which will generalize a +single-value controlled-URI location lookup once it lands. + ## Example: `RedirectsFieldBehavior` ```ruby @@ -237,7 +262,26 @@ The populator folds a sibling `redirects_display_url_index` scalar (a single rad - **Persisted shape:** array of plain hashes (`'path'`, `'is_display_url'`). Declared with `type: hash, multiple: true` in `config/metadata/redirects.yaml`. - **View-side shape:** array of `Hyrax::Redirect` presenters, exposing `.path` and `.is_display_url`. The form partial wraps each persisted hash inline rather than via a prepopulator. -- **Diff from BasedNear:** entries carry multiple sub-fields, so the persisted shape is a hash rather than a string. The populator normalizes paths up front (canonical form lives in storage). The behavior is feature-gated — every callback consults `Hyrax.config.redirects_active?`. +- **Diff from BasedNear:** entries carry multiple sub-properties, so the persisted shape is a hash rather than a string. The populator normalizes paths up front (canonical form lives in storage). The behavior is feature-gated — every callback consults `Hyrax.config.redirects_active?`. + +`redirects` already uses the compound **parent** shape (`type: hash, multiple: +true`), but it is intentionally **not** declared with `subproperty_of: redirects` +children, so [`Hyrax::CompoundSchema`](compound_fields.md) does not treat it as a +compound. Declaring those children would activate the whole generic compound +stack — indexer, `render_as: compound` renderer, `Hyrax::CompoundEntryValidator`, +`Hyrax::CompoundNormalization`, the `_compound_section` partial, and a second +`redirects_attributes` populator — which would collide with the redirects-specific +`RedirectsLabelAttributeRenderer`, indexer, `RedirectValidator`, +`RedirectsNormalization`, and `_form_redirects` partial. That is a functional +change, not a refactor. The populator also folds the sibling +`redirects_display_url_index` radio scalar into per-row `is_display_url` flags and +normalizes each path — neither of which the generic compound populator does. + +The populator does share its fragment-coercion plumbing with the compound +populator: both include `Hyrax::CompoundRowPlumbing` for `fragment_pairs` +and `row_hash`. Each keeps its own row-drop rule (redirects drops blank-path rows; +the compound drops all-blank rows) and its own value handling (redirects +normalizes the path) — only the shape coercion is shared. ## Wiring on `ResourceForm` diff --git a/documentation/redirects.md b/documentation/redirects.md index 733846a7e2..2c03eddfb3 100644 --- a/documentation/redirects.md +++ b/documentation/redirects.md @@ -362,7 +362,7 @@ Add to the host app's Bulkrax field-mapping configuration (usually `config/initi 'is_display_url' => { from: ['redirect_is_display_url'], object: 'redirects', nested_attributes: true } ``` -See [field_behaviors.md](forms/field_behaviors.md#wiring-up-bulkrax-imports) for the conventions around the `nested_attributes: true` flag. +See [field_behaviors.md](field_behaviors.md#wiring-up-bulkrax-imports) for the conventions around the `nested_attributes: true` flag. ### Reserved-prefix list @@ -412,7 +412,7 @@ Alternatively, gate the inclusion of the calling code itself on `Hyrax.config.re ## See also - `documentation/flexible_metadata.md` — m3 profile fundamentals and how the redirects feature interacts with flexible metadata. -- `documentation/forms/field_behaviors.md` — the Field Behavior pattern used by `Hyrax::RedirectsFieldBehavior` to wire the form's nested-attribute property. +- `documentation/field_behaviors.md` — the Field Behavior pattern used by `Hyrax::RedirectsFieldBehavior` to wire the form's nested-attribute property. - `Hyrax::Redirect` (`app/models/hyrax/redirect.rb`) — thin Ruby presenter for a single redirect entry; used on the form's render path. - `Hyrax::RedirectsFieldBehavior` (`app/forms/concerns/hyrax/redirects_field_behavior.rb`) — form-side wiring for the `redirects_attributes` virtual property and the `redirects_display_url_index` radio-group scalar. Owns the populator (which folds the radio-group index into per-row `is_display_url` flags) and the `deserialize!` strip for the nested-attributes payload. The form partial wraps each persisted hash in a `Hyrax::Redirect` presenter inline at render time. - `Hyrax::Indexers::RedirectsIndexer` (`app/indexers/hyrax/indexers/redirects_indexer.rb`) — the indexer mixin. Writes `redirects_path_tesim` to the Solr document for show-page rendering. diff --git a/lib/hyrax/form_fields.rb b/lib/hyrax/form_fields.rb index 037184a505..a0b26880df 100644 --- a/lib/hyrax/form_fields.rb +++ b/lib/hyrax/form_fields.rb @@ -46,13 +46,41 @@ def inspect private + # Full attribute config per field — carries keys beyond `form:` (e.g. + # `subproperties:`), unlike form_field_definitions. + def attribute_configs + @definition_loader.attributes_for(schema: name, version:, contexts:) + end + + # @return [Boolean] whether the named field declares `subproperties:`. + def compound_field?(configs, field_name) + type = configs[field_name.to_sym] || configs[field_name.to_s] + meta = type.respond_to?(:meta) ? type.meta : {} + meta.with_indifferent_access['subproperties'].present? + rescue StandardError + false + end + # rubocop:disable Metrics/MethodLength def included(descendant) super return if @definition_loader.is_a?(Hyrax::M3SchemaLoader) + configs = attribute_configs form_field_definitions.each do |field_name, options| descendant.property field_name.to_sym, options.merge(display: options.fetch(:display, true), default: []) - descendant.validates field_name.to_sym, presence: true if options.fetch(:required, false) + is_compound = compound_field?(configs, field_name) + # A compound's requiredness is enforced by Hyrax::CompoundEntryValidator + # (which understands per-row sub-property rules); a plain `presence` check + # on the array would both duplicate that and pass on a present-but-empty + # row. So only scalar fields get the generic presence validator. + descendant.validates field_name.to_sym, presence: true if options.fetch(:required, false) && !is_compound + # A compound also needs a virtual `_attributes` populator + # property, registered here so it is part of Reform's schema before + # `validate` runs (cf. Hyrax::RedirectsFieldBehavior). + next unless is_compound + descendant.property :"#{field_name}_attributes", + virtual: true, + populator: :compound_attributes_populator end # Auto include any matching FormFieldBehaviors schema_name = name.to_s.camelcase diff --git a/spec/authorities/qa/authorities/compound_works_spec.rb b/spec/authorities/qa/authorities/compound_works_spec.rb new file mode 100644 index 0000000000..238d08ed73 --- /dev/null +++ b/spec/authorities/qa/authorities/compound_works_spec.rb @@ -0,0 +1,43 @@ +# frozen_string_literal: true +RSpec.describe Qa::Authorities::CompoundWorks, :clean_repo do + let(:controller) { Qa::TermsController.new } + let(:user) { create(:user) } + let(:ability) { instance_double(Ability, admin?: false, user_groups: ['public', 'registered'], current_user: user) } + let(:q) { "journal" } + let(:params) do + ActionController::Parameters.new(q: q, user: user.user_key, + controller: "qa/terms", action: "search", vocab: "compound_works") + end + let(:service) { described_class.new } + let!(:journal) { valkyrie_create(:monograph, :public, title: ['Journal of Practice Research']) } + let!(:other) { valkyrie_create(:monograph, :public, title: ['Unrelated Book']) } + + before do + allow(controller).to receive(:params).and_return(params) + allow(controller).to receive(:current_ability).and_return(ability) + end + + subject { service.search(q, controller) } + + describe '#search' do + before { allow(controller).to receive(:current_user).and_return(user) } + + it 'returns readable works matched by a partial title' do + expect(subject.map { |result| result[:id] }).to include(journal.id) + expect(subject.map { |result| result[:id] }).not_to include(other.id) + end + + it 'shapes each result as { id:, label:, value: }' do + result = subject.find { |r| r[:id] == journal.id } + expect(result).to include(id: journal.id, label: 'Journal of Practice Research', value: journal.id) + end + end + + describe '#search without a current user' do + before { allow(controller).to receive(:current_user).and_return(nil) } + + it 'returns an empty list' do + expect(subject).to eq([]) + end + end +end diff --git a/spec/controllers/concerns/hyrax/works_controller_behavior_spec.rb b/spec/controllers/concerns/hyrax/works_controller_behavior_spec.rb index be1e88a5fe..5985b19256 100644 --- a/spec/controllers/concerns/hyrax/works_controller_behavior_spec.rb +++ b/spec/controllers/concerns/hyrax/works_controller_behavior_spec.rb @@ -104,6 +104,22 @@ expect(Sipity::Entity(assigns[:curation_concern]).workflow_state).to have_attributes(name: "deposited") end + context 'when the submission fails validation' do + # A Valkyrie ChangeSet form re-renders directly on a failed create; it + # has no `permitted_params`, so the legacy FailedSubmissionFormWrapper + # must not wrap it (doing so raised ArgumentError). See #rebuild_form. + before do + allow_any_instance_of(Hyrax::ChangeSet).to receive(:validate).and_return(false) + end + + it 'rebuilds the form and renders new with an unprocessable status' do + post :create, params: { test_simple_work: { title: 'comet in moominland' } } + + expect(response).to have_http_status(:unprocessable_entity) + expect(assigns[:form]).to be_a(Hyrax::ChangeSet) + end + end + context 'when depositing as a proxy for (on_behalf_of) another user' do let(:create_params) { { title: 'comet in moominland', on_behalf_of: target_user.user_key } } let(:target_user) { FactoryBot.create(:user) } diff --git a/spec/controllers/hyrax/dashboard/collections_controller_spec.rb b/spec/controllers/hyrax/dashboard/collections_controller_spec.rb index da829a1722..0a89d667a6 100644 --- a/spec/controllers/hyrax/dashboard/collections_controller_spec.rb +++ b/spec/controllers/hyrax/dashboard/collections_controller_spec.rb @@ -231,9 +231,11 @@ context "in validations" do let(:form) { instance_double(Hyrax::Forms::PcdmCollectionForm, errors: errors) } - let(:errors) { instance_double(Reform::Contract::CustomError, messages: messages) } - let(:messages) { { publisher: ["must exist"] } } - let(:errmsg) { "publisher must exist" } + # Plain double: a real Reform form's errors responds to full_messages, + # but Reform::Contract::CustomError (what instance_double would verify) does not. + let(:errors) { double("errors", full_messages: full_messages) } + let(:full_messages) { ["Publisher must exist"] } + let(:errmsg) { "Publisher must exist" } before do skip("these validations only apply to Valkyrie forms") if @@ -459,9 +461,11 @@ context "in validations" do let(:form) { instance_double(Hyrax::Forms::PcdmCollectionForm, errors: errors) } - let(:errors) { instance_double(Reform::Contract::CustomError, messages: messages) } - let(:messages) { { publisher: ["must exist"] } } - let(:errmsg) { "publisher must exist" } + # Plain double: a real Reform form's errors responds to full_messages, + # but Reform::Contract::CustomError (what instance_double would verify) does not. + let(:errors) { double("errors", full_messages: full_messages) } + let(:full_messages) { ["Publisher must exist"] } + let(:errmsg) { "Publisher must exist" } before do skip("these validations only apply to Valkyrie forms") if diff --git a/spec/forms/concerns/hyrax/compound_field_behavior_spec.rb b/spec/forms/concerns/hyrax/compound_field_behavior_spec.rb new file mode 100644 index 0000000000..01bcc87090 --- /dev/null +++ b/spec/forms/concerns/hyrax/compound_field_behavior_spec.rb @@ -0,0 +1,110 @@ +# frozen_string_literal: true + +RSpec.describe Hyrax::CompoundFieldBehavior do + let(:resource_class) do + Class.new(Hyrax::Resource) do + def self.name + 'TestCompoundResource' + end + + attribute :contributors, + Valkyrie::Types::Array.of(Dry::Types['hash']).meta( + subproperties: { + 'given_name' => { 'type' => 'string' }, + 'family_name' => { 'type' => 'string' } + } + ) + end + end + + let(:model) { resource_class.new } + + # A minimal form stand-in that includes the behavior and exposes the model + # and a settable compound accessor, mirroring the redirects behavior spec. + let(:form_class) do + Class.new do + attr_accessor :contributors + + def self.property(*); end + + def initialize(model) + @model = model + end + + attr_reader :model + + def respond_to_missing?(name, *) + %i[contributors contributors=].include?(name) || super + end + + def from_hash(params) + params + end + + def deserialize!(params) + from_hash(params) + end + + # Prepend (not include) so the module's `deserialize!` lands above the + # class's own on the ancestor chain — mirroring how + # `Hyrax::Forms::ResourceForm.inherited` wires the behavior. + prepend Hyrax::CompoundFieldBehavior + end + end + + subject(:form) { form_class.new(model) } + + describe '#compound_attributes_populator' do + it 'builds plain string-keyed hashes with only declared sub-properties' do + fragment = { + '0' => { 'given_name' => 'Ada', 'family_name' => 'Lovelace', 'unknown' => 'drop me' }, + '1' => { 'given_name' => 'Alan', 'family_name' => 'Turing' } + } + form.send(:compound_attributes_populator, fragment: fragment, as: :contributors_attributes) + + expect(form.contributors).to eq([ + { 'given_name' => 'Ada', 'family_name' => 'Lovelace' }, + { 'given_name' => 'Alan', 'family_name' => 'Turing' } + ]) + end + + it 'drops rows marked for destruction' do + fragment = { + '0' => { 'given_name' => 'Ada', 'family_name' => 'Lovelace' }, + '1' => { 'given_name' => 'Gone', 'family_name' => 'Away', '_destroy' => 'true' } + } + form.send(:compound_attributes_populator, fragment: fragment, as: :contributors_attributes) + + expect(form.contributors).to eq([{ 'given_name' => 'Ada', 'family_name' => 'Lovelace' }]) + end + + it 'drops the always-submitted marker and all-blank rows' do + fragment = { + '_marker' => { '_destroy' => '1' }, + '0' => { 'given_name' => '', 'family_name' => ' ' }, + '1' => { 'given_name' => 'Grace', 'family_name' => 'Hopper' } + } + form.send(:compound_attributes_populator, fragment: fragment, as: :contributors_attributes) + + expect(form.contributors).to eq([{ 'given_name' => 'Grace', 'family_name' => 'Hopper' }]) + end + + it 'orders rows by their numeric key' do + fragment = { + '2' => { 'given_name' => 'Third' }, + '0' => { 'given_name' => 'First' }, + '1' => { 'given_name' => 'Second' } + } + form.send(:compound_attributes_populator, fragment: fragment, as: :contributors_attributes) + + expect(form.contributors.map { |r| r['given_name'] }).to eq(%w[First Second Third]) + end + end + + describe '#deserialize!' do + it 'strips the renamed bare compound key so the populator owns the write' do + result = form.deserialize!('contributors' => [{ 'given_name' => 'leaked' }], 'other' => 'keep') + expect(result).to eq('other' => 'keep') + end + end +end diff --git a/spec/forms/hyrax/forms/compound_metadata_form_spec.rb b/spec/forms/hyrax/forms/compound_metadata_form_spec.rb new file mode 100644 index 0000000000..c32579c355 --- /dev/null +++ b/spec/forms/hyrax/forms/compound_metadata_form_spec.rb @@ -0,0 +1,171 @@ +# frozen_string_literal: true + +# Integration spec for the compound-metadata form flow through the real +# Hyrax::Forms::ResourceForm pipeline (factory build -> validate -> sync). +# +# These assertions exercise the wiring that unit specs for the populator and +# helper can't see: +# * the `` reader is registered (so the form partial can render), +# * the virtual `_attributes` populator is registered *in Reform's +# schema* (so `validate` invokes it — registering it too late silently +# drops the value), and +# * `validate` + `sync` actually writes the compound onto the model. +# +# It mirrors the existing redirects coverage in resource_form_spec.rb, which +# documents the same "Reform drops the assigned value during sync without a +# class-level FormFields include" failure mode. +# +# This is a non-flexible-mode integration spec: it includes the compound schema +# on the model class via `Hyrax::Schema(:compound_metadata)` (the +# `HYRAX_FLEXIBLE=false` registration path) and stubs `flexible? => false`. When +# the host app boots in flexible mode (allinson), the model class composition is +# fixed at load time and the stub can't retroactively attach the schema, so this +# path can only be exercised under a non-flexible stack (dassie/koppie). The flex +# path is covered by the singleton-schema registration in +# compound_field_behavior_spec and the live allinson UI. +RSpec.describe 'Compound metadata form flow', type: :model, unless: Hyrax.config.flexible? do + before do + allow(Hyrax.config).to receive(:flexible?).and_return(false) + # A work-like resource that includes the shipped compound schema, the way + # Hyrax::Work does. Named so the form factory's `*Form` lookup falls back + # to the generic ResourceForm. + stub_const('CompoundTestWork', Class.new(Hyrax::Work) do + include Hyrax::Schema(:compound_metadata) + include Hyrax::CompoundNormalization + end) + end + + let(:resource) { CompoundTestWork.new } + let(:form) { Hyrax::Forms::ResourceForm.for(resource: resource) } + + describe 'property registration' do + it 'exposes the compound readers so the form partial can render existing rows' do + expect(form).to respond_to(:participants) + expect(form).to respond_to(:identifiers) + end + + it 'exposes the virtual `_attributes` setters for nested form params' do + expect(form).to respond_to(:participants_attributes=) + expect(form).to respond_to(:identifiers_attributes=) + end + + it 'lists the compounds as compound_terms (and keeps them out of primary/secondary terms)' do + expect(form.compound_terms).to include(:participants, :identifiers, :compound_rights) + expect(form.primary_terms).not_to include(:participants, :identifiers, :compound_rights) + expect(form.secondary_terms).not_to include(:participants, :identifiers, :compound_rights) + end + + it 'partitions compounds by their form: { primary: } flag' do + # The shipped samples declare `participants` and `relationships` primary; + # `identifiers` and `compound_rights` are non-primary ("Additional fields"). + expect(form.primary_compound_terms).to contain_exactly(:participants, :relationships) + expect(form.secondary_compound_terms).to contain_exactly(:identifiers, :compound_rights) + expect(form.display_additional_fields?).to be true + end + + it 'registers `participants_attributes` as a real Reform property (not just method-missing)' do + # This is the property that has to exist in Reform's schema *before* + # validate runs; if it is registered too late the populator never fires. + # In non-flexible mode it is registered on the form class via FormFields; + # check the class definitions registry (the same lens the redirects + # coverage uses). + definition_keys = form.class.definitions.keys.map(&:to_s) + expect(definition_keys).to include('participants_attributes') + end + end + + describe 'validate + sync writes the compound to the model' do + let(:params) do + { 'participants_attributes' => + { '0' => { 'participant_title' => 'Dr', 'participant_name' => 'Ada Lovelace', 'participant_role' => 'Author' } }, + 'identifiers_attributes' => + { '0' => { 'identifier_value' => '10.1234/x', 'identifier_type' => 'DOI' } } } + end + + it 'populates the compound on the form during validate' do + form.validate(params) + expect(form.participants) + .to eq([{ 'participant_title' => 'Dr', 'participant_name' => 'Ada Lovelace', 'participant_role' => 'Author' }]) + expect(form.identifiers) + .to eq([{ 'identifier_value' => '10.1234/x', 'identifier_type' => 'DOI' }]) + end + + it 'writes the compound through to the model on sync' do + form.validate(params) + form.sync + expect(form.model.participants) + .to eq([{ 'participant_title' => 'Dr', 'participant_name' => 'Ada Lovelace', 'participant_role' => 'Author' }]) + expect(form.model.identifiers) + .to eq([{ 'identifier_value' => '10.1234/x', 'identifier_type' => 'DOI' }]) + end + + it 'drops `_destroy` and all-blank rows' do + form.validate('participants_attributes' => + { '0' => { 'participant_name' => 'Keep', 'participant_role' => 'Author' }, + '1' => { 'participant_name' => 'Remove', '_destroy' => 'true' }, + '2' => { 'participant_title' => '', 'participant_name' => '', 'participant_role' => '' } }) + form.sync + expect(form.model.participants).to eq([{ 'participant_title' => nil, 'participant_name' => 'Keep', 'participant_role' => 'Author' }]) + end + end + + describe 'required-subproperty validation blocks save' do + # Exercises the real form + Hyrax::CompoundEntryValidator end to end against + # the *shipped* compound schema (no stub). The shipped samples require + # sub-properties within a row (e.g. participants needs participant_name + participant_role, + # relationships needs related_item + relationship_type) but none is required + # at the compound level, so an empty compound is valid. Asserting through the + # real schema is what catches a validator that reads the form wrapper instead + # of `form.model`. + # + # Compound errors are attached to :base with the compound named in the + # message (so they render cleanly on both the work and collection forms). + def base_errors(form) + form.valid? + form.errors[:base] + end + + it 'allows an empty compound (none is required at the compound level)' do + form.validate('participants_attributes' => { '_marker' => { '_destroy' => '1' } }) + expect(base_errors(form)).to be_empty + end + + it 'flags a participants row that omits a required sub-property' do + form.validate('participants_attributes' => { '0' => { 'participant_role' => 'Author' } }) + expect(base_errors(form)).to include(a_string_including('Participants')) + end + + it 'flags a relationships row that omits a required sub-property' do + form.validate('relationships_attributes' => { '0' => { 'relationship_type' => 'References' } }) + expect(base_errors(form)).to include(a_string_including('Relationships')) + end + + it 'does not flag participants when its required sub-properties are filled' do + form.validate('participants_attributes' => { '0' => { 'participant_name' => 'Ada', 'participant_role' => 'Author' } }) + expect(base_errors(form)).not_to include(a_string_including('Participants')) + end + + it 'never flags `compound_rights` (no required sub-properties)' do + form.validate('participants_attributes' => { '0' => { 'participant_name' => 'Ada', 'participant_role' => 'Author' } }) + expect(base_errors(form)).not_to include(a_string_including('Rights')) + end + end + + describe 'persistence round-trip', :active_fedora do + # Skips on the Fedora adapter, which cannot serialize plain-hash compounds + # to RDF; the compound feature targets the Valkyrie/Postgres path. + it 'round-trips the compound through the persister' do + skip 'requires a Valkyrie (non-Fedora) persister' if + Hyrax.persister.is_a?(Wings::Valkyrie::Persister) + + form.validate('participants_attributes' => + { '0' => { 'participant_name' => 'Grace Hopper', 'participant_role' => 'Editor' } }) + form.sync + saved = Hyrax.persister.save(resource: form.model) + reloaded = Hyrax.query_service.find_by(id: saved.id) + + expect(reloaded.participants.first['participant_name'] || reloaded.participants.first[:participant_name]) + .to eq('Grace Hopper') + end + end +end diff --git a/spec/forms/hyrax/forms/pcdm_collection_form_spec.rb b/spec/forms/hyrax/forms/pcdm_collection_form_spec.rb index 30feca14bb..54a331d996 100644 --- a/spec/forms/hyrax/forms/pcdm_collection_form_spec.rb +++ b/spec/forms/hyrax/forms/pcdm_collection_form_spec.rb @@ -28,6 +28,20 @@ end end + # Compounds render via the dedicated compound section, not as scalar fields; + # they must be excluded from primary/secondary terms or they would render a + # second, broken scalar input (a `required` compound would block form submit + # client-side). PcdmCollectionForm overrides these terms, so it needs the same + # exclusion ResourceForm has. + describe 'compound terms are excluded from scalar term lists' do + it 'keeps compounds out of primary_terms and secondary_terms' do + compounds = form.compound_terms + skip 'no compounds declared on the collection in this configuration' if compounds.empty? + expect(form.primary_terms).not_to include(*compounds) + expect(form.secondary_terms).not_to include(*compounds) + end + end + describe '#banner_info' do let(:banner_info) do CollectionBrandingInfo.new( diff --git a/spec/helpers/hyrax/compound_fields_helper_spec.rb b/spec/helpers/hyrax/compound_fields_helper_spec.rb new file mode 100644 index 0000000000..e68eefb27d --- /dev/null +++ b/spec/helpers/hyrax/compound_fields_helper_spec.rb @@ -0,0 +1,132 @@ +# frozen_string_literal: true + +RSpec.describe Hyrax::CompoundFieldsHelper, type: :helper do + describe '#compound_subproperty_options' do + let(:spec) { { type: 'controlled', authority: nil, values: [%w[Author Author], %w[Editor ed]] } } + + it 'returns the inline values list' do + expect(helper.compound_subproperty_options(spec)).to eq([%w[Author Author], %w[Editor ed]]) + end + + it 'leaves the list unchanged when the current value is already offered' do + expect(helper.compound_subproperty_options(spec, 'ed')).to eq([%w[Author Author], %w[Editor ed]]) + end + + it 'appends a stored value that is not among the offered options' do + expect(helper.compound_subproperty_options(spec, 'Legacy')) + .to eq([%w[Author Author], %w[Editor ed], %w[Legacy Legacy]]) + end + + it 'returns an empty list for a controlled sub-property with neither values nor authority' do + expect(helper.compound_subproperty_options({ type: 'controlled', authority: nil, values: nil })).to eq([]) + end + end + + describe '#compound_subproperty_forced?' do + let(:spec) { { type: 'controlled', authority: nil, values: [%w[Author Author]] } } + + it 'is false when the value is blank' do + expect(helper.compound_subproperty_forced?(spec, '')).to be false + end + + it 'is false when the value is among the offered options' do + expect(helper.compound_subproperty_forced?(spec, 'Author')).to be false + end + + it 'is true when the value is not among the offered options' do + expect(helper.compound_subproperty_forced?(spec, 'Legacy')).to be true + end + end + + describe '#compound_subproperty_label' do + it 'falls back to a humanized sub-property key when no translation exists' do + expect(helper.compound_subproperty_label(:nonexistent_compound, :some_field)).to eq('Some field') + end + end + + describe '#compound_field_label' do + it 'uses a declared display_label' do + expect(helper.compound_field_label(:compound_rights, display_label: { 'default' => 'Rights' })).to eq('Rights') + end + + it 'falls back to a humanized name when no display_label or translation exists' do + expect(helper.compound_field_label(:nonexistent_compound)).to eq('Nonexistent compound') + end + end + + describe 'card display' do + # A schema whose only card compound is :relationships. + let(:schema) do + instance_double(Hyrax::CompoundSchema, + card?: false, + card_compound_names: [:relationships]) + end + + # Show presenters resolve their compound schema from the backing Solr + # document (so flexible mode works), so stub that resolution path. + before do + allow(Hyrax::CompoundSchema).to receive(:for_solr_document).and_return(schema) + allow(schema).to receive(:card?).with(:relationships).and_return(true) + allow(schema).to receive(:card?).with(:contributors).and_return(false) + end + + describe '#compound_card_field?' do + let(:solr_document) { instance_double(SolrDocument, hydra_model: GenericWork) } + let(:presenter) do + pres = instance_double(Hyrax::WorkShowPresenter, solr_document: solr_document) + allow(pres).to receive(:respond_to?).with(:solr_document).and_return(true) + pres + end + + it 'is true for a card-display compound' do + expect(helper.compound_card_field?(presenter, :relationships)).to be true + end + + it 'is false for an inline compound' do + expect(helper.compound_card_field?(presenter, :contributors)).to be false + end + + it 'is false (not raising) when the schema cannot be resolved' do + expect(helper.compound_card_field?(Object.new, :relationships)).to be false + end + end + + describe '#render_compound_cards' do + let(:solr_document) { instance_double(SolrDocument, hydra_model: GenericWork) } + + # A presenter test object that responds to `solr_document` and the + # compound reader. A plain instance_double can't `and_call_original` on + # `respond_to?`, and `render_compound_cards` probes the presenter with + # `respond_to?(name)`, so build a small stand-in that answers honestly. + def work_presenter(relationships:) + Class.new do + attr_reader :solr_document, :relationships + def initialize(solr_document, relationships) + @solr_document = solr_document + @relationships = relationships + end + end.new(solr_document, relationships) + end + + it 'renders a card for each card compound with a present value' do + presenter = work_presenter(relationships: [{ 'related_item' => 'x' }]) + allow(helper).to receive(:render).and_return('
'.html_safe) + + expect(helper).to receive(:render) + .with('hyrax/compounds/compound_card', presenter: presenter, field: :relationships) + helper.render_compound_cards(presenter) + end + + it 'skips a card compound with no value' do + presenter = work_presenter(relationships: []) + + expect(helper).not_to receive(:render) + expect(helper.render_compound_cards(presenter)).to eq(''.html_safe) + end + + it 'returns an empty safe string when the class cannot be resolved' do + expect(helper.render_compound_cards(Object.new)).to eq(''.html_safe) + end + end + end +end diff --git a/spec/indexers/hyrax/indexers/compound_indexer_spec.rb b/spec/indexers/hyrax/indexers/compound_indexer_spec.rb new file mode 100644 index 0000000000..81c0b9d38f --- /dev/null +++ b/spec/indexers/hyrax/indexers/compound_indexer_spec.rb @@ -0,0 +1,87 @@ +# frozen_string_literal: true + +RSpec.describe Hyrax::Indexers::CompoundIndexer do + let(:resource_class) do + Class.new(Hyrax::Resource) do + def self.name + 'TestIndexedCompoundResource' + end + + attribute :contributors, + Valkyrie::Types::Array.of(Dry::Types['hash']).meta( + subproperties: { + # searchable + facetable, via the literal field names the + # sub-property declares + 'given_name' => { 'type' => 'string', + 'index_keys' => %w[contributors_given_name_tesim contributors_given_name_sim] }, + # searchable only + 'family_name' => { 'type' => 'string', + 'index_keys' => %w[contributors_family_name_tesim] }, + # display-only: no index_keys, so no own Solr field, but in the blob + 'role_label' => { 'type' => 'controlled', 'authority' => 'contributor_role' }, + # searchable but excluded from the display blob + 'note' => { 'type' => 'string', 'index_keys' => %w[contributors_note_tesim], 'display' => false } + } + ) + end + end + + let(:host_indexer_class) do + Class.new(Hyrax::Indexers::ResourceIndexer) do + include Hyrax::Indexers::CompoundIndexer + end + end + + let(:indexer) { host_indexer_class.new(resource: resource) } + + context 'for a resource with compound entries' do + let(:resource) do + resource_class.new(contributors: [ + { 'given_name' => 'Ada', 'family_name' => 'Lovelace', 'role_label' => 'author', 'note' => 'n1' }, + { 'given_name' => 'Alan', 'family_name' => 'Turing', 'role_label' => 'author', 'note' => 'n2' } + ]) + end + + it 'writes each sub-property value to the literal Solr field names it declares' do + doc = indexer.to_solr + expect(doc['contributors_given_name_tesim']).to contain_exactly('Ada', 'Alan') + expect(doc['contributors_given_name_sim']).to contain_exactly('Ada', 'Alan') + expect(doc['contributors_family_name_tesim']).to contain_exactly('Lovelace', 'Turing') + end + + it 'does not write a Solr field for a sub-property with no index_keys' do + # role_label declares no index_keys; it has no field of its own. + expect(indexer.to_solr.keys.grep(/role_label/)).to be_empty + end + + it 'accepts symbol-keyed entries (JSONValueMapper reload shape)' do + resource.contributors = [{ given_name: 'Grace', family_name: 'Hopper' }] + doc = indexer.to_solr + expect(doc['contributors_given_name_tesim']).to contain_exactly('Grace') + end + + it 'stores the displayable sub-properties as a JSON blob for the show page' do + parsed = JSON.parse(indexer.to_solr['contributors_json_ss']) + # role_label (display-only) is included; note (display: false) is omitted. + expect(parsed).to eq([ + { 'given_name' => 'Ada', 'family_name' => 'Lovelace', 'role_label' => 'author' }, + { 'given_name' => 'Alan', 'family_name' => 'Turing', 'role_label' => 'author' } + ]) + end + + it 'omits display: false sub-properties from the blob even though they are indexed' do + parsed = JSON.parse(indexer.to_solr['contributors_json_ss']) + expect(parsed.first).not_to have_key('note') + # ...but `note` is still written to its own searchable field + expect(indexer.to_solr['contributors_note_tesim']).to contain_exactly('n1', 'n2') + end + end + + context 'for a resource with no compound entries' do + let(:resource) { resource_class.new(contributors: []) } + + it 'emits no compound Solr fields' do + expect(indexer.to_solr.keys).not_to include('contributors_given_name_tesim', 'contributors_json_ss') + end + end +end diff --git a/spec/models/concerns/hyrax/compound_normalization_spec.rb b/spec/models/concerns/hyrax/compound_normalization_spec.rb new file mode 100644 index 0000000000..53f6f035e3 --- /dev/null +++ b/spec/models/concerns/hyrax/compound_normalization_spec.rb @@ -0,0 +1,59 @@ +# frozen_string_literal: true + +RSpec.describe Hyrax::CompoundNormalization do + let(:resource_class) do + Class.new(Hyrax::Resource) do + def self.name + 'TestNormalizedCompoundResource' + end + + attribute :contributors, + Valkyrie::Types::Array.of(Dry::Types['hash']).meta( + subproperties: { 'given_name' => { 'type' => 'string' } } + ) + + include Hyrax::CompoundNormalization + end + end + + describe '.compound_attribute_names' do + it 'derives the compound list from the schema' do + expect(resource_class.compound_attribute_names).to contain_exactly(:contributors) + end + end + + describe 'read-path normalization' do + it 'collapses a multi-key splayed pair array back into a hash' do + # This is the shape JSONValueMapper produces on reload of a single entry: + # the persisted [{given_name:, family:}] is unwrapped and splayed to pairs. + expect(Hyrax::CompoundNormalization.normalize_compound([[:given_name, 'Ada'], [:family, 'Lovelace']])) + .to eq([{ 'given_name' => 'Ada', 'family' => 'Lovelace' }]) + end + + it 'leaves a well-formed array of hashes unchanged (stringifying keys)' do + expect(Hyrax::CompoundNormalization.normalize_compound([{ given_name: 'Ada' }])) + .to eq([{ 'given_name' => 'Ada' }]) + end + + it 'returns nil unchanged' do + expect(Hyrax::CompoundNormalization.normalize_compound(nil)).to be_nil + end + + it 'wraps a single hash in an array' do + expect(Hyrax::CompoundNormalization.normalize_compound({ 'given_name' => 'Ada' })) + .to eq([{ 'given_name' => 'Ada' }]) + end + end + + describe 'round-trip through the class constructor' do + it 'normalizes splayed compound input passed to .new' do + resource = resource_class.new(contributors: [[:given_name, 'Ada']]) + expect(resource.contributors).to eq([{ 'given_name' => 'Ada' }]) + end + + it 'normalizes well-formed compound input passed to .new' do + resource = resource_class.new(contributors: [{ 'given_name' => 'Grace' }]) + expect(resource.contributors).to eq([{ 'given_name' => 'Grace' }]) + end + end +end diff --git a/spec/models/solr_document_spec.rb b/spec/models/solr_document_spec.rb index f2c63cc3b9..ee1554635a 100644 --- a/spec/models/solr_document_spec.rb +++ b/spec/models/solr_document_spec.rb @@ -241,4 +241,49 @@ class Mimes is_expected.to eq ['foo'] end end + + describe "compound readers" do + # A compound (hierarchical) attribute is indexed as a `_json_ss` + # blob. The document defines a reader per such field at construction, so an + # application's own compounds render without a per-app declaration and + # without depending on the (possibly stale) flexible schema. See + # documentation/compound_fields.md. + let(:attributes) do + { 'contributors_json_ss' => + [%([{"given_name":"Grace","family_name":"Hopper"}])], + 'funding_references_json_ss' => + [%([{"funder_name":"NSF","award_number":"123"}])], + 'empty_compound_json_ss' => [] } + end + + it "defines a reader for each compound present on the document" do + expect(document).to respond_to(:contributors) + expect(document).to respond_to(:funding_references) + end + + it "defines the reader as a real method (not via method_missing)" do + # A real method is required so the reader wins over a same-named dynamic + # field accessor that would otherwise shadow it and return nil. + expect(document.singleton_class.method_defined?(:contributors)).to be true + expect(document.method(:contributors).owner).to eq(document.singleton_class) + end + + it "parses the stored rows back into an array of hashes" do + expect(document.contributors) + .to eq([{ 'given_name' => 'Grace', 'family_name' => 'Hopper' }]) + end + + it "handles multi-word compound names" do + expect(document.funding_references) + .to eq([{ 'funder_name' => 'NSF', 'award_number' => '123' }]) + end + + it "returns an empty array for a present-but-blank compound blob" do + expect(document.empty_compound).to eq([]) + end + + it "does not define a reader for a compound that is not on the document" do + expect(document).not_to respond_to(:nonexistent_compound) + end + end end diff --git a/spec/presenters/hyrax/collection_presenter_spec.rb b/spec/presenters/hyrax/collection_presenter_spec.rb index 9e71ce9588..9df9bfed63 100644 --- a/spec/presenters/hyrax/collection_presenter_spec.rb +++ b/spec/presenters/hyrax/collection_presenter_spec.rb @@ -18,9 +18,11 @@ it { is_expected.to delegate_method(:date_created).to(:solr_document) } describe ".terms" do + # The class method is the default terms only; compound terms are resolved + # per instance (see "#terms") because a flexible schema is per-resource. subject { described_class.terms } - it do + it 'is the default terms' do is_expected.to eq [:total_items, :alternative_title, :size, :resource_type, :creator, :contributor, :keyword, :license, :publisher, :date_created, :subject, :language, :identifier, @@ -28,6 +30,47 @@ end end + describe "#terms" do + it 'starts with the default terms' do + expect(presenter.terms.first(15)).to eq described_class::DEFAULT_TERMS + end + + it 'appends this collection\'s inline compound terms' do + allow(presenter).to receive(:compound_terms).and_return([:participants, :identifiers]) + expect(presenter.terms).to end_with(:participants, :identifiers) + end + end + + describe "compound attribute delegation" do + # Resolution is per instance, from the backing document's compound schema. + context "when a compound is declared for the collection" do + before do + allow(presenter).to receive(:all_compound_names).and_return([:relationships]) + allow(solr_doc).to receive(:relationships).and_return([{ 'related_item' => 'https://example.org' }]) + end + + it "responds to the declared compound reader" do + expect(presenter).to respond_to(:relationships) + end + + it "delegates the compound reader to the solr document" do + expect(presenter.relationships).to eq([{ 'related_item' => 'https://example.org' }]) + end + end + + context "for an undeclared method" do + before { allow(presenter).to receive(:all_compound_names).and_return([]) } + + it "does not respond to it" do + expect(presenter).not_to respond_to(:not_a_compound) + end + + it "raises NoMethodError" do + expect { presenter.not_a_compound }.to raise_error(NoMethodError) + end + end + end + describe "collection type methods" do it { is_expected.to delegate_method(:collection_type_is_nestable?).to(:collection_type).as(:nestable?) } it { is_expected.to delegate_method(:collection_type_is_brandable?).to(:collection_type).as(:brandable?) } diff --git a/spec/renderers/hyrax/renderers/compound_attribute_renderer_spec.rb b/spec/renderers/hyrax/renderers/compound_attribute_renderer_spec.rb new file mode 100644 index 0000000000..8c4fada149 --- /dev/null +++ b/spec/renderers/hyrax/renderers/compound_attribute_renderer_spec.rb @@ -0,0 +1,164 @@ +# frozen_string_literal: true + +RSpec.describe Hyrax::Renderers::CompoundAttributeRenderer do + let(:values) do + [{ 'title' => 'Dr', 'agent_name' => 'Ada Lovelace', 'agent_role' => 'Author' }, + { 'agent_name' => 'Alan Turing' }] + end + let(:renderer) { described_class.new(:agent, values, label: 'Agent', html_dl: true) } + + describe '#render_dl_row' do + subject(:markup) { renderer.render_dl_row } + + it 'renders the field label' do + expect(markup).to include('
Agent
') + end + + it 'renders one entry block per value' do + expect(markup.scan('class="hyrax-compound-entry"').count).to eq(2) + end + + it 'renders each populated sub-property as a labeled value' do + expect(markup).to include('Ada Lovelace').and include('Author') + expect(markup).to include('Alan Turing') + end + + it 'uses the i18n sub-property labels (humanized fallback)' do + # `title` has no compound_fields.agent.title key in the engine locale by + # default for an arbitrary compound, so it humanizes; agent_name maps to + # the shipped label "Name". + expect(markup).to include('Dr') + end + + it 'does not use
/
/
for entries (avoids inheriting metadata dividers)' do + entries = markup.split('hyrax-compound-values').last + expect(entries).not_to include(' '' }], label: 'Agent', html_dl: true) + expect(r.render_dl_row).to eq('') + end + end + + describe '#render (table row)' do + it 'renders a table row with the label and entries' do + markup = described_class.new(:agent, values, label: 'Agent').render + expect(markup).to include('Agent') + expect(markup).to include('Ada Lovelace') + end + end + + describe 'controlled sub-property term translation' do + let(:values) { [{ 'role' => 'ed', 'name' => 'Ada' }] } + let(:subproperties) do + { 'role' => { type: 'controlled', authority: nil, values: [%w[Author author], %w[Editor ed]] }, + 'name' => { type: 'string', authority: nil, values: nil } } + end + let(:renderer) { described_class.new(:agent, values, label: 'Agent', html_dl: true, subproperties: subproperties) } + + it 'displays the controlled term, not the stored id' do + markup = renderer.render_dl_row + expect(markup).to include('Editor') + expect(markup).not_to match(/>ed uri }] } + let(:subproperties) do + { 'rights_statement' => { type: 'controlled', authority: 'rights_statements', values: nil } } + end + let(:renderer) { described_class.new(:compound_rights, values, label: 'Rights', html_dl: true, subproperties: subproperties) } + + before do + allow(Hyrax::CompoundSubpropertyLabeler).to receive(:label_for) + .with(subproperties['rights_statement'], uri).and_return('In Copyright') + end + + it 'links the resolved term to its URI' do + markup = renderer.render_dl_row + expect(markup).to include(%(In Copyright)) + end + end + + describe 'url sub-property auto-linking' do + let(:values) { [{ 'related_item_url' => 'https://example.org/item/42', 'note' => 'see also' }] } + let(:subproperties) do + { 'related_item_url' => { type: 'url', authority: nil, values: nil }, + 'note' => { type: 'string', authority: nil, values: nil } } + end + let(:renderer) { described_class.new(:relationships, values, label: 'Relationships', html_dl: true, subproperties: subproperties) } + + it 'renders a url sub-property as an anchor' do + expect(renderer.render_dl_row).to include(' { type: 'work_or_url', authority: nil, values: nil } } } + + context 'when the value is an external URL' do + let(:values) { [{ 'related_item' => 'https://example.org/x' }] } + let(:renderer) { described_class.new(:relationships, values, label: 'Relationships', html_dl: true, subproperties: subproperties) } + + it 'auto-links the URL' do + expect(renderer.render_dl_row).to include(' 'work-123' }] } + let(:renderer) { described_class.new(:relationships, values, label: 'Relationships', html_dl: true, subproperties: subproperties) } + + before do + allow(Hyrax::CompoundWorkResolver).to receive(:resolve) + .with('work-123').and_return(['Linked Work', '/catalog/work-123']) + end + + it 'links to the work show path with its title' do + markup = renderer.render_dl_row + expect(markup).to include('href="/catalog/work-123"') + expect(markup).to include('Linked Work') + end + end + + context 'when the value is neither a URL nor a resolvable work' do + let(:values) { [{ 'related_item' => 'not-a-real-id' }] } + let(:renderer) { described_class.new(:relationships, values, label: 'Relationships', html_dl: true, subproperties: subproperties) } + + before { allow(Hyrax::CompoundWorkResolver).to receive(:resolve).with('not-a-real-id').and_return(nil) } + + it 'renders the value as plain text without a link' do + markup = renderer.render_dl_row + expect(markup).to include('not-a-real-id') + expect(markup).not_to include(' sub, 'b' => sub }) } + + it 'is valid with no rows' do + expect(described_class.new(definition, []).violations).to be_empty + end + + it 'is valid with a partially-filled row' do + expect(described_class.new(definition, [{ 'a' => 'x' }]).violations).to be_empty + end + end + + describe 'a compound with required sub-properties (optional compound)' do + let(:definition) { build_definition(subproperties: { 'item' => sub(required: true), 'type' => sub(required: true), 'note' => sub }) } + + it 'is valid with no rows (compound itself is optional)' do + expect(described_class.new(definition, []).violations).to be_empty + end + + it 'is valid when every populated row fills all required sub-properties' do + entries = [{ 'item' => 'a', 'type' => 't' }, { 'item' => 'b', 'type' => 't', 'note' => 'n' }] + expect(described_class.new(definition, entries).violations).to be_empty + end + + it 'flags a row missing a required sub-property' do + violations = described_class.new(definition, [{ 'item' => 'a' }]).violations + expect(violations).to contain_exactly(type: :missing_required_subproperties, missing: ['type']) + end + + it 'reports one violation per distinct missing-key set (deduped)' do + entries = [{ 'item' => 'a' }, { 'item' => 'b' }] # both miss only `type` + violations = described_class.new(definition, entries).violations + expect(violations.size).to eq(1) + end + + it 'accepts symbol-keyed rows' do + expect(described_class.new(definition, [{ item: 'a', type: 't' }]).violations).to be_empty + end + end + + describe 'a required compound' do + let(:definition) { build_definition(required: true, subproperties: { 'a' => sub(required: true) }) } + + it 'flags an empty compound' do + expect(described_class.new(definition, []).violations) + .to contain_exactly(type: :required_but_empty, missing: ['a']) + end + + it 'is valid with a complete row' do + expect(described_class.new(definition, [{ 'a' => 'x' }]).violations).to be_empty + end + + it 'flags an incomplete row rather than emptiness when a row is present' do + violations = described_class.new(definition, [{ 'a' => '' }]).violations + # an all-blank row is not "populated", so the compound reads as empty + expect(violations).to contain_exactly(type: :required_but_empty, missing: ['a']) + end + end + + describe '#valid?' do + it 'is true when there are no violations' do + expect(described_class.new(build_definition(subproperties: { 'a' => sub }), []).valid?).to be true + end + end +end diff --git a/spec/services/hyrax/compound_schema_spec.rb b/spec/services/hyrax/compound_schema_spec.rb new file mode 100644 index 0000000000..f7a596135d --- /dev/null +++ b/spec/services/hyrax/compound_schema_spec.rb @@ -0,0 +1,309 @@ +# frozen_string_literal: true + +RSpec.describe Hyrax::CompoundSchema do + # These resources stand in for what the schema loaders produce: each compound + # parent is a `hash` attribute whose meta carries the folded `subproperties:` + # map (the loaders fold each compound's `subproperty_of` members into the + # parent's meta) plus optional `groups:` label metadata. Each subproperty spec + # carries its own `group`, `form` (cols/as), index_keys, etc. + let(:resource_class) do + Class.new(Hyrax::Resource) do + def self.name + 'TestResourceWithCompounds' + end + + attribute :title, Valkyrie::Types::Array.of(Valkyrie::Types::String) + attribute :contributors, + Valkyrie::Types::Array.of(Dry::Types['hash']).meta( + subproperties: { + 'given_name' => { 'type' => 'string', 'group' => 'identity', 'form' => { 'cols' => 6 }, + 'index_keys' => %w[contributors_given_name_sim contributors_given_name_tesim] }, + 'family_name' => { 'type' => 'string', 'group' => 'identity', 'form' => { 'cols' => 6 }, + 'index_keys' => %w[contributors_family_name_tesim] }, + 'role_label' => { 'type' => 'controlled', 'authority' => 'contributor_role', 'group' => 'role', 'display' => false } + }, + groups: { 'identity' => { 'label' => 'Identity' }, 'role' => { 'label' => 'Role' } } + ) + attribute :identifiers, + Valkyrie::Types::Array.of(Dry::Types['hash']).meta( + subproperties: { + 'value' => { 'type' => 'string' }, + 'identifier_type' => { 'type' => 'controlled', 'authority' => 'identifier_type' } + } + ) + attribute :agent, + Valkyrie::Types::Array.of(Dry::Types['hash']).meta( + subproperties: { + 'agent_name' => { 'type' => 'string' }, + 'agent_role' => { 'type' => 'controlled', 'values' => ['Author', { 'id' => 'ed', 'label' => 'Editor' }] } + } + ) + attribute :relationships, + Valkyrie::Types::Array.of(Dry::Types['hash']).meta( + subproperties: { + 'related_item' => { 'type' => 'work_or_url' }, + 'relationship_type' => { 'type' => 'controlled', 'authority' => 'relationship_type' } + }, + display_label: { 'default' => 'Related works' }, + view: { 'display' => 'card' } + ) + # Required at the compound level (non-flexible `required: true`) with two + # required sub-properties and one optional. + attribute :required_compound, + Valkyrie::Types::Array.of(Dry::Types['hash']).meta( + required: true, + subproperties: { + 'item' => { 'type' => 'string', 'required' => true }, + 'kind' => { 'type' => 'string', 'required' => true }, + 'note' => { 'type' => 'string' } + } + ) + # Required at the compound level via m3 minimum cardinality (flexible). + attribute :cardinality_compound, + Valkyrie::Types::Array.of(Dry::Types['hash']).meta( + cardinality: { 'minimum' => 1 }, + subproperties: { 'value' => { 'type' => 'string' } } + ) + end + end + + subject(:schema) { described_class.for(resource_class) } + + describe '#compound_names' do + it 'returns only the attributes that declare subproperties' do + expect(schema.compound_names).to contain_exactly(:contributors, :identifiers, :agent, :relationships, + :required_compound, :cardinality_compound) + end + + it 'does not treat a plain scalar attribute as a compound' do + expect(schema.compound_names).not_to include(:title) + end + end + + describe 'display mode (view: { display: card })' do + describe '#inline_compound_names' do + it 'excludes compounds declared as cards' do + expect(schema.inline_compound_names).to contain_exactly(:contributors, :identifiers, :agent, + :required_compound, :cardinality_compound) + end + end + + describe '#card_compound_names' do + it 'returns only compounds declared as cards' do + expect(schema.card_compound_names).to contain_exactly(:relationships) + end + end + + describe '#card?' do + it 'is true for a compound declared view: { display: card }' do + expect(schema.card?(:relationships)).to be true + expect(schema.card?('relationships')).to be true + end + + it 'is false for an inline (default) compound' do + expect(schema.card?(:contributors)).to be false + end + + it 'is false for a non-compound attribute' do + expect(schema.card?(:title)).to be false + end + end + + it 'records the display_mode on the definition' do + expect(schema.definition_for(:relationships)[:display_mode]).to eq(:card) + expect(schema.definition_for(:contributors)[:display_mode]).to eq(:inline) + end + end + + describe 'display_label' do + it 'normalizes a declared display_label to a locale hash' do + expect(schema.definition_for(:relationships)[:display_label]).to eq('default' => 'Related works') + end + + it 'is nil when none is declared' do + expect(schema.definition_for(:contributors)[:display_label]).to be_nil + end + end + + describe 'required declarations' do + describe '#required?' do + it 'is true when the compound declares required: true' do + expect(schema.required?(:required_compound)).to be true + end + + it 'is true when a minimum cardinality of 1 is declared (m3/flexible)' do + expect(schema.required?(:cardinality_compound)).to be true + end + + it 'is false for an optional compound' do + expect(schema.required?(:relationships)).to be false + end + + it 'is false for a non-compound attribute' do + expect(schema.required?(:title)).to be false + end + end + + describe '#required_subproperty_keys' do + it 'returns the sub-properties declared required: true' do + expect(schema.required_subproperty_keys(:required_compound)).to eq(%w[item kind]) + end + + it 'is empty when no sub-property is required' do + expect(schema.required_subproperty_keys(:relationships)).to eq([]) + end + + it 'is empty for a non-compound' do + expect(schema.required_subproperty_keys(:title)).to eq([]) + end + end + + it 'records required on the definition (subproperty + compound level)' do + definition = schema.definition_for(:required_compound) + expect(definition[:required]).to be true + expect(definition[:subproperties]['item'][:required]).to be true + expect(definition[:subproperties]['note'][:required]).to be false + end + end + + describe '#compound?' do + it 'is true for a declared compound' do + expect(schema.compound?(:contributors)).to be true + expect(schema.compound?('contributors')).to be true + end + + it 'is false for a non-compound attribute' do + expect(schema.compound?(:title)).to be false + end + end + + describe '#subproperty_keys' do + it 'returns the ordered sub-property keys' do + expect(schema.subproperty_keys(:contributors)).to eq(%w[given_name family_name role_label]) + end + + it 'is empty for a non-compound' do + expect(schema.subproperty_keys(:title)).to eq([]) + end + end + + describe '#definition_for' do + it 'normalizes subproperties with type, authority, index_keys, display, group, cols and as' do + definition = schema.definition_for(:contributors) + expect(definition[:subproperties]['role_label']) + .to eq(type: 'controlled', authority: 'contributor_role', values: nil, index_keys: [], + display: false, required: false, group: 'role', cols: 6, as: nil) + expect(definition[:subproperties]['given_name']) + .to eq(type: 'string', authority: nil, values: nil, + index_keys: %w[contributors_given_name_sim contributors_given_name_tesim], + display: true, required: false, group: 'identity', cols: 6, as: nil) + end + + it 'reads per-sub-property index_keys (literal Solr field names)' do + expect(schema.definition_for(:contributors)[:subproperties]['family_name'][:index_keys]) + .to eq(%w[contributors_family_name_tesim]) + end + + it 'defaults a sub-property with no index declaration to no index_keys' do + expect(schema.definition_for(:agent)[:subproperties]['agent_name'][:index_keys]).to eq([]) + end + + it 'defaults display to true and honors display: false' do + expect(schema.definition_for(:contributors)[:subproperties]['given_name'][:display]).to be true + expect(schema.definition_for(:contributors)[:subproperties]['role_label'][:display]).to be false + end + + it 'defaults cols to 6 when no form placement is declared' do + expect(schema.definition_for(:identifiers)[:subproperties]['value'][:cols]).to eq(6) + end + + it 'normalizes an inline controlled-vocabulary values list to [label, id] pairs' do + values = schema.definition_for(:agent)[:subproperties]['agent_role'][:values] + expect(values).to eq([%w[Author Author], %w[Editor ed]]) + end + + it 'reconstructs groups from subproperty membership and the parent group labels' do + groups = schema.definition_for(:contributors)[:groups] + expect(groups).to eq([ + { key: 'identity', label: 'Identity', fields: %w[given_name family_name] }, + { key: 'role', label: 'Role', fields: %w[role_label] } + ]) + end + + it 'puts subproperties with no group in a single default (unlabeled) group' do + groups = schema.definition_for(:identifiers)[:groups] + expect(groups).to eq([{ key: nil, label: nil, fields: %w[value identifier_type] }]) + end + end + + describe '.for' do + it 'builds from a resource instance' do + expect(described_class.for(resource_class.new).compound_names) + .to contain_exactly(:contributors, :identifiers, :agent, :relationships, + :required_compound, :cardinality_compound) + end + end + + describe 'instance schema source ordering (singleton before class)' do + # build_definitions is first-source-wins per name, so a flexible instance's + # singleton schema (current version) must precede its class schema (frozen + # at class-load) for a freshly-uploaded profile to win. See + # #schema_sources_for. + it 'lists the singleton schema before the class schema for an instance' do + class_schema = double('class_schema') + singleton_schema = double('singleton_schema') + resource = double('resource', + class: double('klass', schema: class_schema), + singleton_class: double('singleton', schema: singleton_schema)) + + sources = described_class.send(:schema_sources_for, resource) + expect(sources).to eq([singleton_schema, class_schema]) + end + + it 'uses the class schema alone for a class argument (non-flexible)' do + schema_obj = double('schema') + klass = Class.new { define_singleton_method(:schema) { schema_obj } } + expect(described_class.send(:schema_sources_for, klass)).to eq([schema_obj]) + end + end + + describe '.for_solr_document' do + # The attribute map a flexible schema loader returns for a version: + # `{ name => dry_type_with_meta }` (with subproperties folded into the + # parent meta) — the same shape SchemaLoader#attributes_for produces. Show + # pages resolve compounds from this (via schema_version) without loading the + # resource. + let(:version_attributes) do + resource_class.schema.index_by(&:name) + end + + context 'in flexible mode (document has a schema_version)' do + let(:document) { instance_double(SolrDocument, hydra_model: resource_class) } + + before do + allow(document).to receive(:[]).with('schema_version_ssi').and_return('7') + loader = instance_double(Hyrax::M3SchemaLoader) + allow(Hyrax::Schema).to receive(:m3_schema_loader).and_return(loader) + allow(loader).to receive(:attributes_for) + .with(schema: 'TestResourceWithCompounds', version: '7', contexts: []) + .and_return(version_attributes) + end + + it 'resolves compounds from the version attribute map (no resource load)' do + expect(described_class.for_solr_document(document).card_compound_names) + .to contain_exactly(:relationships) + end + end + + context 'when the document has no schema_version (non-flexible)' do + let(:document) { instance_double(SolrDocument, hydra_model: resource_class) } + + before { allow(document).to receive(:[]).with('schema_version_ssi').and_return(nil) } + + it 'falls back to the model class schema' do + expect(described_class.for_solr_document(document).compound_names) + .to include(:relationships, :agent) + end + end + end +end diff --git a/spec/services/hyrax/compound_subproperty_labeler_spec.rb b/spec/services/hyrax/compound_subproperty_labeler_spec.rb new file mode 100644 index 0000000000..c400dde88b --- /dev/null +++ b/spec/services/hyrax/compound_subproperty_labeler_spec.rb @@ -0,0 +1,50 @@ +# frozen_string_literal: true + +RSpec.describe Hyrax::CompoundSubpropertyLabeler do + describe '.label_for' do + it 'returns the value unchanged for a non-controlled sub-property' do + spec = { type: 'string', authority: nil, values: nil } + expect(described_class.label_for(spec, 'plain text')).to eq('plain text') + end + + it 'returns the value unchanged when spec is nil' do + expect(described_class.label_for(nil, 'whatever')).to eq('whatever') + end + + it 'returns blank values unchanged' do + spec = { type: 'controlled', authority: 'x', values: nil } + expect(described_class.label_for(spec, '')).to eq('') + end + + context 'with an inline values list' do + let(:spec) { { type: 'controlled', authority: nil, values: [%w[Author author], %w[Editor ed]] } } + + it 'translates the stored id to its label' do + expect(described_class.label_for(spec, 'ed')).to eq('Editor') + end + + it 'falls back to the id when no matching option exists' do + expect(described_class.label_for(spec, 'unknown')).to eq('unknown') + end + end + + context 'with a QA authority' do + let(:spec) { { type: 'controlled', authority: 'rights_statements', values: nil } } + let(:service) { instance_double(Hyrax::TolerantSelectService) } + + before do + allow(Hyrax::TolerantSelectService).to receive(:new).with('rights_statements').and_return(service) + end + + it 'translates the stored id to the authority term' do + allow(service).to receive(:label).and_return('In Copyright') + expect(described_class.label_for(spec, 'http://rightsstatements.org/vocab/InC/1.0/')).to eq('In Copyright') + end + + it 'falls back to the id on lookup error' do + allow(service).to receive(:label).and_raise(StandardError) + expect(described_class.label_for(spec, 'http://example.org/x')).to eq('http://example.org/x') + end + end + end +end diff --git a/spec/services/hyrax/compound_work_resolver_spec.rb b/spec/services/hyrax/compound_work_resolver_spec.rb new file mode 100644 index 0000000000..844dd18767 --- /dev/null +++ b/spec/services/hyrax/compound_work_resolver_spec.rb @@ -0,0 +1,82 @@ +# frozen_string_literal: true + +RSpec.describe Hyrax::CompoundWorkResolver do + # Stub a Solr hit for `id` with the given fields (e.g. has_model_ssim/title). + def stub_solr(fields) + allow(Hyrax::SolrService).to receive(:query).and_return([fields].compact) + end + + describe '.url?' do + it 'is true for http(s) URLs' do + expect(described_class.url?('https://example.org/x')).to be true + expect(described_class.url?('http://example.org')).to be true + end + + it 'is false for a work id (UUID/noid)' do + expect(described_class.url?('691b0622-3f77-48ad-a00f-00c254b83165')).to be false + expect(described_class.url?('abc123')).to be false + end + + it 'is false for blank' do + expect(described_class.url?('')).to be false + expect(described_class.url?(nil)).to be false + end + end + + describe '.resolve' do + let(:id) { 'rec-123' } + + it 'links a work to its work show page' do + stub_solr('id' => id, 'title_tesim' => ['A Work'], 'has_model_ssim' => ['GenericWork']) + title, path = described_class.resolve(id) + expect(title).to eq('A Work') + expect(path).to eq("/concern/generic_works/#{id}") + end + + it 'links a collection to its collection show page' do + stub_solr('id' => id, 'title_tesim' => ['A Collection'], 'has_model_ssim' => [Hyrax.config.collection_model]) + title, path = described_class.resolve(id) + expect(title).to eq('A Collection') + expect(path).to eq("/collections/#{id}") + end + + it 'falls back to the catalog route when the model is not a routable work/collection' do + stub_solr('id' => id, 'title_tesim' => ['Mystery']) + _title, path = described_class.resolve(id) + expect(path).to eq("/catalog/#{id}") + end + + it 'returns nil when nothing matches (so the caller renders plain text)' do + stub_solr(nil) + expect(described_class.resolve(id)).to be_nil + end + + it 'returns nil when the Solr lookup raises' do + allow(Hyrax::SolrService).to receive(:query).and_raise(StandardError) + expect(described_class.resolve(id)).to be_nil + end + end + + describe '.title_and_path' do + let(:id) { 'rec-123' } + + it 'resolves the title and the work show path' do + stub_solr('id' => id, 'title_tesim' => ['Resolved Title'], 'has_model_ssim' => ['GenericWork']) + title, path = described_class.title_and_path(id) + expect(title).to eq('Resolved Title') + expect(path).to eq("/concern/generic_works/#{id}") + end + + it 'falls back to the id when no record is indexed' do + stub_solr(nil) + title, = described_class.title_and_path(id) + expect(title).to eq(id) + end + + it 'falls back to the id when the Solr lookup raises' do + allow(Hyrax::SolrService).to receive(:query).and_raise(StandardError) + title, = described_class.title_and_path(id) + expect(title).to eq(id) + end + end +end diff --git a/spec/services/hyrax/flexible_schema_validators/compound_validator_spec.rb b/spec/services/hyrax/flexible_schema_validators/compound_validator_spec.rb new file mode 100644 index 0000000000..e1bd0e5cfc --- /dev/null +++ b/spec/services/hyrax/flexible_schema_validators/compound_validator_spec.rb @@ -0,0 +1,119 @@ +# frozen_string_literal: true + +RSpec.describe Hyrax::FlexibleSchemaValidators::CompoundValidator do + subject(:validator) { described_class.new(profile: profile, errors: errors) } + let(:errors) { [] } + + def t(key, **opts) + I18n.t("hyrax.flexible_schema_validators.compound_validator.errors.#{key}", **opts) + end + + describe '#validate!' do + context 'with a well-formed compound (parent + flat subproperties)' do + let(:profile) do + { + 'properties' => { + 'participants' => { 'type' => 'hash' }, + 'participant_title' => { 'type' => 'string', 'subproperty_of' => 'participants' }, + 'participant_name' => { 'type' => 'string', 'subproperty_of' => 'participants', 'indexing' => %w[participant_name_tesim] }, + 'participant_role' => { 'type' => 'controlled', 'subproperty_of' => 'participants', + 'values' => %w[Author Editor], 'indexing' => %w[participant_role_sim] } + } + } + end + + it 'records no errors' do + validator.validate! + expect(errors).to be_empty + end + end + + context 'with a controlled subproperty that uses an authority instead of inline values' do + let(:profile) do + { + 'properties' => { + 'participants' => { 'type' => 'hash' }, + 'participant_role' => { 'type' => 'controlled', 'subproperty_of' => 'participants', 'authority' => 'participant_role' } + } + } + end + + it 'is valid (authority satisfies the option-source requirement)' do + validator.validate! + expect(errors).to be_empty + end + end + + context 'with a controlled subproperty that declares neither authority nor values' do + let(:profile) do + { + 'properties' => { + 'participants' => { 'type' => 'hash' }, + 'participant_role' => { 'type' => 'controlled', 'subproperty_of' => 'participants' } + } + } + end + + it 'records an error keyed on the subproperty' do + validator.validate! + expect(errors).to include(t('controlled_without_source', property: 'participant_role')) + end + end + + context 'with a top-level indexing declaration on the compound parent' do + let(:profile) do + { + 'properties' => { + 'participants' => { 'type' => 'hash', 'indexing' => %w[participants_tesim] }, + 'participant_name' => { 'type' => 'string', 'subproperty_of' => 'participants' } + } + } + end + + it 'records an error that indexing belongs on subproperties' do + validator.validate! + expect(errors).to include(t('top_level_indexing', property: 'participants')) + end + end + + context 'with a subproperty whose parent is not a declared type: hash compound' do + let(:profile) do + { + 'properties' => { + # parent missing entirely + 'participant_name' => { 'type' => 'string', 'subproperty_of' => 'participants' }, + # parent exists but is not a hash + 'note' => { 'type' => 'string' }, + 'note_detail' => { 'type' => 'string', 'subproperty_of' => 'note' } + } + } + end + + it 'records an unknown_parent error for each' do + validator.validate! + expect(errors).to include(t('unknown_parent', property: 'participant_name', parent: 'participants')) + expect(errors).to include(t('unknown_parent', property: 'note_detail', parent: 'note')) + end + end + + context 'with a hash property that is not a compound (no subproperties, e.g. redirects)' do + let(:profile) do + { 'properties' => { 'redirects' => { 'type' => 'hash' } } } + end + + it 'ignores it (not a compound)' do + validator.validate! + expect(errors).to be_empty + end + end + + context 'with no compound properties at all' do + let(:profile) { { 'properties' => { 'title' => { 'type' => 'string' } } } } + + it 'is silent' do + validator.validate! + expect(errors).to be_empty + end + end + end +end diff --git a/spec/services/hyrax/simple_schema_loader_spec.rb b/spec/services/hyrax/simple_schema_loader_spec.rb index 25fc43dcd8..98732db2af 100644 --- a/spec/services/hyrax/simple_schema_loader_spec.rb +++ b/spec/services/hyrax/simple_schema_loader_spec.rb @@ -140,7 +140,7 @@ let(:permissive_schema) { schema_loader.permissive_schema_for_valkrie_adapter } it 'provides the expected hash' do - expect(permissive_schema.size).to eq(68) + expect(permissive_schema.size).to eq(72) expect(permissive_schema.values.all? { |v| v.is_a? RDF::URI }).to be_truthy expect(permissive_schema.values.all? { |v| v.value.present? }).to be_truthy expect(permissive_schema[:sample_attribute].value).to eq("http://hyrax-example.com/sample_attribute") diff --git a/spec/validators/hyrax/compound_entry_validator_spec.rb b/spec/validators/hyrax/compound_entry_validator_spec.rb new file mode 100644 index 0000000000..6868b74d82 --- /dev/null +++ b/spec/validators/hyrax/compound_entry_validator_spec.rb @@ -0,0 +1,99 @@ +# frozen_string_literal: true + +RSpec.describe Hyrax::CompoundEntryValidator do + let(:record_class) do + Class.new do + include ActiveModel::Validations + attr_accessor :relationships + validates_with Hyrax::CompoundEntryValidator + end + end + let(:record) { record_class.new.tap { |r| r.relationships = entries } } + let(:entries) { [] } + + # A schema with one required-sub-property compound (`relationships`), optional + # at the compound level. + let(:definition) do + { required: false, + subproperties: { 'related_item' => { type: 'work_or_url', required: true }, + 'relationship_type' => { type: 'controlled', required: true } } } + end + let(:schema) { instance_double(Hyrax::CompoundSchema, definitions: { relationships: definition }) } + + before do + allow(Hyrax::CompoundSchema).to receive(:for).and_return(schema) + end + + # Compound errors are attached to :base (so the work and collection forms, + # which render errors differently, both show them cleanly), and the message + # names the compound. + context 'with no rows (optional compound)' do + it 'is valid' do + record.valid? + expect(record.errors[:base]).to be_empty + end + end + + context 'with a complete row' do + let(:entries) { [{ 'related_item' => 'work-1', 'relationship_type' => 'References' }] } + + it 'is valid' do + record.valid? + expect(record.errors[:base]).to be_empty + end + end + + context 'with a row missing a required sub-property' do + let(:entries) { [{ 'related_item' => 'work-1' }] } + + it 'adds one base error' do + record.valid? + expect(record.errors[:base].size).to eq(1) + end + + it 'names the compound and the missing sub-property in the message' do + # Assert the English wording under :en, so the test is independent of the + # suite's default locale (other locales need only the same key structure). + I18n.with_locale(:en) do + record.valid? + expect(record.errors[:base].first).to include('Relationships').and include('Relationship type') + end + end + end + + context 'when the compound itself is required' do + let(:definition) { super().merge(required: true) } + + context 'and empty' do + it 'flags the empty compound by name' do + # English wording; see the note above on locale. + I18n.with_locale(:en) do + record.valid? + expect(record.errors[:base].first).to include('Relationships').and include('at least one entry') + end + end + end + + context 'and populated completely' do + let(:entries) { [{ 'related_item' => 'work-1', 'relationship_type' => 'References' }] } + + it 'is valid' do + record.valid? + expect(record.errors[:base]).to be_empty + end + end + end + + context 'when the record does not respond to the compound reader' do + let(:record) do + Class.new do + include ActiveModel::Validations + validates_with Hyrax::CompoundEntryValidator + end.new + end + + it 'skips it without error' do + expect { record.valid? }.not_to raise_error + end + end +end