[BD-24] [TNL-7318] BB-2355: Move LTI configuration to models. #93
Conversation
|
Thanks for the pull request, @giovannicimolin! I've created BLENDED-504 to keep track of it in Jira. More details are on the BD-24 project page. When this pull request is ready, tag your edX technical lead. |
openedx-webhooks
added
blended
|
@giovannicimolin I'm trying to test this PR on a master devstack but the XBlocks fails to load correctly after running migrations: Any ideas what I'm doing wrong? |
|
@viadanna I've hit that issue too at first, then I reinstalled the block and cleaned up all pyc files and it started working again. |
giovannicimolin
force-pushed
the
giovanni/bb-2355-change-config-to-model
branch
2 times, most recently
from
8605ed4
to
ac52bf2
Compare
|
@davestgermain @davidjoy I've rebased the PR bumped the version and squashed the commits. This is good to do. Note that this will require running migrations on stage/production, so I'm not sure how we should roll-out this feature. |
lti_consumer/models.py
Outdated
| # In the future, the LTI configuration will be | ||
| # stored in this model in a JSON field. | ||
| # This field stores a UsageKey. | ||
| location = models.CharField( |
There was a problem hiding this comment.
Why not use a UsageKeyField?
There was a problem hiding this comment.
Much better! I though that this was part of the core, but it seems that I was wrong. I'll implement it.
lti_consumer/models.py
Outdated
| (CONFIG_ON_XBLOCK, 'Configuration Stored on XBlock fields'), | ||
| ] | ||
| config_store = models.CharField( | ||
| max_length=25, |
There was a problem hiding this comment.
As far as I know, there isn't any space advantage to making this field small.
lti_consumer/models.py
Outdated
| """ | ||
| Return a class of LTI 1.1 consumer. | ||
|
|
||
| Not implemented yet. |
lti_consumer/lti_xblock.py
Outdated
| get_lti_consumer | ||
| ) | ||
|
|
||
| lti_config_id = get_or_create_local_lti_config( |
There was a problem hiding this comment.
Instead of having to get/create the lti_config_id and then immediately looking up the model by id, how about having something like get_lti_consumer(config_id=None, block=self), which will get_or_create the config and return the consumer? And since configuration is only on the block for now, you can avoid having to load the block again (in _load_block), since it's already being passed in.
lti_consumer/models.py
Outdated
| blank=True, | ||
| ) | ||
|
|
||
| def _load_block(self): # pragma: no cover |
There was a problem hiding this comment.
How about this instead? This way, you can set the block on the config if it's passed in to get_lti_consumer, otherwise it'll load the block.
@property
def block(self):
block = getattr(self, '_block', None)
if block is None:
from xmodule.modulestore.django import modulestore
block = self._block = modulestore().get_item(self.location)
return block
@block.setter
def block(self, block):
self._block = block
|
@davestgermain I've implemented your review suggestions and added a few more tests, can you take a look again? |
|
@giovannicimolin looks good to me! Squash and merge 👍 |
This change moves the LTI configuration retrieval to a Django model that will LTI configuration. The current model proxies back the to blocks to keep backwards compatibility. See reasoning for change at: https://github.com/edx/xblock-lti-consumer/blob/master/docs/decisions/0001-lti-extensions-plugin.rst
giovannicimolin
force-pushed
the
giovanni/bb-2355-change-config-to-model
branch
from
08dfee6
to
4a70705
Compare
|
@davestgermain Done! |
This PR moves the LTI configuration handling to a django model. All the settings are still stored in the XBlock, but the retrieving and instancing of LTI config is done through a Python API now.
Testing instructions:
Notes:
There's no new "features" here, and this is just laying the groundwork to support LTI Advantage extensions.
This is the first implementation step of #92.
Reviewers: