-
Notifications
You must be signed in to change notification settings - Fork 3
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
Relationship interfaces #88
Conversation
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 file VERY closely resembles usaon_vta_survey/routes/response/relationships/data_product_application.py
so the thought was potentially moving a lot of the re-used functions elsewhere to a util file. I wasn't sure if it would be best to do that now or wait until i start on the SBA relationship stuff.
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 think if any functions are already just plain duplicates, let's go ahead and extract them. But I think a lot of these functions are just a bit different from each other because they handle relationships between ObsSys and DataProduct, as opposed to DataProduct and App. If you see a great way to DRY that now, I say go for it, that could save some time later. If you don't, let's just keep thinking about it and plan to work on DRY when we get to the next relationship. There's a lot of code here, so I think it'll be really important to do another pass.
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.
Nice work! Many of my comments are thinking out loud about what we may want to do in the future, but some may make sense to you to implement now. Whatever feels right :)
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 think if any functions are already just plain duplicates, let's go ahead and extract them. But I think a lot of these functions are just a bit different from each other because they handle relationships between ObsSys and DataProduct, as opposed to DataProduct and App. If you see a great way to DRY that now, I say go for it, that could save some time later. If you don't, let's just keep thinking about it and plan to work on DRY when we get to the next relationship. There's a lot of code here, so I think it'll be really important to do another pass.
) -> ResponseDataProduct: | ||
"""Return a data product db object (or 404), and do some mutations. | ||
|
||
TODO: Extract mutations to another function responsible for that. |
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.
Is the "do some mutations" part of this docstring still correct? It seems to either get or initialize a data product object, so maybe a clearer name would be _get_or_init_dataproduct
?
That said, the interface here is a bit tricky. Only one or the other of the arguments is used, never both at the same time. And which one is used changes the outcome; if data_product
is used, we get a live database object. If response_id
is used and data_product
is None
, we get a transient object that will need to be added to the db. I don't have a great suggestion to improve that. Maybe this logic should just be exposed at the top level instead of being in a function. Maybe when this function creates a new object instead of getting it from the db, it should add and commit that object so the function always returns a live DB object.
What do you think?
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.
- Please remove "do some mutations" and related OBE TODOs from docstrings. I think they were copied from the other relationship, so let's get them there too.
observing_system_id: int | None, | ||
response_id: int, | ||
) -> ResponseObservingSystem: | ||
"""Return an observing system db object (or 404), and do some mutations.""" |
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.
Same as the function above. Maybe we could genericize things a bit since so many things depend on what arguments are coming in from the route.
Maybe something like this pseudocode:
# TODO: Use TypeVar to ensure type of `model` is same as return type; replace `Any`s
def _get_or_404_or_init(model: Type[Model], identifiers: list[Any], init_params: dict[str, Any]) -> Model:
"""Query for record(s) matching `identifiers`, if provided.
Otherwise, insert a new record with `init_params`. This pattern is useful for routes which deal with database relationships.
E.g. a route which receives `response_id` (required) and `observing_system_id` (optional) would want to lookup or create the observing system based on whether it's passed. If the observing system needs to be created, we want to link it to the `response_id` that's required by the route.
"""
if identifiers:
record = db.get_or_404(model, tuple(identifiers))
else:
record = model(*init_params)
# TODO: Should we actually add the record to the DB at this point?
# EDIT: I think no; at the point we're calling these functions we don't know whether the request is a POST or not, so we don't know whether the DB should be modified. I don't like that this means that the function would return either a real DB object or a transient object, but we can keep thinking about how to do better :)
# db.add(record)
# db.commit()
return record
Maybe after we replace multiple functions with this pattern, another repeated pattern will jump out that we can leverage. What do you think of this?
Again maybe this idea is more for the next PR than this one :)
|
||
if request.method == 'POST': | ||
limit_response_editors() | ||
form = SuperForm(request.form, obj=form_obj) |
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.
Was this "super form" stuff intuitive enough?
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.
yes!
response_observing_system_data_product, | ||
f'response_{key}_id', | ||
obj.id, | ||
) |
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'd love to extract this whole block to a re-usable function if we can! Maybe update_response_object_relationship_from_super_form(model: Type[Model], form: Form) -> None
? 😅
Maybe for the next PR as well, just sharing thoughts! :)
@@ -27,7 +27,13 @@ <h3>Data products</h3> | |||
<br /> | |||
{% endfor %} | |||
|
|||
<a href="#">Add relationship</a> | |||
<a href="{{url_for( | |||
'view_response_observing_system_data_product_relationships', |
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.
Thinking about our conversation with Hazel, maybe the route becomes view_response_object_relationships
, and it looks at the arguments it receives to understand what type of relationship it's dealing with. E.g. if it gets survey_id
and data_product_id
, it can know that is a valid type of relationship and handle it. But if it gets data_product_id
and sba_id
, it knows that's not valid.
Resolves #76