Bugfix/vov 1419 #45

Merged
merged 2 commits into from Apr 4, 2013

Projects

None yet

3 participants

@atomical

No description provided.

@cjcolvar
Avalon Media System member

Do we really need to remove the terms before setting them? I'm afraid of removing physical_description and only setting [physical_description, internet_media_type]. It seems like we could accidentally remove more than we are setting.

@cjcolvar
Avalon Media System member

It seems like we could avoid this mapping if we make MasterFile and MediaObject use the same vocabulary. And do we need to store lowercase in the MODS or can we use uppercase to avoid having to titleize in the to_solr?

@atomical

@cjcolvar I've made some changes and created a pull request based on your comments. Could you comment on the pull request and I'll update the pull request if necessary. In the case of removing terms, it's the only way to update them.

@cjcolvar
Avalon Media System member

I think this is great work identifying where these values were hard-coded. It seems to point toward a need for a more robust controlled vocabulary solution instead of ruby classes but this is a good first step. I would hope that we could change the constant values to "sound recording" and "moving image" to match what is needed for the MODS document and then we wouldn't need to do the mapping.

All that being said and despite my pushing in this direction, I think this changeset might be a little too risky and unnecessary right now. I think it would be great to merge this post-R1. I can work on making a pull request that has just what we need for this bug and tests for it.

@atomical

Updated.

@cjcolvar
Avalon Media System member

+1 This is pretty much what I was thinking. It would be nice to have tests but I guess they can come later. When playing around in rails console I didn't need to do the ensure_root_node_exists! or remove before update_values, but it shouldn't hurt to have them for now. Something that testing should be able to help solve post-R1. Thanks for working through this one atomical!

@atomical

@mbklein Can we merge this?

@mbklein mbklein merged commit f51d9f0 into release/1.0.0 Apr 4, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment