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

Relationship interfaces #88

Merged
merged 9 commits into from
Aug 15, 2023
Merged

Relationship interfaces #88

merged 9 commits into from
Aug 15, 2023

Conversation

rmarow
Copy link
Contributor

@rmarow rmarow commented Aug 14, 2023

Resolves #76

@rmarow rmarow marked this pull request as ready for review August 14, 2023 19:50
Copy link
Contributor Author

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.

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.

Copy link

@MattF-NSIDC MattF-NSIDC left a 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 :)

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.

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?

Copy link

@MattF-NSIDC MattF-NSIDC Aug 15, 2023

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

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)

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?

Copy link
Contributor Author

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,
)

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! :)

usaon_vta_survey/templates/macros/forms/misc.j2 Outdated Show resolved Hide resolved
@@ -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',

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.

@rmarow rmarow merged commit 3ae767e into main Aug 15, 2023
2 checks passed
@rmarow rmarow deleted the relationship_interfaces branch August 15, 2023 17:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ensure all relationship creation interfaces work
2 participants