fix parent cp visibility error #2989
Conversation
antgonza
changed the title
qiita_db/artifact.py
Outdated
| # set based on the "lowest" visibility | ||
| if 'sandbox' in visibilities: | ||
| instance.visibility = 'sandbox' | ||
| instance._set_visibility('sandbox') |
There was a problem hiding this comment.
is this change necessary? it looks like instance.visibility is a property where the setter calls _set_visibility
There was a problem hiding this comment.
Some background: when an artifact is created, it will "copy" the parents visibility; however, right now the visibility "regular" setter (visibility) has a validation step that prevents this from happening. Thus, the "solution" is that for creation we use the new setter (_set_visibility) without validation and when we do whatever else we use the regular one, which has the validation ... AKA special case for artifact creation
There was a problem hiding this comment.
Changes look good. However if I am understanding correctly, _set_visibility is a privileged and private method that bypasses validation for artifacts that inherit a "public" status but are no longer valid (such is life). The only place where this new method is called is during creation, are we sure that all artifacts can be safely created without any additional validation? My concern is that all artifact creations are no longer going to be validated. Maybe this should only happen when the inherited visibility is "public"?
The alternative would be to require users to make their metadata compliant before they can create new artifacts.
|
Good point, I'll change so we only use |
|
Thanks @antgonza! |
3 participants
The issue comes from studies/artifacts that became public before we had more stringent metadata requirements and when a new artifact (new reprocessing) is added the new artifact will not comply with them and the artifact will not be created. The easiest solution is to simply skip the validation step for when a new artifact is created.
All tests passed. So ready for review.