-
Notifications
You must be signed in to change notification settings - Fork 3
Enhance list blueprints #69
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,94 @@ | ||
| import re | ||
| from surge.projects import Project | ||
| from surge.errors import SurgeMissingAttributeError | ||
|
|
||
|
|
||
| class Blueprint(Project): | ||
| def __int__(self, **kwargs): | ||
| super().__init__(kwargs) | ||
|
|
||
| @classmethod | ||
| def _from_project(cls, project): | ||
| '''Converts a Project to a Blueprint. This is useful when we want to create a Project and then return | ||
| a Blueprint to the user. | ||
|
|
||
| Arguments: | ||
| project (Project): The project to convert to a Blueprint | ||
|
|
||
| Returns: | ||
| blueprint (Blueprint): A Blueprint derived from the given Project | ||
| ''' | ||
| kwargs = project.__dict__ | ||
| # A Project object will have already parsed created_at, so omit that and save current value to avoid errors. | ||
| created_at = kwargs.pop('created_at', None) | ||
| b = Blueprint(**project.__dict__) | ||
| b.created_at = created_at | ||
| return b | ||
|
|
||
| def required_data_fields(self): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This probably is outside the scope here but is this validated on the server side? I think we have fairly elaborate templating logic that could continue to evolve so this probably won't be exhaustive |
||
| ''' | ||
| Returns all keys surrounded by {{}} from the fields_template string. | ||
|
|
||
| Returns: | ||
| matches (list): all the required data fields from fields_template | ||
| e.g. ['field1', 'field2'] | ||
| ''' | ||
| if not self.fields_template: | ||
| return [] | ||
| pattern = r"{{(.*?)}}" | ||
| matches = re.findall(pattern, self.fields_template) | ||
| return matches | ||
|
|
||
| def create_new_batch(self, name): | ||
| ''' | ||
| Create a new project from this blueprint. Once created, call create_tasks on the returned object | ||
| to assign tasks. | ||
|
|
||
| Arguments: | ||
| name (str): Name of the project | ||
|
|
||
| Returns: | ||
| blueprint (Blueprint): a created project from a blueprint project which can be assigned tasks and launched. | ||
| ''' | ||
| if not name: | ||
| raise SurgeMissingAttributeError('name is required when creating a project from a template') | ||
|
|
||
| create_params = {'template_id': self.id, 'name': name} | ||
| project = Project.create(**create_params) | ||
| blueprint = Blueprint._from_project(project) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm - if you wanted to split the concept of a Blueprint project from a blueprint-derived project, could we split off the methods below this one, plus required_data_fields, into a separate class?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's a good question! I lingered over this for a while. But the real sticking point for me is the fact that list_blueprints was already returning Project objects. I was trying to paint "within the lines" as I saw them, and to my eye we already decided that Projects types were going to represent both Blueprints (or templates) and the Projects derived from them. If we decided that list_blueprints should return a non-Project Blueprint or Template object that would make sense to me, but it would be backward-incompatible which I assumed we didn't want in this PR. |
||
| return blueprint | ||
|
|
||
| def create_tasks(self, tasks_data: list, launch=False): | ||
| '''Create tasks for this Blueprint project. Ensures that task_data contains fields referenced in | ||
| fields_template. | ||
|
|
||
| Arguments: | ||
| tasks_data (list): List of dicts. Each dict is key/value pairs for populating fields_template. | ||
|
|
||
| Returns: | ||
| tasks (list): list of Tasks objects | ||
| ''' | ||
| Blueprint._validate_fields_data(self.required_data_fields(), tasks_data) | ||
| return super().create_tasks(tasks_data, launch) | ||
|
|
||
| @classmethod | ||
| def _validate_fields_data(cls, required_fields, tasks_data): | ||
| ''' | ||
| Checks the keys in task_data exist in required_fields. | ||
| NOTE: this assumes tasks_data is a flat dict (no nested keys). | ||
|
|
||
| Arguments: | ||
| required_fields (list): list of required field names | ||
| tasks_data (list) list of dicts of task_data. All keys in these dicts should exist in required_fields | ||
|
|
||
| Returns: | ||
| None. Only raises if there are required fields missing. | ||
| ''' | ||
| missing_keys = [] | ||
| for data in tasks_data: | ||
| diff = set(required_fields) - set(data) | ||
| if diff: | ||
| missing_keys.append(diff) | ||
| if missing_keys: | ||
| msg = f'task_data is missing required keys: {missing_keys}' | ||
| raise SurgeMissingAttributeError(msg) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,78 @@ | ||
| from datetime import datetime | ||
| from dateutil.tz import tzutc | ||
| import unittest | ||
|
|
||
| from surge.errors import SurgeMissingAttributeError | ||
| from surge.api_resource import APIResource | ||
| from surge.projects import Project | ||
| from surge.blueprints import Blueprint | ||
|
|
||
|
|
||
| class BlueprintTests(unittest.TestCase): | ||
| '''Use unittest in this class to assert on error messages.''' | ||
|
|
||
| def test_validate_fields_data_fail(self): | ||
| b = Blueprint(id=id, | ||
| name='name_blueprint', | ||
| created_at='2021-01-22T19:49:03.185Z') | ||
| b.fields_template = '<p><iframe src="{{video}}" width="560" height="315"></iframe><br></p>' | ||
| key = 'video' | ||
| with self.assertRaises(SurgeMissingAttributeError) as context: | ||
| Blueprint._validate_fields_data(b.required_data_fields(), [{'foo': 'task'}]) | ||
| self.assertTrue(key in str(context.exception), f'error message {context.exception} should contain {key}') | ||
|
|
||
|
|
||
| def test_validate_fields_data(): | ||
| # TODO: more test coverage | ||
| b = Blueprint(id=id, | ||
| name='name_blueprint', | ||
| created_at='2021-01-22T19:49:03.185Z') | ||
| b.fields_template = '<p><iframe src="{{video}}" width="560" height="315"></iframe><br></p>' | ||
|
|
||
| key = 'video' | ||
| Blueprint._validate_fields_data(b.required_data_fields(), [{key: 'task'}]) | ||
|
|
||
|
|
||
| def test_required_data_fields(): | ||
| '''Ensure a Blueprint object knows how to find required fields in fields_template.''' | ||
| assert_required_data_fields(None, []) | ||
| assert_required_data_fields('', []) | ||
| assert_required_data_fields('foo', []) | ||
| assert_required_data_fields('{{one}}', ['one']) | ||
| assert_required_data_fields('{{video}}{{audio}}{{spacial}}', ["video", "audio", "spacial"]) | ||
| assert_required_data_fields('<p><iframe src="{{video}}" width="560" height="315"></iframe><br></p>', ['video']) | ||
|
|
||
|
|
||
| def test_init_basic(): | ||
| project_id = "ABC1234" | ||
| name = "Hello World Blueprint" | ||
| blueprint = Blueprint(id=project_id, | ||
| name=name, | ||
| created_at='2021-01-22T19:49:03.185Z') | ||
|
|
||
| assert isinstance(blueprint, APIResource) | ||
| assert isinstance(blueprint, Project) | ||
| assert isinstance(blueprint, Blueprint) | ||
| assert blueprint.id == project_id | ||
| assert blueprint.name == name | ||
| assert blueprint.created_at == datetime(2021, | ||
| 1, | ||
| 22, | ||
| 19, | ||
| 49, | ||
| 3, | ||
| 185000, | ||
| tzinfo=tzutc()) | ||
|
|
||
|
|
||
| def assert_required_data_fields(fields_template, expected_fields): | ||
| blueprint = Blueprint(id="id1", | ||
| name="name1", | ||
| created_at='2021-01-22T19:49:03.185Z') | ||
| blueprint.fields_template = fields_template | ||
| assert_values_in(blueprint.required_data_fields(), expected_fields) | ||
|
|
||
|
|
||
| def assert_values_in(ary, expected): | ||
| for value in expected: | ||
| assert value in ary |
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.
Might be good to validate that launch/pause/cancel etc. in Project aren't called for a blueprint, either in this class or in Project (perhaps in a follow-up PR) https://github.com/CivJ/surge-python/blob/3134a80007faad6b524a28bdc2cf9885df0a1136/surge/projects.py#L200
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.
This is of course validated on the server side though so maybe it doesn't matter
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.
Hi Tim! Yea I was a little worried about this. But since a Blueprint project can either be "just the template" or a real project that was derived from a template, it seems like we'd want the full project API available in the second case. For the first case, I am hoping the server will protect us (which may not be a good assumption).