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

Derek furst/add creation action #538

Merged
merged 5 commits into from
Sep 28, 2023
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion src/app.py
Original file line number Diff line number Diff line change
Expand Up @@ -4126,7 +4126,6 @@ def create_entity_details(request, normalized_entity_type, user_token, json_data
# Meaning the returned target property key is different from the original key
# in the trigger method, e.g., Donor.image_files_to_add
filtered_merged_dict = schema_manager.remove_transient_and_none_values(merged_dict, normalized_entity_type)

# Create new entity
try:
# Check if the optional `superclass` property is defined, None otherwise
Expand Down
16 changes: 14 additions & 2 deletions src/schema/provenance_schema.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -88,8 +88,6 @@ ACTIVITIES:
<<: *shared_properties
creation_action:
type: string
generated: true
immutable: true
description: "The activity that was performed."
before_create_trigger: set_activity_creation_action

Expand Down Expand Up @@ -279,6 +277,13 @@ ENTITIES:
antibodies:
type: list
description: "A list of antibodies used in the assay that created the dataset"
creation_action:
type: string
transient: true
immutable: true
description: "The activity that was performed."
before_property_create_validators:
- validate_creation_action
description:
type: string
description: "Free text description of the dataset"
Expand Down Expand Up @@ -508,6 +513,13 @@ ENTITIES:
type: string
description: "Additional information about the dataset, which can be used to find this dataset, including lab specific (non-PHI) identifiers."
# The Dataset.data_access_level is based on Dataset.status and Dataset.contains_human_genetic_sequences
creation_action:
type: string
transient: true
immutable: true
description: "The activity that was performed."
before_property_create_validators:
- validate_creation_action
data_access_level:
type: string
generated: true
Expand Down
3 changes: 2 additions & 1 deletion src/schema/schema_triggers.py
Original file line number Diff line number Diff line change
Expand Up @@ -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']:
Copy link
Member

@yuanzhou yuanzhou Sep 28, 2023

Choose a reason for hiding this comment

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

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.

return property_key, new_data_dict['creation_action']
return property_key, f"Create {new_data_dict['normalized_entity_type']} Activity"


Expand Down
21 changes: 21 additions & 0 deletions src/schema/schema_validators.py
Original file line number Diff line number Diff line change
Expand Up @@ -509,6 +509,27 @@ def validate_publication_date(property_key, normalized_entity_type, request, exi
except ValueError:
raise ValueError(f"Invalid {property_key} format, must be YYYY-MM-DD")

"""
Validate the provided value of the activity creation action. Only very specific
values are allowed.
Parameters
----------
property_key : str
The target property key
normalized_type : str
Submission
request: Flask request object
The instance of Flask request passed in from application request
existing_data_dict : dict
A dictionary that contains all existing entity properties
new_data_dict : dict
The json data in request body, already after the regular validations
"""
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"]:
Copy link
Member

@yuanzhou yuanzhou Sep 28, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.

raise ValueError(f"Invalid {property_key} value")


####################################################################################################
## Internal Functions
Expand Down