Expand context when the value is an URI and expasion was requested in the config #204

elrayle · 2019-02-14T20:37:44Z

Adds:

expands context for URIs to optionally include label and id

Fixes:

group label improperly passed default to I18n
clean up some comments
extracts parts of longer methods into private methods to avoid rubocop complaints and/or to keep DRY

Examples:

Regular context (from previous PR)

{
    property: "Authoritative Label",
    values: [
        "Science films"
    ],
    selectable: true,
    drillable: false
},

Expanding URI values in context (with this PR)

{
    group: "Hierarchy",
    property: "Narrower",
    values: [
        {
            uri: "http://id.loc.gov/authorities/genreForms/gf2011026416",
            id: "gf2011026416",
            label: "Nature films"
        }
    ],
    selectable: true,
    drillable: true
},

elrayle · 2019-02-14T20:44:40Z

app/models/qa/linked_data/config/context_map.rb

@@ -68,7 +68,7 @@ def add_group(group_id, group_map)
            return if groups.key? group_id
            i18n_key = Qa::LinkedData::Config::Helper.fetch(group_map, :group_label_i18n, nil)
            default = Qa::LinkedData::Config::Helper.fetch(group_map, :group_label_default, nil)
-            return groups[group_id] = I18n.t(i18n_key, default) if i18n_key.present?
+            return groups[group_id] = I18n.t(i18n_key, default: default) if i18n_key.present?


BUGFIX: passing default incorrectly to i18n

elrayle · 2019-02-15T18:30:03Z

app/models/qa/linked_data/config/context_property_map.rb

        def drillable?
          @drillable
        end

+        def group?
+          group_id.present?
+        end


READABILITY: Just moved group? up so all the ? methods are together. No code change.

elrayle · 2019-02-15T18:31:23Z

app/models/qa/linked_data/config/context_property_map.rb

-          output.present? ? output['property'].uniq : nil
-        rescue
-          'PARSE ERROR'
+          ldpath_evaluate(basic_program, graph, subject_uri)


DRY: Moved the ldpath evaluate process to a private method so it can be shared by multiple methods.

elrayle · 2019-02-15T18:39:57Z

app/services/qa/linked_data/mapper/context_mapper_service.rb

@@ -21,7 +21,7 @@ class << self
          def map_context(graph:, context_map:, subject_uri:)
            context = []
            context_map.properties.each do |property_map|
-              values = property_map.values(graph, subject_uri)
+              values = values(property_map, graph, subject_uri)


RUBOCOP: Extracted to a private method to appease rubocop.

@elrayle you may be sad later on when you try and find values. It is not a very distinct name. Is it really expanded_values?

Addressed by renaming method values to property_values.

elrayle · 2019-02-15T18:40:26Z

app/services/qa/linked_data/mapper/search_results_mapper_service.rb

-                context = context_mapper_service.map_context(graph: graph, context_map: context_map, subject_uri: subject) if context_map.present?
-                value_map[:context] = context
-                value_map
+                map_context(graph, sort_key, context_map, value_map, subject)


RUBOCOP: Extracted to a private method to appease rubocop.

elrayle · 2019-02-15T18:41:14Z

app/services/qa/linked_data/mapper/search_results_mapper_service.rb

              end
-              search_matches << values unless sort_key.present? && values[sort_key].blank?
+              search_matches << values if result_subject? values, sort_key


DRY: Extracted to a private method so the check for sort_key can happen here and in the context mapping code.

elrayle · 2019-02-15T18:42:18Z

app/services/qa/linked_data/mapper/search_results_mapper_service.rb

+
+            def map_context(graph, sort_key, context_map, value_map, subject)
+              return value_map if context_map.blank?
+              return value_map unless result_subject? value_map, sort_key


PERFORMANCE: Added this check because we shouldn't take the time to map context if the current subject isn't an actual search result.

elrayle · 2019-02-15T18:44:14Z

spec/models/linked_data/config/context_map_spec.rb

-    allow(I18n).to receive(:t).with('qa.linked_data.authority.locnames_ld4l_cache.dates', 'default_Dates').and_return('Dates')
-    allow(I18n).to receive(:t).with('qa.linked_data.authority.locnames_ld4l_cache.authoritative_label', 'default_Authoritative Label').and_return('Authoritative Label')
-    allow(I18n).to receive(:t).with('qa.linked_data.authority.locnames_ld4l_cache.birth_date', 'default_Birth').and_return('Birth')
-    allow(I18n).to receive(:t).with('qa.linked_data.authority.locnames_ld4l_cache.death_date', 'default_Death').and_return('Death')


DEADCODE: These weren't needed for the tests to pass.

elrayle · 2019-02-15T18:44:47Z

spec/models/linked_data/config/context_map_spec.rb

@@ -75,6 +71,10 @@
  describe '#group_label' do
    context 'when map defines group_label_i18n key' do
      context 'and i18n translation is defined in locales' do
+        before do
+          allow(I18n).to receive(:t).with('qa.linked_data.authority.locnames_ld4l_cache.dates', default: 'default_Dates').and_return('Dates')
+        end


READABILITY: Moved closer to test.

carolyncole · 2019-02-15T19:43:28Z

app/models/qa/linked_data/config/context_property_map.rb

+            output = program.evaluate subject_uri, graph
+            output.present? ? output['property'].uniq : nil
+          rescue
+            'PARSE ERROR'


@elrayle I know this is not new code, but where does the string show up to the user? I'm wondering if it should be logged.

@Cam156 Are you asking about the 'PARSE ERROR' string? This is a stop gap from the ldpath gem failing to parse some ldpaths. So it would show up here...

{ group: "Hierarchy", property: "Narrower", values: [ 'PARSE ERROR' ], selectable: true, drillable: true },

Not ideal. Better is perhaps logging and returning nothing for the property that wouldn't parse or values: ['']

@elrayle I was just wondering if it was something people would notice. Rescuing errors and silently handling them is something I have done and regretted.

With 2nd commit, reworked the results so that the error is in a separate field in the hash...

{ group: "Hierarchy", property: "Narrower", values: [], selectable: true, drillable: true, error: "LDPATH PARSE ERROR (See log for more information)" },

carolyncole

@elrayle A couple small suggestions, but nothing blocking

carolyncole · 2019-02-15T19:46:20Z

app/services/qa/linked_data/mapper/context_mapper_service.rb

@@ -21,7 +21,7 @@ class << self
          def map_context(graph:, context_map:, subject_uri:)
            context = []
            context_map.properties.each do |property_map|
-              values = property_map.values(graph, subject_uri)
+              values = values(property_map, graph, subject_uri)


@elrayle you may be sad later on when you try and find values. It is not a very distinct name. Is it really expanded_values?

… the config

carolyncole · 2019-02-18T17:54:39Z

app/models/qa/linked_data/config/context_property_map.rb

+          end
+
+          def ldpath_evaluate(program, graph, subject_uri)
+            return PARSE_ERROR_VALUE if program.blank?


@elrayle Hate to cause issues, but did you mean nil? Is the empty string a valid value?

With 2nd commit, now returns an empty array when an error occurs. The value on error is set in ContextMapperService using the ContextPropertyMap::VALUE_ON_ERROR constant.

carolyncole · 2019-02-19T14:11:19Z

app/services/qa/linked_data/mapper/context_mapper_service.rb

              property_info = {}
              property_info["group"] = context_map.group_label(property_map.group_id) if property_map.group?
              property_info["property"] = property_map.label
              property_info["values"] = values
              property_info["selectable"] = property_map.selectable?
              property_info["drillable"] = property_map.drillable?
+              property_info["error"] = error if error.present?


elrayle changed the title ~~Expand context when the value is an URI and expasion was requested in…~~ Expand context when the value is an URI and expasion was requested in the config Feb 14, 2019

elrayle changed the title ~~Expand context when the value is an URI and expasion was requested in the config~~ [WIP] Expand context when the value is an URI and expasion was requested in the config Feb 14, 2019

elrayle commented Feb 14, 2019

View reviewed changes

elrayle force-pushed the ld_refactor_expansion_ldpath branch from 6c97a5d to 0c6de05 Compare February 15, 2019 17:01

elrayle commented Feb 15, 2019

View reviewed changes

elrayle changed the title ~~[WIP] Expand context when the value is an URI and expasion was requested in the config~~ Expand context when the value is an URI and expasion was requested in the config Feb 15, 2019

elrayle requested review from carolyncole and randalldfloyd February 15, 2019 18:46

elrayle self-assigned this Feb 15, 2019

carolyncole reviewed Feb 15, 2019

View reviewed changes

carolyncole approved these changes Feb 15, 2019

View reviewed changes

Expand context when the value is an URI and expasion was requested in…

0df08a6

… the config

elrayle force-pushed the ld_refactor_expansion_ldpath branch from 0c6de05 to 0df08a6 Compare February 18, 2019 17:37

carolyncole reviewed Feb 18, 2019

View reviewed changes

process errors from ldpath

e333fc3

carolyncole approved these changes Feb 19, 2019

View reviewed changes

carolyncole reviewed Feb 19, 2019

View reviewed changes

carolyncole merged commit 9bb5987 into master Feb 19, 2019

carolyncole deleted the ld_refactor_expansion_ldpath branch February 19, 2019 14:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expand context when the value is an URI and expasion was requested in the config #204

Expand context when the value is an URI and expasion was requested in the config #204

elrayle commented Feb 14, 2019 •

edited

elrayle Feb 14, 2019 •

edited

elrayle Feb 15, 2019 •

edited

elrayle Feb 15, 2019

elrayle Feb 15, 2019 •

edited

carolyncole Feb 15, 2019

elrayle Feb 18, 2019

elrayle Feb 15, 2019 •

edited

elrayle Feb 15, 2019

elrayle Feb 15, 2019

elrayle Feb 15, 2019

elrayle Feb 15, 2019

carolyncole Feb 15, 2019

elrayle Feb 15, 2019

carolyncole Feb 15, 2019

elrayle Feb 18, 2019

carolyncole left a comment

carolyncole Feb 15, 2019

carolyncole Feb 18, 2019

elrayle Feb 18, 2019 •

edited

carolyncole Feb 19, 2019

Expand context when the value is an URI and expasion was requested in the config #204

Expand context when the value is an URI and expasion was requested in the config #204

Conversation

elrayle commented Feb 14, 2019 • edited

elrayle Feb 14, 2019 • edited

Choose a reason for hiding this comment

elrayle Feb 15, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elrayle Feb 15, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elrayle Feb 15, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carolyncole left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elrayle Feb 18, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elrayle commented Feb 14, 2019 •

edited

elrayle Feb 14, 2019 •

edited

elrayle Feb 15, 2019 •

edited

elrayle Feb 15, 2019 •

edited

elrayle Feb 15, 2019 •

edited

elrayle Feb 18, 2019 •

edited