Derek furst/add creation action #538
Conversation
Derek furst/track status
'hubmap process' action to 'central process'. Also added multi-assay split
…. It already couldn't be changed because there's no update triggers on the activity, but users could still attempt to update and would not know why it failed. So now an error will be raised.
src/schema/schema_triggers.py
Outdated
| @@ -1907,7 +1907,8 @@ def get_upload_datasets(property_key, normalized_type, user_token, existing_data | |||
| def set_activity_creation_action(property_key, normalized_type, user_token, existing_data_dict, new_data_dict): | |||
| if 'normalized_entity_type' not in new_data_dict: | |||
| raise KeyError("Missing 'normalized_entity_type' key in 'existing_data_dict' during calling 'set_activity_creation_action()' trigger method.") | |||
|
|
|||
| if new_data_dict and new_data_dict['creation_action']: | |||
There was a problem hiding this comment.
This condition will cause KeyError: 'creation_action' for when creation_action field is not provided in the request, which will result 500 error. Need to check the existence of the key and no need to keep the non-empty check since the validator should have handled that.
src/schema/schema_validators.py
Outdated
| """ | ||
| def validate_creation_action(property_key, normalized_entity_type, request, existing_data_dict, new_data_dict): | ||
| creation_action = new_data_dict[property_key] | ||
| if creation_action and creation_action not in ["Central Process", "Lab Process", "Multi-Assay Split"]: |
There was a problem hiding this comment.
The problem is when the provided creation_action value is an empty string, it'll satisfy this validator with no exception raised, which will further cause the corresponding trigger method set_activity_creation_action() to use the default value instead. So non-empty check is required here.
I would also make this string match case-insensitive so it's more fault-tolerant, just like the way we check status values. It's also a good idea to tell the API consumers what are the accepted values in the ValueError() next line. Because these values are not as easy-to-get-right-for-the-first-time as those simple status values.
There was a problem hiding this comment.
@yuanzhou I changed new_data_dict['creatoin_action'] to `new_data_dict.get('creation_action') which on its own should fix the error being raised. While I think it might be acceptable to just use the default creation action when the field is empty, I still went ahead and added validation to make sure its not empty anyway. I made the validation case insensitive and added some case normalization within the trigger so it will always display with the first letters capitalized (e.g. "Lab Process"). I modified the value error message to include the accepted values for creation_action as well.
added additional validation for empty values. Made it more clear in error messages what the acceptable values are for creation_action. Made creation action validator case insensitive. Also normalized the case for creation action within the trigger. Now always has the first letter capitalized.
Added property creation_action to datasets and publications. This property is transient and immutable, so it won't be saved in the
database within the dataset/publication node itself, however the trigger method for the activity node with this property has been updated to grab this value if its in the json. If this field isn't present, it defaults to the existing value. Validator verifies that the property is only 'Central Process', 'Lab Process', or 'Multi-assay Split'.