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

add contexts to the flexible metadata model. #6915

Open
wants to merge 6 commits into
base: flexible_double_combo
Choose a base branch
from

Conversation

orangewolf
Copy link
Member

they can be assigned on admin sets and then apply the contexts to the works made with that admin set selected

…admin sets and then apply the contexts to the works made with that admin set selected

# OVERRIDE disposable 0.6.3 to make schema dynamic
def schema
if Hyrax.config.flexible?
Copy link
Contributor

@laritakr laritakr Oct 8, 2024

Choose a reason for hiding this comment

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

I assume the inclusion of this flexible? check is unnecessary, given that including this module is also conditioned by the flexible check. I'm curious if there is another reason for including it?

Or it's just there because it was directly moved over from the resource_form?

Copy link
Member Author

Choose a reason for hiding this comment

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

correct, I'll take them out

@@ -0,0 +1,5 @@
class AddContextsToHyraxFlexibleSchemas < ActiveRecord::Migration<%= migration_version %>
def change
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this migration have a conditional check if it already exists?

Copy link
Member Author

Choose a reason for hiding this comment

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

yep

@@ -18,6 +18,8 @@ classes:
contexts:
flexible_context:
display_label: Flexible Metadata Example
special_context:
Copy link
Contributor

Choose a reason for hiding this comment

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

Corresponding changes to this profile should probably also be made in the .koppie (and possibly .dassie?) versions, as well as the spec version.

Copy link
Contributor

@laritakr laritakr left a comment

Choose a reason for hiding this comment

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

A few comments in addition to the above...

  • the M3 profiles in specs, .koppie, and .dassie do not match the changes in the config profile. These need to be updated
  • no specs exist for the code changes here
  • the migration needs to also be added to koppie & dassie
  • Admin set show page should display the selected metadata context if there is one
  • I was unable to save a work with the dimensions term

This is the error I got trying to save the work using the context term. I'm not certain of the cause (my app crashed badly and I had to rebuild everything), but it originates on this line and the actual failure is when it calls definitions in the m3_schema_loader.

Screenshot 2024-10-08 at 5 40 09 PM

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.

2 participants