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

Adding new MODS fields as per Metadata Interest Group mappings #5

Merged
merged 17 commits into from
Jan 15, 2020

Conversation

Natkeeran
Copy link
Contributor

@Natkeeran Natkeeran commented Aug 2, 2019

GitHub Issue: (link)

What does this Pull Request do?

This PR adds various fields needed for a default MODS form as per the above noted ticket.

How should this be tested?

  • SSH into the vagrant
  • Go to Features: http://localhost:8000/admin/config/development/features
  • Select Islandora bundle, click newly detected, import changes
  • Add a vocabulary list item if needed in the taxonomy
  • Create a new repository item
  • Verify that new fields appear in the form and display

Interested parties

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

@Natkeeran Natkeeran changed the title Mods fields Adding new MODS fields as per Metadata Interest Group mappings Aug 2, 2019
@MarcusBarnes
Copy link

I've just tested and this appears to work as expected.

@whikloj
Copy link
Member

whikloj commented Aug 14, 2019

This is failing travis and I'm guessing due to the dependency on the various taxonomy configs which don't exist here. Did you mean to add them @Natkeeran

@seth-shaw-unlv
Copy link
Contributor

My guess is that the updates to controlled access terms needs to merge first.

@whikloj
Copy link
Member

whikloj commented Aug 14, 2019

Relies on Islandora/controlled_access_terms#35

@rosiel
Copy link
Member

rosiel commented Nov 7, 2019

@Natkeeran the merge conflicts are, as far as i can tell, due to the addition of the weight field at the spots where your changes began. They're not too hard to resolve.

Copy link
Member

@rosiel rosiel left a comment

Choose a reason for hiding this comment

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

Sorry for the novel! I did a lot of the same thing:

  • most single-valued fields should be multi-valued
  • some fields need to be translated
  • would it be ok to mark some of these as "auto-create" i.e. you don't have to add the term to the taxonomy before entering it here? I think this will increase ease of use in general and people can lock them down if they know what they're doing. We can discuss the general principle and the fields that aren't included in this PR as a later date, and if you want to push all "auto-create" suggestions to that time, that's fine with me.

The other question is whether we will be able to pre-populate some vocabularies but i should probably take that to the Controlled Access Terms PR.

Thanks for your patience, this is a lot of great work. Thank you for reading the messy MIG documentation.

@Natkeeran
Copy link
Contributor Author

@rosiel thank you for the feedback.

Just getting to this one. I will make the requested changes.

@Natkeeran
Copy link
Contributor Author

@rosiel

I've made all the requested changes.

The vocabs have been made translatable, thus whether field is transtlable or not does not seem matter.

@Natkeeran
Copy link
Contributor Author

Hoping this gets reviewed before the next release!

@seth-shaw-unlv
Copy link
Contributor

@Natkeeran, we need updates to Islandora/controlled_access_terms#35 approved and merged before we can do this one.

@seth-shaw-unlv
Copy link
Contributor

seth-shaw-unlv commented Jan 10, 2020

I know there is a big thread of discussion about what needs to be adjusted on that ^^ PR. Let me see if I can go through and summarize them.

@Natkeeran
Copy link
Contributor Author

@seth-shaw-unlv oh, right. Thank you.

@Natkeeran
Copy link
Contributor Author

@dannylamb @rosiel

I've made the changes to controlled_access_terms as per Seth's suggestions. Mainly, moving the country vocab and fields into a sub module.

The only thing that I am considering adding would be add a hook to create an rdf entry for place_country field when the module gets installed. However, that can be a separate PR.

@MarcusBarnes
Copy link

I've just tested this PR and it appears to be functioning as expected.

Copy link
Contributor

@seth-shaw-unlv seth-shaw-unlv left a comment

Choose a reason for hiding this comment

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

As per the Slack discussion, we should use text_long instead of string_long for 'Note' and 'Table of Contents'.

Copy link
Contributor

@seth-shaw-unlv seth-shaw-unlv left a comment

Choose a reason for hiding this comment

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

:shipit:

@seth-shaw-unlv seth-shaw-unlv merged commit a2034cf into Islandora:8.x-1.x Jan 15, 2020
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