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.

Sign up for GitHub

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

MixedModulestore wraps most getters, update_item, delete_item #2356

Merged
merged 4 commits into from
Feb 10, 2014
Merged

MixedModulestore wraps most getters, update_item, delete_item #2356

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c601b75
MixedModulestore wraps most getters, update_item, delete_item
Jan 16, 2014
53f7c50
Added test for defaulting use_locations
Jan 31, 2014
b8ea7f3
update_item and other refactorings continued
Jan 31, 2014
1de9d55
Review-driven changes (to-be-squashed)
Feb 5, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions CHANGELOG.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,12 @@ Blades: Fix comparison of float numbers. BLD-434.

Blades: Allow regexp strings as the correct answer to a string response question. BLD-475.

Common: MixedModulestore is now the only approved access to the persistence layer
- takes a new parameter 'reference_type' which can be 'Location' or 'Locator'. Mixed
then tries to ensure that every reference in any xblock gets converted to that type on
retrieval. Because we're moving to Locators, the default is Locator; so, you should change
all existing configurations to 'Location' (unless you're using split)

Common: Add feature flags to allow developer use of pure XBlocks
- ALLOW_ALL_ADVANCED_COMPONENTS disables the hard-coded list of advanced
components in Studio, and allows any xblock to be added as an
Expand Down
8 changes: 4 additions & 4 deletions cms/djangoapps/contentstore/course_info_model.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ def get_course_updates(location, provided_id):
return course_upd_collection


def update_course_updates(location, update, passed_id=None):
def update_course_updates(location, update, passed_id=None, user=None):
"""
Either add or update the given course update. It will add it if the passed_id is absent or None. It will update it if
it has an passed_id which has a valid value. Until updates have distinct values, the passed_id is the location url + an index
Expand Down Expand Up @@ -102,7 +102,7 @@ def update_course_updates(location, update, passed_id=None):

# update db record
course_updates.data = html.tostring(course_html_parsed)
modulestore('direct').update_item(location, course_updates.data)
modulestore('direct').update_item(course_updates, user.id if user else None)

return {
"id": idx,
Expand All @@ -125,7 +125,7 @@ def _course_info_content(html_parsed):


# pylint: disable=unused-argument
def delete_course_update(location, update, passed_id):
def delete_course_update(location, update, passed_id, user):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does user need to default to None?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so. I only had it defaulting to None where there were existing kwargs ahead of it or where I couldn't change all callers.

"""
Delete the given course_info update from the db.
Returns the resulting course_updates b/c their ids change.
Expand Down Expand Up @@ -158,7 +158,7 @@ def delete_course_update(location, update, passed_id):
# update db record
course_updates.data = html.tostring(course_html_parsed)
store = modulestore('direct')
store.update_item(location, course_updates.data)
store.update_item(course_updates, user.id)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to check if user is None?


return get_course_updates(location, None)

Expand Down
2 changes: 1 addition & 1 deletion cms/djangoapps/contentstore/tests/test_checklists.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ def test_get_checklists(self):
# Save the changed `checklists` to the underlying KeyValueStore before updating the modulestore
self.course.save()
modulestore = get_modulestore(self.course.location)
modulestore.update_metadata(self.course.location, own_metadata(self.course))
modulestore.update_item(self.course, self.user.id)
self.assertEqual(self.get_persisted_checklists(), None)
response = self.client.get(self.checklists_url)
self.assertEqual(payload, response.content)
Expand Down
84 changes: 18 additions & 66 deletions cms/djangoapps/contentstore/tests/test_contentstore.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,6 @@

from django_comment_common.utils import are_permissions_roles_seeded
from xmodule.exceptions import InvalidVersionError
import datetime
from pytz import UTC
from uuid import uuid4
from pymongo import MongoClient
from student.models import CourseEnrollment
Expand Down Expand Up @@ -126,11 +124,7 @@ def check_components_on_page(self, component_types, expected_types):

course.advanced_modules = component_types

# Save the data that we've just changed to the underlying
# MongoKeyValueStore before we update the mongo datastore.
course.save()

store.update_metadata(course.location, own_metadata(course))
store.update_item(course, self.user.id)

# just pick one vertical
descriptor = store.get_items(Location('i4x', 'edX', 'simple', 'vertical', None, None))[0]
Expand Down Expand Up @@ -269,7 +263,7 @@ def test_draft_metadata(self):
self.assertIn('graceperiod', own_metadata(html_module))
self.assertEqual(html_module.graceperiod, new_graceperiod)

draft_store.update_metadata(html_module.location, own_metadata(html_module))
draft_store.update_item(html_module, self.user.id)

# read back to make sure it reads as 'own-metadata'
html_module = draft_store.get_item(Location('i4x', 'edX', 'simple', 'html', 'test_html', None))
Expand Down Expand Up @@ -385,8 +379,7 @@ def test_create_static_tab_and_rename(self):
self.assertEqual(course.tabs, expected_tabs)

item.display_name = 'Updated'
item.save()
module_store.update_metadata(item.location, own_metadata(item))
module_store.update_item(item, self.user.id)

course = module_store.get_item(course_location)

Expand Down Expand Up @@ -834,9 +827,9 @@ def test_portable_link_rewrites_during_clone_course(self):
html_module = module_store.get_instance(source_location.course_id, html_module_location)

self.assertIsInstance(html_module.data, basestring)
new_data = html_module.data.replace('/static/', '/c4x/{0}/{1}/asset/'.format(
new_data = html_module.data = html_module.data.replace('/static/', '/c4x/{0}/{1}/asset/'.format(
source_location.org, source_location.course))
module_store.update_item(html_module_location, new_data)
module_store.update_item(html_module, self.user.id)

html_module = module_store.get_instance(source_location.course_id, html_module_location)
self.assertEqual(new_data, html_module.data)
Expand All @@ -858,22 +851,18 @@ def test_illegal_draft_crud_ops(self):
draft_store = modulestore('draft')
direct_store = modulestore('direct')

CourseFactory.create(org='MITx', course='999', display_name='Robot Super Course')
course = CourseFactory.create(org='MITx', course='999', display_name='Robot Super Course')

location = Location('i4x://MITx/999/chapter/neuvo')
# Ensure draft mongo store does not allow us to create chapters either directly or via convert to draft
self.assertRaises(InvalidVersionError, draft_store.create_and_save_xmodule, location)
direct_store.create_and_save_xmodule(location)
self.assertRaises(InvalidVersionError, draft_store.convert_to_draft, location)
chapter = draft_store.get_instance(course.id, location)
chapter.data = 'chapter data'

self.assertRaises(InvalidVersionError, draft_store.update_item, location, 'chapter data')

# taking advantage of update_children and other functions never checking that the ids are valid
self.assertRaises(InvalidVersionError, draft_store.update_children, location,
['i4x://MITx/999/problem/doesntexist'])

self.assertRaises(InvalidVersionError, draft_store.update_metadata, location,
{'due': datetime.datetime.now(UTC)})
with self.assertRaises(InvalidVersionError):
draft_store.update_item(chapter, self.user.id)

self.assertRaises(InvalidVersionError, draft_store.unpublish, location)

Expand Down Expand Up @@ -992,8 +981,8 @@ def test_export_course(self, mock_get):
sequential = module_store.get_item(Location(['i4x', 'edX', 'toy',
'sequential', 'vertical_sequential', None]))
private_location_no_draft = private_vertical.location.replace(revision=None)
module_store.update_children(sequential.location, sequential.children +
[private_location_no_draft.url()])
sequential.children.append(private_location_no_draft.url())
module_store.update_item(sequential, self.user.id)

# read back the sequential, to make sure we have a pointer to
sequential = module_store.get_item(Location(['i4x', 'edX', 'toy',
Expand Down Expand Up @@ -1285,31 +1274,6 @@ def test_prefetch_children(self):
self.assertFalse(Location(['i4x', 'edX', 'toy', 'vertical', 'vertical_test', None])
in course.system.module_data)

def test_export_course_with_unknown_metadata(self):
module_store = modulestore('direct')
content_store = contentstore()

import_from_xml(module_store, 'common/test/data/', ['toy'])
location = CourseDescriptor.id_to_location('edX/toy/2012_Fall')

root_dir = path(mkdtemp_clean())

course = module_store.get_item(location)

metadata = own_metadata(course)
# add a bool piece of unknown metadata so we can verify we don't throw an exception
metadata['new_metadata'] = True

# Save the data that we've just changed to the underlying
# MongoKeyValueStore before we update the mongo datastore.
course.save()
module_store.update_metadata(location, metadata)

print 'Exporting to tempdir = {0}'.format(root_dir)

# export out to a tempdir
export_to_xml(module_store, content_store, location, root_dir, 'test_export')

def test_export_course_without_content_store(self):
module_store = modulestore('direct')
content_store = contentstore()
Expand All @@ -1319,16 +1283,7 @@ def test_export_course_without_content_store(self):
import_from_xml(module_store, 'common/test/data/', ['toy'])
location = CourseDescriptor.id_to_location('edX/toy/2012_Fall')

# Add a sequence

stub_location = Location(['i4x', 'edX', 'toy', 'sequential', 'vertical_sequential'])
sequential = module_store.get_item(stub_location)
module_store.update_children(sequential.location, sequential.children)

# Get course and export it without a content_store

course = module_store.get_item(location)
course.save()

root_dir = path(mkdtemp_clean())

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test no longer has any value?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe it's an error to set an unknown field on an xblock; so, this test was in violation of the xblock assumptions. It got away with it by only setting it in the db as if it were a field and assuming the kvs wouldn't access it.

@cpennington Do you think I should have preserved some test of unknown fields in the db?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, deleting it is fine.

Expand All @@ -1343,7 +1298,7 @@ def test_export_course_without_content_store(self):
module_store, root_dir, ['test_export_no_content_store'],
draft_store=None,
static_content_store=None,
target_location_namespace=course.location
target_location_namespace=location
)

# Verify reimported course
Expand Down Expand Up @@ -1810,7 +1765,8 @@ def test_metadata_inheritance(self):
# crate a new module and add it as a child to a vertical
module_store.create_and_save_xmodule(new_component_location)
parent = verticals[0]
module_store.update_children(parent.location, parent.children + [new_component_location.url()])
parent.children.append(new_component_location.url())
module_store.update_item(parent, self.user.id)

# flush the cache
module_store.refresh_cached_metadata_inheritance_tree(new_component_location)
Expand All @@ -1827,8 +1783,7 @@ def test_metadata_inheritance(self):
# now let's define an override at the leaf node level
#
new_module.graceperiod = timedelta(1)
new_module.save()
module_store.update_metadata(new_module.location, own_metadata(new_module))
module_store.update_item(new_module, self.user.id)

# flush the cache and refetch
module_store.refresh_cached_metadata_inheritance_tree(new_component_location)
Expand Down Expand Up @@ -1942,10 +1897,7 @@ def test_metadata_not_persistence(self):
delattr(self.video_descriptor, field_name)

self.assertNotIn('html5_sources', own_metadata(self.video_descriptor))
get_modulestore(location).update_metadata(
location,
own_metadata(self.video_descriptor)
)
get_modulestore(location).update_item(self.video_descriptor, '**replace_user**')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this right? Shouldn't that be a mock user, rather than a string?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nevermind. Was mixing up this w/ the api further up that takes a user object.

That said: user.id is going to be an int, not a string.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. At some point we need to decide whether the pattern should be to pass the user or user.id (per @singingwolfboy 's comment) and how to handle callers which don't have a user (e.g., command line invocations). If possible, I'd like to handle that decision either in the next iteration or in a completely separate story. I've added it to https://edx-wiki.atlassian.net/browse/STUD-452

module = get_modulestore(location).get_item(location)

self.assertNotIn('html5_sources', own_metadata(module))
Expand Down Expand Up @@ -2001,7 +1953,7 @@ def _course_factory_create_course():
Creates a course via the CourseFactory and returns the locator for it.
"""
course = CourseFactory.create(org='MITx', course='999', display_name='Robot Super Course')
return loc_mapper().translate_location(course.location.course_id, course.location, True, True)
return loc_mapper().translate_location(course.id, course.location, False, True)


def _get_course_id(test_course_data):
Expand Down
Loading