Deployment handling interface

[michaelwheeler]

Looking for feedback on this approach to addressing #26. A few items to call out...

1. When a launch occurs with a deployment ID that has not been seen previously by the tool, we are now creating a new, inactive LtiDeployment.
2. To account for the previous change we are handling launches with inactive deployments using the new handle_inactive_deployment method of LtiLaunchBaseView. By default a slightly helpful message is returned (better than the previous behavior of raising an exception, I think). This also allows for more sophisticated behaviors, like redirecting to a page where the user could submit a deployment activation request or associate it with a paid license.
3. A tool can modify a deployments status prior to handling a launch by implementing the new launch_setup method. Suggestions for a more appropriate name are welcome. This allows workflows like automatically activating all deployments for a specific registration (or for every registration).

[cmurtaugh]

This looks like a good, flexible approach that will work well for our use case! 👍

[doreilly]

Thanks for getting this work started Michael.

I've been trying to wrap my head around where best to handle the decision to create a LtiDeployment. My initial reaction is that creating a resource in a find_by_… method, feels a bit unexpected. Upon further review, it does seem like we're limited by the control flow of pylti1p3, so this is a logical enough spot. If I'm reading this right, there isn't a possibility of these resources being created without reasonable assurance that it's a valid situation, given that it's happening in a validated OIDC response (with a validated Registration beforehand).

I appreciate the extension of the base view to have an extension point to override the response for inactive deployments on a per-project basis.

Is there ever a case where we wouldn't want to create an LtiDeployment? In other words, what argument would there be against making what you have drafted here the default?

Assuming the answer to that question is "Yes", we'd have to consider whether it's the right solution is to always create a record, or to make it switchable. I'm thinking of a flag on DjangoToolConfig that is switchable from somewhere—add a better worded option where create_deployment_if_not_present as an optional argument to the initializer, and then using that to trigger creation or not. That might have to be configured from a Django setting, given that the message launch fetch is also managed in middleware, making project-level customization less convenient. I'm not convinced the increase in complexity is a better choice than just creating an inactive LtiDeployment in every case as you have here and coping with that after the fact.

[michaelwheeler]

Thanks for taking a look @doreilly!

Upon further review, it does seem like we're limited by the control flow of pylti1p3, so this is a logical enough spot.

I share your hesitation, but landed in the same spot. I think deployment creation in the find_by methods is the practical path forward at the moment.

Is there ever a case where we wouldn't want to create an LtiDeployment? In other words, what argument would there be against making what you have drafted here the default?

I considered these questions while working on this, and didn't come up with much. Since the platform mints and manages deployments we don't have much of a basis not to record a deployment that is a part of a valid launch from a registered platform. Where my head is at now, I think creating a record of a deployment the first time it is seen (as we do with other launch objects) is probably "more correct" than the current behavior of raising an exception.

The primary concern that did cross my mind was that this change may make it more likely that tool could mistakenly proceed with a launch that has an inactive deployment. I've tried to prevent/deter/mitigate this possibility in a few ways:

• LtiDeployment objects are always created in an inactive state.
• A reasonable default return is provided for LtiLaunchBaseView.handle_inactive_deployment.
• LtiLaunchBaseView does not add the launch ID to the session until after the deployment is known to be active.

I also don't think this approach closes the door to adding some kind of swappable ToolConfig setting in the future, if we determine it is needed after all. Unless you have concerns, I think I'll proceed with the approach in this PR and submit for a final review after some docs/tests/etc are added.

[doreilly]

Unless you have concerns, I think I'll proceed with the approach in this PR and submit for a final review after some docs/tests/etc are added.

That all sounds good to me! Thanks @michaelwheeler!

[michaelwheeler]

This is ready for a quick review when you're back @doreilly.

[michaelwheeler]

@doreilly I'll go ahead and merge this, unless you have any changes you'd like to see.