Refactor of marc856generator to serialize 856 string #4432

aaron-collier · 2023-03-10T02:00:00Z

Why was this change made? 🤔

This refactors marc856generator to return an array of hashed data used to create stub 856 records and moves all symphony specific serialization into SymphonyWriter

Example output with 2 previous catkey values:

        [
          {
            catalog_record_id: '123',
            druid: bare_druid
          },
          {
            catalog_record_id: '456',
            druid: bare_druid
          },
          {
            catalog_record_id: '8832162',
            druid: bare_druid,
            indicators: '41',
            subfields: [
              { code: 'u', value: "https://purl.stanford.edu/#{bare_druid}" },
              { code: 'x', value: 'SDR-PURL' },
              { code: 'x', value: 'item' },
              { code: 'x', value: "file:#{bare_druid}%2Fwt183gy6220_00_0001.jp2" },
              { code: 'x', value: "collection:#{collection_bare_druid}:8832162:Collection label & A Special character" },
              { code: 'x', value: 'rights:world' }
            ]
          }
        ]

ndushay

a few small notes on ...stuff we didn't fully understand before today. Tagging @justinlittman as well.

app/services/catalog/marc_856_generator.rb

app/services/catalog/symphony_writer.rb

ndushay

We're going to talk about this; just making extra sure we don't merge by accident.

ndushay · 2023-03-10T02:40:48Z

Possibly useful for later: /sdrpurl_transfer is where to find the output file on a dsa box.

example:

9876	fg794df6815	.856. 41|uhttps://sul-purl-stage.stanford.edu/fg794df6815|xSDR-PURL|xcollection|xrights:world
4567	xj052tw5711	
9876	fg794df6815	
3060835	ff390ph1693	.856. 40|uhttps://sul-purl-stage.stanford.edu/ff390ph1693|xSDR-PURL|xitem|xrights:world
3060835	ff390ph1693	.856. 40|uhttps://sul-purl-stage.stanford.edu/ff390ph1693|xSDR-PURL|xitem|xrights:world
3060835	ff390ph1693	.856. 40|uhttps://sul-purl-stage.stanford.edu/ff390ph1693|xSDR-PURL|xitem|xrights:world
666	fk408xq4386	.856. 40|zAvailable to Stanford-affiliated users.|uhttps://sul-purl-stage.stanford.edu/fk408xq4386|xSDR-PURL|xitem|xrights:group=stanford

app/services/catalog/marc_856_generator.rb

ndushay

I'm happy to pair or talk through my thinking on the 856 generator vs. the symphony writer -- what logic lives where and what the data structure needs to be.

My vision is that all the logic for exactly what data goes in the 856 lives in the 856 generator, and the writer classes are really stupid and just munge the data into the form needed by the consuming ILS. In Symphony's case, one string per identiier, with periods and "|" as markup. In Folio's case, not sure, but maybe a particular JSON structure.

app/services/catalog/marc_856_generator.rb

ndushay · 2023-03-13T18:04:28Z

app/services/catalog/marc_856_generator.rb

@@ -2,7 +2,7 @@

 # rubocop:disable Metrics/ClassLength
 module Catalog
-  # Creates a stub MARC 856 field (currently for transferring to symphony only) given a cocina object.
+  # Creates a hash that is used to stub the MARC 856 field (currently for transferring to symphony only) given a cocina object.


ditch "currenty for symphony only" clause

And now were not talking about a stub any longer -- this class is returning identifiers and then a hash of data to build a plain old marc 856 field, or no such hash to imply 856 matching druid should be removed.

@justinlittman did you want to keep the identifier information and whether or not it has an 856 associated in THIS class? (makes sense to ... just confirming)

app/services/catalog/marc_856_generator.rb

app/services/catalog/symphony_writer.rb

aaron-collier · 2023-03-13T21:20:47Z

app/services/catalog/marc_856_generator.rb

+          collections,
+          parts,
+          rights
+        ].compact.flatten


Note that this compact and flatten removes any nil values returned from the list of methods, and then flattens the ones that return arrays like collections and rights

ndushay

changes to 856 looking good; a few small quibbles. I also float the idea that you migh somehow indicate that, for example, "access" is returning the hash for subfield z. Maybe access_subfield_z or subfield_z_access or maybe you have something better.

app/services/catalog/marc_856_generator.rb

ndushay

Approving, modulo:

look for catkey and ckey in the 856 code and spec (can leave for symphony writer)
typo in comment line 55

having Andrew (?) try it in test.

ndushay · 2023-03-13T22:18:49Z

spec/services/catalog/symphony_writer_spec.rb

+          }
+        ]
+      end
+      let(:marc856) do


maybe call this marc856file or something?

ndushay · 2023-03-13T22:18:54Z

spec/services/catalog/symphony_writer_spec.rb

+          }
+        ]
+      end
+      let(:marc856) do


maybe call this marc856file or something?

ndushay · 2023-03-13T22:21:01Z

app/services/catalog/marc_856_generator.rb


      # first create "blank" records for any previous catkeys
-      records = previous_ckeys.map { |previous_catkey| new_identifier_record(previous_catkey) }
+      records = previous_catalog_record_ids.map { |previous_catkey| new_identifier_record(previous_catkey) }


ugh - missed this before - previous_catkey

ndushay · 2023-03-13T22:21:19Z

app/services/catalog/marc_856_generator.rb

-      new856 += get_x2_part_info unless get_x2_part_info.nil?
-      new856 += get_x2_rights_info unless get_x2_rights_info.nil?
-      new856
+    # NOTE: this is the data to go in an 856 field (not a (not a properly formatted 856 field nor a marc record)


not a not a

ndushay · 2023-03-13T22:23:29Z

app/services/catalog/marc_856_generator.rb

+    #                         (future types of sets to describe other aggregations like albums, atlases, etc)
+    # x - barcode (optional): the barcode if known (<identityMetadata><otherId name="barcode">, recorded as barcode:barcode-value
+    # x - thumbnail (optional): the file-id to be used as thumb if available, recorded as file:file-id-value
+    # x - collections (optional): Collection(s) this object is a member of, formatted as: druid:catkey:label


ndushay · 2023-03-13T22:23:46Z

app/services/catalog/marc_856_generator.rb

+    def subfield_x_thumbnail
+      return unless @thumbnail_service.thumb
+
+      { code: 'x', value: "file:#{ERB::Util.url_encode(@thumbnail_service.thumb)}" }
    end

    # returns the collection information subfields if exists
    # @return [String] the collection information druid-value:catkey-value:title format


ndushay · 2023-03-13T22:23:58Z

app/services/catalog/marc_856_generator.rb

      return unless @cocina_object.respond_to?(:structural) && @cocina_object.structural

      collections = @cocina_object.structural.isMemberOf
-      collection_info = ''
+      collection_info = []

      collections.each do |collection_druid|
        collection = CocinaObjectStore.find(collection_druid)
        next unless released_to_searchworks?(collection)

        catkey = collection.identification&.catalogLinks&.find { |link| link.catalog == 'symphony' }


ndushay · 2023-03-13T22:24:05Z

app/services/catalog/marc_856_generator.rb


      collections.each do |collection_druid|
        collection = CocinaObjectStore.find(collection_druid)
        next unless released_to_searchworks?(collection)

        catkey = collection.identification&.catalogLinks&.find { |link| link.catalog == 'symphony' }
-        collection_info += "|xcollection:#{collection.externalIdentifier.sub('druid:',
-                                                                             '')}:#{catkey&.catalogRecordId}:#{Cocina::Models::Builders::TitleBuilder.build(collection.description.title)}"
+        collection_info << { code: 'x', value: "collection:#{collection.externalIdentifier.sub('druid:', '')}:#{catkey&.catalogRecordId}:#{Cocina::Models::Builders::TitleBuilder.build(collection.description.title)}" }


ndushay · 2023-03-13T22:24:17Z

app/services/catalog/marc_856_generator.rb

-    def previous_ckeys
-      @previous_ckeys ||= fetch_ckeys(current: false)
+    def previous_catalog_record_ids
+      @previous_catalog_record_ids ||= fetch_catalog_record_ids(current: false)
    end

    # List of current or previous ckeys for the cocina object (depending on parameter passed)
    # @param current [boolean] if you want the current or previous ckeys
    # @return [Array] previous or current catkeys for the object in an array, empty array if none exist


ndushay · 2023-03-13T22:25:14Z

spec/services/catalog/marc856_generator_spec.rb

@@ -165,7 +165,7 @@

    context "when the druid object doesn't have catkey or previous catkeys" do


look for catkey and ckey in the spec, too.

justinlittman

This PR gets us most of the way there. However, there is still some Symphony specific logic:

Determining which records have 865s to be deleted and added.
The inclusion of the catkey in subfield x collections. (Missed that before.)

That being said, I'd suggest that this is good enough (=huge improvement and step in the right direction) for this PR. I'd suggest that we make the minor tweaks recommended by Naomi, but leave all remaining catkey references as is. (Just renaming them to catalog record id doesn't solve the problem.) I'll address them in a follow-up PR that removes the last bits of Symphony-specific logic.

Also, makes sense to delay testing until the follow-up PR.

justinlittman · 2023-03-14T13:17:45Z

For the sake of expediency, I made the tweaks and squished the PR.

ndushay reviewed Mar 10, 2023

View reviewed changes

ndushay requested changes Mar 10, 2023

View reviewed changes

aaron-collier force-pushed the 4419-refactor-marc856generator-new branch 6 times, most recently from 9abd243 to 9568f93 Compare March 13, 2023 17:51

aaron-collier changed the title ~~[DRAFT | REDO] Refactor of marc856generator to serialize 856 string~~ [HOLD] Refactor of marc856generator to serialize 856 string Mar 13, 2023

aaron-collier marked this pull request as ready for review March 13, 2023 17:51

mjgiarlo reviewed Mar 13, 2023

View reviewed changes

app/services/catalog/marc_856_generator.rb Outdated Show resolved Hide resolved

ndushay requested changes Mar 13, 2023

View reviewed changes

aaron-collier force-pushed the 4419-refactor-marc856generator-new branch 3 times, most recently from 8b2dc37 to de0d041 Compare March 13, 2023 21:15

aaron-collier commented Mar 13, 2023

View reviewed changes

ndushay reviewed Mar 13, 2023

View reviewed changes

ndushay approved these changes Mar 13, 2023

View reviewed changes

justinlittman reviewed Mar 14, 2023

View reviewed changes

Simplify refactor of marc856generator

60e35ef

justinlittman force-pushed the 4419-refactor-marc856generator-new branch from 17c0935 to 60e35ef Compare March 14, 2023 13:16

justinlittman changed the title ~~[HOLD] Refactor of marc856generator to serialize 856 string~~ Refactor of marc856generator to serialize 856 string Mar 14, 2023

justinlittman merged commit 85dbd5e into main Mar 14, 2023

justinlittman deleted the 4419-refactor-marc856generator-new branch March 14, 2023 13:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor of marc856generator to serialize 856 string #4432

Refactor of marc856generator to serialize 856 string #4432

aaron-collier commented Mar 10, 2023 •

edited

ndushay left a comment

ndushay left a comment •

edited

ndushay commented Mar 10, 2023 •

edited

ndushay left a comment •

edited

ndushay Mar 13, 2023

aaron-collier Mar 13, 2023

ndushay left a comment

ndushay left a comment

ndushay Mar 13, 2023

justinlittman Mar 14, 2023

ndushay Mar 13, 2023

ndushay Mar 13, 2023

ndushay Mar 13, 2023

justinlittman Mar 14, 2023

ndushay Mar 13, 2023

ndushay Mar 13, 2023

ndushay Mar 13, 2023

ndushay Mar 13, 2023

ndushay Mar 13, 2023

ndushay Mar 13, 2023

justinlittman left a comment

justinlittman commented Mar 14, 2023

		@@ -165,7 +165,7 @@

		context "when the druid object doesn't have catkey or previous catkeys" do

Refactor of marc856generator to serialize 856 string #4432

Refactor of marc856generator to serialize 856 string #4432

Conversation

aaron-collier commented Mar 10, 2023 • edited

Why was this change made? 🤔

ndushay left a comment

Choose a reason for hiding this comment

ndushay left a comment • edited

Choose a reason for hiding this comment

ndushay commented Mar 10, 2023 • edited

ndushay left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ndushay left a comment

Choose a reason for hiding this comment

ndushay left a 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

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

justinlittman left a comment

Choose a reason for hiding this comment

justinlittman commented Mar 14, 2023

aaron-collier commented Mar 10, 2023 •

edited

ndushay left a comment •

edited

ndushay commented Mar 10, 2023 •

edited

ndushay left a comment •

edited