Add a 'MixedModuleStore' which aggregates between XML and Mongo Modulestores - #496
Conversation
There was a problem hiding this comment.
What's going on here? Can you at least add a comment about why this is the way it is, and make the dummy value clearer (if that's what it is)?
|
@dmitchell @cpennington @e0d @jarv Can we pick this up this review? I think we're all anxious to get this done and up in prod. I'm still manually verifying on localdev. Soon will pick up testing on a dedicated AWS instance. |
|
@chrisndodge I had one comment about how to store the configuration values set by one-time-startup that you haven't addressed. Other than that, I'm 👍 |
There was a problem hiding this comment.
Seems like it would be better to write this line as '"{}"'.format(mock_static_content.convert_legacy_static_url_with_course_id())
|
Looks good to me; I checked out the code, and it works. |
There was a problem hiding this comment.
We need unit tests for this new class and its methods.
…oth XML and Mongo module stores
…s can be used to pass in settings run the Django tier
…r the isinstance() with respect to modulestores to use this get_modulestore_type()
…erty accessor. Also update some django-admin configs to use this means to set the runtime configuration
|
@jzoldak I think I have total coverage on MixedModuleStore (and the create_modulestore_instance). I think just that Content method is the only thing uncovered now. |
…een mapped, unless the store provider has been labeled as 'default'
|
@cpennington @dmitchell @singingwolfboy @jzoldak I believe I've addressed the various comments. We have a go/no-go meeting at 12:30 about whether we deploy this next week. Can I get final comments and/or +1's? |
There was a problem hiding this comment.
Can we get a unit test for the get_html method? I'm concerned because of the comment on line 177 "This won't work when we move to data that isn't on the filesystem"
There was a problem hiding this comment.
done (via a CMS test since I needed to go through some Module HTML rendering pipeline)
There was a problem hiding this comment.
Pythonically, this would probably just be written as a @property.setter method in the MixedModuleStore, and everywhere else the modulestore configuration would just be accessed as a regular attribute (xmlstore.configuration = {}).
I don't think this should block the PR, but it would be nice to capture it as a cleanup task (or, if someone has time, to clean it up now).
|
👍 |
|
OK, did a quick spot check on this branch when running in just XML and Mongo module store providers (aka old configuration). Courseware works, transcripts work, static images work. @dmitchell can I get one more thumbs on this? |
|
OK, per hip chat, Don OK'd the merge |
Add a 'MixedModuleStore' which aggregates between XML and Mongo Modulestores
Fix for 500 on showanswer for choiceresponse
…gging-cache-info Add logging for cache infor openedx#492
feat: add encrypt for email field and responding migration files
5 participants
This is an early PR while I work on manual/automated testing.
This should allow us to do away with:
@dmitchell @cpennington @jarv @e0d Can you all start to take a look? I'm still manually validating right now and I need to figure out some unit tests around this. But I figured I'd get the review process started.
Also, we need to come up with production configuration settings. Right now dev configuration is in lms/envs/mixed_dev.py so to run locally
rake lms[cms.mixed_dev]