-
Notifications
You must be signed in to change notification settings - Fork 0
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 concept to reference dt node migration #24 #28
Add concept to reference dt node migration #24 #28
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Some very minor feedback at your option, otherwise happy to merge this as is.
) | ||
) | ||
|
||
REFERENCE_SELECT_WIDGET = Widget.objects.get(name="reference-select-widget") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wondering if we should make this more resilient against widget renames. Should we use id? Or just filter on datatype and use the hardcoded one if present else an aribtrary one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, but I think for now it's okay. Currently the reference select widget handles both single and multi-select. The forecasts for folding other core datatypes into Arches-References are far away - I say we cross this bridge when we get there.
@@ -82,6 +105,8 @@ def handle(self, *args, **options): | |||
overwrite=options["overwrite"], | |||
preferred_sort_language=psl, | |||
) | |||
elif options["operation"] == "migrate_concept_nodes_to_reference_datatype": | |||
self.migrate_concept_nodes_to_reference_datatype(options["graph"]) | |||
|
|||
def migrate_collections_to_controlled_lists( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of scope for this PR, but I just ran into a case sensitivity thing, so it might have been nicer to do a looser match, e.g. value__lower__in=collections_to_migrate
and then on the python side lower() everything in collections_to_migrate
.
management.call_command( | ||
"controlled_lists", | ||
operation="migrate_concept_nodes_to_reference_datatype", | ||
graph="b974103f-73bb-4f2a-bffb-8303227ba0da", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a follow-up ticket we could accept slugs, but not now. 👍
Resolves #24
Should be preceded by #26 ✅