Refactor LTI consumer classes to enable wider use of LTI features

[MichaelRoytman]

Context

The current implementation of LTI specifications uses a set of "LTI consumer" classes. These classes define the various LTI 1.3 features: basic LTI 1.3 functionality, LTI 1.3 Advantage Services, and LTI 1.3 Proctoring Services. These consumer classes follow an inheritance model.

• LtiConsumer1p3 is the base class and implements a basic LTI 1.3 launch.
• LtiAdvantageConsumer extends LtiConsumer1p3 and implements an LTI 1.3 Advantage Services launch.
  • Note that basic LTI 1.3 launches use LtiAdvantageConsumer, not LtiConsumer1p3.
• LtiProctoringConsumer extends LtiAdvantageConsumer and implements an LTI 1.3 Proctoring Services launch.

Problem

This became an issue in #297.

Because of the inheritance model, it is not possible to have an LTI integration that can both support LTI 1.3 Advantage Services and LTI Proctoring Services, because they're implemented as separate classes. This limits the use of and expansion of this library.

• For reusable LTI configurations (i.e. those with config_store = CONFIG_EXTERNAL or config_store = CONFIG_ON_DB), this means that a single LTI configuration cannot support both LTI features and that separate configs will need to be created to support the features.
• For LTI configurations on an XBlock (i.e. those with config_store = CONFIG_ON_XBLOCK), this means that a single XBlock cannot support both LTI features. For the time being, that seems acceptable, but it is a limitation.

Solution

The recommendation is to refactor the LTI consumer classes to share code through composition instead of inheritance. Suggestions from Giovanni on #297 are quoted below.

@MichaelRoytman These lines mean that if lti_1p3_proctoring_enabled is true, then none of the advantage services will be available to that consumer, right? If we're working with a globally re-usable configuration then this adds a constraint: proctoring configs and other types of LTI configs must always be separate.

We don't have a use case for this right now, so this shouldn't block this ticket, but can you add a comment mentioning this new constraint with some context?

I have one idea in mind: instead of using inheritance in the LtiConsumer, we could use composition, and add the services incrementally to the main class only if they are enabled in the configuration.

I did a review pass and left some comments. Something that bothers me a bit is the added complexity that having two different consumer classes is added. Maybe we should rework this and work with composition, only setting up each service if it is defined in the LTI configuration?

I don't think this needs to be added here since it will require some rework, but worth keeping in mind and discussing on a GH issue.

Also, with these complications arising, I'd be in favor of switching from inheritance to composition in the LtiConsumer classes - only configuring the LTI services if they are in the LTI config and throwing errors if the clients try to launch invalid LTI setups.