MixedModulestore wraps most getters, update_item, delete_item #2356
Conversation
| @@ -1360,6 +1355,16 @@ def definition_locator(self, definition): | |||
| else: | |||
| return DefinitionLocator(definition['_id']) | |||
|
|
|||
| def get_modulestore_type(self, course_id): | |||
There was a problem hiding this comment.
No change, just moved up to the "public methods" part of the file. It was in the midst of the underscored private methods.
|
@cahrens @singingwolfboy I think this is ready to merge. It passed performance tests and unit tests. I'll rebase it. (Rebase is not trivial. I'll resume first thing in the morning) |
|
Oh, and I'm doing a subsequent PR which tests all of the paths in mixed ms as a separate PR. |
with code to translate between addressing schemes based on app and persistence layer addressing scheme specification. STUD-1206
| @@ -179,7 +179,7 @@ def primitive_delete(course, num): | |||
| # Note for future implementations: if you delete a static_tab, then Chris Dodge | |||
| # points out that there's other stuff to delete beyond this element. | |||
| # This code happens to not delete static_tab so it doesn't come up. | |||
| modulestore('direct').update_metadata(course.location, own_metadata(course)) | |||
| modulestore('direct').update_item(course, '**replace_user**') | |||
There was a problem hiding this comment.
If you have not already, please create a story (in database arch epic) for fixing these replace_user instances.
|
Why does |
| altered_grader = CourseGradingModel.fetch(self.course_locator) | ||
| self.assertDictEqual(test_grader.grade_cutoffs, altered_grader.grade_cutoffs, "cutoff change 'Pass'") | ||
|
|
||
| def test_delete_grace_period(self): | ||
| test_grader = CourseGradingModel.fetch(self.course_locator) | ||
| CourseGradingModel.update_grace_period_from_json(self.course_locator, test_grader.grace_period) | ||
| CourseGradingModel.update_grace_period_from_json( | ||
| self.course_locator, test_grader.grace_period, self.user) |
There was a problem hiding this comment.
Can you move the closing parenthesis onto its own line? It's more readable that way.
CourseGradingModel.update_grace_period_from_json(
self.course_locator, test_grader.grace_period, self.user,
)There was a problem hiding this comment.
all a matter of convention and opinion, but sure. I get tired of 3 lines per expression just to do a line wrap.
| @@ -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) | |||
There was a problem hiding this comment.
Do we need to check if user is None?
|
@cahrens @cpennington @singingwolfboy I've submitted my changes in response to the review comments. Please approve. |
|
👍 |
|
I'm still not happy about passing |
MixedModulestore wraps most getters, update_item, delete_item
with code to translate between addressing schemes based on app
and persistence layer addressing scheme specification.
STUD-1206
@singingwolfboy @cahrens Can you go ahead and review this. I still need to refactor some of the caching logic to ensure it performs ok in production. When I add that, I'll add cale and ormsbee to the reviewers.
The big change here is refactoring update_item to take an xblock and persist all of it's changes to the db rather than having the caller use update_[item|children|metadata] w/ location and json.
The purpose of this PR; however, is the titular description.