Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Incorporate MIG sandbox edits. #52

Merged
merged 6 commits into from
Jul 15, 2021
Merged

Conversation

rosiel
Copy link
Member

@rosiel rosiel commented May 20, 2021

The MIG held a sprint to audit the metadata profile. They made changes on a sandbox that were tracked in this document:
https://docs.google.com/document/d/18XChGOCWm_bT_S-UsfOl8l9xnsioF-L8sdsBXMQcqXU/edit#heading=h.9bn0ps9gm8x9

Diff between the altered sandbox and a clean sandbox was analyzed line-by-line, and relevant changes to Defaults were pulled in here. Tracking document: https://docs.google.com/spreadsheets/d/15speAh8yQ0-Cuww08efOUGjqw2v6KBXD2sUji5X47zQ/edit#gid=0

What does this Pull Request do?

Pulls in changes that applied to Islandora Defaults.

What's new?

Lots of changes to field labels and descriptions.
A few new fields
A few new taxonomy vocabularies:

  • country
  • form
  • frequency
  • issuance
  • temporal subject

Edits to the genre and language vocabularies will be in another PR to controlled access terms.

How should this be tested?

Does it install as cleanly as Defaults?

Additional Notes:

This process sucks.

Interested parties

Tag (@ mention) interested parties or, if unsure, @Islandora/8-x-committers

@rosiel
Copy link
Member Author

rosiel commented May 20, 2021

Putting field_place_published_country in its own module is maybe not the greatest idea.

@rosiel
Copy link
Member Author

rosiel commented May 20, 2021

good to know: it's a bad idea to machine-name a vocabulary "form" because it seems to break the yaml.

@rosiel
Copy link
Member Author

rosiel commented May 21, 2021

As you can see, the previous build worked, but we needed to move a field storage definition into controlled_access_terms for its pull request to be possible. That's why this will fail for the moment.

@kayakr
Copy link

kayakr commented May 25, 2021

There are risks in this PR when it comes to updating existing sites to this configuration, that may require further documentation:

  • field_alternative_title, if this field is removed, there is potential data loss; should there be a process to transfer data to the new field field_alt_title?
  • changes some RDF namespaces from dcterms to dc11; what is process for data already in fcrepo and Blazegraph
  • reference changes to target new vocabulary temporal_subjects and presumably existing taxonomy temporal is deprecated, to be removed.

@rosiel
Copy link
Member Author

rosiel commented May 25, 2021

Thanks @kayakr. Those are really important issues.

I don't know if it's possible to update any content type (through features) when there's content in it. Like as soon as you touch field storage settings you're basically forcing a new site. I don't know if we've ever said that you should be able to update islandora defaults, or just have to look at the new setup and tweak yours to match. (ps. i hate it too)

WRT alt title and subject (temporal), I think those might have been already committed to code, but for some reason the sandbox that we started with didn't have them (or didn't have them configured properly) so we re-added them? I'm following up to see why.

For RDF changes, wouldn't a "full reindex" update existing data?

@rosiel
Copy link
Member Author

rosiel commented May 25, 2021

I can confirm that the entire reason for the alternative title -> alt title thing in this PR is to change the cardinality from 1 to -1 (unlimited).

I could refactor this PR to change the cardinality without creating a new field, but I don't know how that would go over on existing sites either. Or, again, if that is something we are concerned with.

@kayakr
Copy link

kayakr commented May 26, 2021

@rosiel Thanks for the response. Re "I don't know if we've ever said that you should be able to update islandora defaults", perhaps not, but if Islandora defaults changes and sites stay stuck on older versions and schemas IMO it will become increasingly challenging to incorporate functionality and features from the main islandora project. If the Islandora project provides no upgrade path for features in its purview then it imposes a burden on implementors that could mean that some sites fall behind and can't benefit from other community advancements.

If there is a recommended approach to building on or with Islandora defaults, it would be good to have that documented clearly. If it is already documented, perhaps I missed that.

Yes, a full reindex is the brute force approach to changing namespace values but for large collections it would be good to have tools providing a more precise data migration (probably out of scope for this PR but I expect the need will grow over time).

Re changing the cardinality, it should be possible to achieve without needing a new field. https://www.drupal.org/node/2554097 describes a node_update hook for "changing cardinality of a field having existing data from single to multiple" that sounds like what is needed. We would need to adapt it to this usecase.

@dannylamb
Copy link
Contributor

@kayakr We're trying to make some sort of delineation between what Islandora (a.k.a. all of us) are responsible for maintaining and what each individual site is maintaining. We've got the islandora_core_feature, which if there are changes we definitely need update hooks. This is field_model and field_media_use type stuff that many other features are built around. islandora_defaults is much less so, and in general features aren't based off of it. But yeah, it's fuzzy and Drupal config is so intertangled it can be a bit of a bear to figure out what we're really responsible for.

That said, all those thoughts came at a time when it was mostly just programmers involved. This PR is an attempt to get contributions out of our Metadata Interest Group, and I guess is really highlighting the importance of the metadata best practices they are suggesting. If we think that's worthy enough for update hooks, we can go there and make sure they are included in the release. I'm not sure how we can do it without stomping on existing sites that have made significant changes, so there'd need to be some research / deep dives involved. Also, it need not be done by @rosiel, she actually took this task off of my plate as a favor and I don't want to burden her with a ton more work.

Interested to hear what other @Islandora/8-x-committers think too. It's work. Do we think it's worthwhile?

@dara2
Copy link

dara2 commented May 26, 2021

Wow, this is a lot of awesome work - and so good to have the input of metadata experts!
Thank you @kayakr for raising the questions about incorporating these changes into existing sites, I had the same questions. I also wondered a couple of things after reviewing the spreadsheet, from the perspective of someone helping clients migrate data into i8 sites:

  • I see a note about "remove code from display" for the linked agent relators. Could someone explain more about what this will do? I find it helpful to see that list by going to Content Types > Repository Item > Manage Fields > Linked Agent when migrating from Workbench (to see what is there by default and what its abbreviation is, to add to my CSV) and I add custom relators there as well. Will all that still be possible?
  • In general, I'm wondering about how Workbench migration will be affected by some of these changes. For example where there are fields like Subject that are currently linked to certain taxonomies by default, and Workbench looks for those as namespaces when migrating - if that changes, does anything need updating on the Workbench side? Or maybe it just "knows" which vocabs are associated with the field.
  • I saw a note about changing the form groups for several system-related fields (resource type, model, etc.) as well as standard Drupal fields like status, promote, created, etc. that are typically in tabs on the side of the form. I wondered how these groupings are being changed - is there somewhere I can see that?
    Thank you!

@seth-shaw-unlv
Copy link
Contributor

We've gone back and forth on this topic a number of times in Tech Calls over the past few years (and just now in response to this ticket). In the past year (or more?) we've generally settled on "once you install islandora_defaults, it's yours" as @dannylamb described. We tried to use Features to support selectively incorporating updates, but the configs are so intertwined that it doesn't always work well and, of course, some things can't be changed without an update hook. Adding update hooks will cause Drupal to constantly nag sites that don't want specific changes to run them.

I think we need to get to the point where people think of islandora_defaults is search_api_solr_defaults.... you install it once when you create your site to get your initial config, but then you disable it and the config stays behind. It is then yours. Defaults would represent the community's best guess as to what a new site wants but sites need to make changes on their own (either via the UI, config sync, or their own update hooks). The community can try to help through documentation ("Here is how to get the cool new thing!") and conversations on Slack or email.

I took a quick glance at islandora_defaults and the main blocker from getting to that point, where you can uninstall islandora_defaults, is

  1. pushing the relators namespace into the new JSON-LD namespace configuration (so, an install hook instead of an alter hook like we recently did for the islandora module),
  2. moving the mirador code into a stand-alone module like the openseadragon one, and
  3. ensuring the existing configs don't have islandora_defaults listed in the required dependencies section.

TL;DR: core islandora module config changes need update hooks to keep existing sites working while islandora_defaults config changes should be left alone to avoid forcing changes on sites that don't want them.

@rosiel
Copy link
Member Author

rosiel commented Jul 13, 2021

😭 I can't find where to restart tests now that we've changed to using Github CI!
Anyway, we merged the changes to Controlled Access Terms Defaults. I think it should resolve the issues with tests... I hope?

@dannylamb
Copy link
Contributor

dannylamb commented Jul 15, 2021

I tested this with ISLE quite easily. For posterity, here's what I did

  • git clone https://github.com/islandora-devops/isle-dc.git if you don't already have it
  • cd isle-dc
  • sudo rm -rf codebase if it already exists
  • git clone https://github.com/islandora-devops/islandora-sandbox.git codebase
  • rm codebase/composer.lock to guarantee I'm getting the latest code
  • make local, which will install Islandora, but only enable the islandora module and nothing else
  • Pulled in the changes for this PR with
    • cd codebase/web/modules/contrib/islandora_defaults
    • git checkout -b rosiel-mig-edits 8.x-1.x
    • git pull https://github.com/rosiel/islandora_defaults.git mig-edits
    • cd /path/to/isle-dc to get back to the root folder
  • docker-compose exec drupal with-contenv bash -lc 'drush en islandora_defaults' to enable islandora_defaults
  • make hydrate to configure the newly enabled modules and run the new migrations

This resulted in a new site with no content, the latest code and configuration from this PR.

@dannylamb
Copy link
Contributor

This all looks good! No overlapping / duplicate config between here and the controlled_access_terms updates. The new form looks great!

image

Also double checked all the taxonomy term forms, etc... Nothing seems out of place. I'm merging this one in 🚀

Copy link
Contributor

@dannylamb dannylamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this process wasn't as smooth as we all wanted, so I just want to thank @rosiel and @kspurgin and all the rest of the MIG who contributed to this PR. It was a team effort, and I appreciate your work, and just as importantly, your patience. 🙇‍♂️

@dannylamb dannylamb merged commit b117998 into Islandora:8.x-1.x Jul 15, 2021
@rosiel rosiel deleted the mig-edits branch August 27, 2021 15:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants