Timx 232 springshare ids

[ghukill]

What does this PR do?

This PR adds two new OPTIONAL methods to the base transform:

• get_source_link()
• get_timdex_record_id()

For transformations that require it, this allows for decoupling the logic of constructing the timdex record id from the source link. This directly supports these new sources, and it's believed will also be helpful for future sources where this tight coupling may not be ideal.

Dependencies are also updated, with slight type check updates for new flake8 requirements in ead transformation.

Helpful background context

While analyzing the transformed records from new libguides and researchdatabases sources, it was found that some timdex record ids were poorly formed, e.g. libguides:c.php?g=175847.

The original approach was splitting the dc:identifier and using that for the timdex record id and concatenating with the source base url. However, in a small subset, this resulted in poorly formed ids like the one above.

Given the terse nature of these source records, there is not a meaningful identifier in the record that could serve both the timdex record id and the source link.

How can a reviewer manually see the effects of these changes?

For the following fixture (showing subset of fields here):

<?xml version="1.0" encoding="UTF-8"?>
<records>
  <record>
    <header>
      <identifier>oai:libguides.com:guides/175846</identifier>  <!-- OAI identifier -->
      ...
    </header>
    <metadata>
      <oai_dc:dc>
        ...
        <dc:identifier>https://libguides.mit.edu/materials</dc:identifier>  <!-- DC identifier -->
        ...
      </oai_dc:dc>
    </metadata>
  </record>
</records>

Run a libguides transformation:

pipenv run transform -s libguides -i "tests/fixtures/oai_dc/springshare/libguides/libguides_record_all_fields.xml" -o "output/libguides.json"

Note the following fields are set:

timdex_record_id="libguides:guides-175846"
source_link="https://libguides.mit.edu/materials",

Demonstrating the source link, due to the overriden method, is decoupled from the timdex id.

What are the relevant tickets?

https://mitlibraries.atlassian.net/browse/TIMX-232

Developer

• All new ENV is documented in README
• Stakeholder approval has been confirmed (or is not needed)

Code Reviewer

• The commit message is clear and follows our guidelines
  (not just this pull request message)
• There are appropriate tests covering any new functionality
• The documentation has been updated or is unnecessary
• The changes have been verified
• New dependencies are appropriate or there were no changes

Includes new or updated dependencies?

YES

[ehanson8]

Two comments

[ehanson8]

Do we need xml as a parameter here if it's not being used? Couldn't the method just be overridden in the source transforms with an xml parameter when it's needed?

[ghukill]

Nice catch, and I had also paused on this. I think if these are getting decoupled with the understanding transformers may override them, it's plausible they might need the document XML, so I included here.

Because the base transform calls this method, it will always use the same signature, so a sub-class transformer couldn't add xml:Tag on their own.

We could adopt a kwargs approach, where the sub-class transformer could pass anything they want as named arguments, but we'd lose the typing this provides. Other than xml:Tag, it's hard to guess what they may want to pass?

See pros and cons both ways. My thinking was to model these after other methods that are designed to overridden, like get_valid_title() or get_source_record_id() which both pass xml:Tag.

[ghukill]

But if helpful, we could add the docstring comment:

xml: A BeautifulSoup Tag representing a single XML record.
    - not used by default implementation, but could be useful for subclass
      overrides

which get_timdex_record_id() contains below. I had actually intended to do so.

[ehanson8]

Let's just add the comment, I did some more reading and see your point on losing the type hinting

[ehanson8]

Optional: you could call these directly in the dict

[ghukill]

Happy to change if you'd like, and I considered that too. But at the risk of a being a bit more verbose, I thought it helped break it up a bit. There is some important stuff happening here -- establishing record title, primary timdex id, link, etc. -- and I found it helpful to see those steps as distinct statements vs embedded in a dictionary.

[ehanson8]

Optional is always just a suggestion 🙂