-
Notifications
You must be signed in to change notification settings - Fork 36
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 a realtionship to CO to be able to add parent COG #5433
base: production
Are you sure you want to change the base?
Conversation
Will #5437 be fixed in this PR? |
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.
Issue is in #5437, as CO/COG children do not display the parent in the parentCog
or coParentCog
QCBX if not set using that QCBX. This is not unique to this implementation as it already exists with parentCog
, so let me know if this is out of scope.
This functions as you'd expect and makes the QCBX work properly otherwise! COs can be added to COGs, but a corresponding COJO is created automatically.
If you create a new Consolidated COG, typically the first item entered would be "Primary". When using this approach, it is not set as primary automatically, enabling users to create a Consolidated COG without any primary COJO.
See I can't unset a subsequent addition to the COG:
https://github.com/user-attachments/assets/e1b437e5-0602-4fad-b90c-7ba184b1cf9f
Deleting a COJO created after setting this FK also doesn't remove the link, so it still preserves the COG FK in the QCBX while no corresponding COJO record exists:
Screen.Recording.2024-12-04.at.10.39.35.AM.mov
@@ -1466,6 +1466,7 @@ class Collectionobject(models.Model): | |||
paleocontext = models.ForeignKey('PaleoContext', db_column='PaleoContextID', related_name='collectionobjects', null=True, on_delete=protect_with_blockers) | |||
visibilitysetby = models.ForeignKey('SpecifyUser', db_column='VisibilitySetByID', related_name='+', null=True, on_delete=protect_with_blockers) | |||
collectionobjecttype = models.ForeignKey('CollectionObjectType', db_column='CollectionObjectTypeID', related_name='collectionobjects', null=True, on_delete=models.SET_NULL) | |||
coparentcog = models.ForeignKey('CollectionObjectGroup', db_column='CoParentCogID', related_name='+', null=True, on_delete=protect_with_blockers) |
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.
(optional) Would it make more sense to name the field parentCog
so the name is consistent across both COG and CO tables?
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.
I was thinking the same
@@ -8298,6 +8299,7 @@ | |||
Relationship(name='collection', type='many-to-one', required=False, relatedModelName='Collection', column='CollectionID'), | |||
Relationship(name='cogType', type='many-to-one', required=True, relatedModelName='CollectionObjectGroupType', column='COGTypeID'), | |||
Relationship(name='parentCog', type='many-to-one', required=False, relatedModelName='CollectionObjectGroup', column='ParentCogID'), | |||
Relationship(name='coParentCog', type='many-to-one', required=False, relatedModelName='CollectionObject', column='CoParentCogID'), |
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 like coParentCog
got added to both CO and COGs. I don't think you need to add the field to COG here as you already added it to CO and the field doesn't actually belong to COG
@orm_signal_handler('post_save', 'Collectionobject') | ||
def cog_post_save(co): |
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.
Can you rename this function as we already have a cog_post_save
function in this file? It shouldn't make any difference functionally but it would be less confusing
parentCog: async (cog): Promise<BusinessRuleResult> => { | ||
if (cog.url() === cog.get('parentCog')) { | ||
return { | ||
isValid: false, | ||
reason: resourcesText.parentCogSameAsChild(), | ||
saveBlockerKey: PARENTCOG_KEY, | ||
}; | ||
} | ||
|
||
return { | ||
isValid: true, | ||
saveBlockerKey: PARENTCOG_KEY, | ||
}; | ||
}, |
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.
Do we need this business rule at all? The parentCog
business rule in COG helps avoid linking a COG to itself - but that will not be a problem in a CO and so this shouldn't be needed
readonly toOneIndependent: { | ||
readonly coParentCog: CollectionObject | null; |
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.
COG -> coParentCog is not needed (see other comment)
Fixes #5428
Checklist
and self-explanatory (or properly documented)
Testing instructions