GitHub PullRequestReviewCommentEvent at 2014-06-19 16:15:19
Repository: edx-platform
Actor: cahrens
Payload: {"action":"created","comment":{"url":"https://api.github.com/repos/edx/edx-platform/pulls/comments/13977308","id":13977308,"diff_hunk":"@@ -129,33 +155,23 @@ def get_courses(self):\n         Returns a list containing the top level XModuleDescriptors of the courses\n         in this modulestore.\n         '''\n-        # order the modulestores and ensure no dupes (default may be a dupe of a named store)\n-        # remove 'draft' as we know it's a functional dupe of 'direct' (ugly hardcoding)\n-        stores = set([value for key, value in self.modulestores.iteritems() if key != 'draft'])\n-        stores = sorted(stores, cmp=_compare_stores)\n-\n-        courses = {}  # a dictionary of stringified course locations to course objects\n-        has_locators = any(issubclass(CourseLocator, store.reference_type) for store in stores)\n-        for store in stores:\n+        courses = {}  # a dictionary of course keys to course objects\n+        has_locators = any(issubclass(CourseLocator, store.reference_type) for store in self.modulestores)\n+        for store in self.modulestores:\n             store_courses = store.get_courses()\n             # filter out ones which were fetched from earlier stores but locations may not be ==\n             for course in store_courses:\n-                course_location = unicode(course.location)\n-                if course_location not in courses:\n-                    if has_locators and isinstance(course.location, Location):\n+                if course.id not in courses:\n+                    if has_locators and isinstance(course.id, SlashSeparatedCourseKey):\n                         # see if a locator version of course is in the result\n                         try:\n-                            # if there's no existing mapping, then the course can't have been in split\n-                            course_locator = loc_mapper().translate_location(\n-                                course.location,\n-                                add_entry_if_missing=False\n-                            )\n-                            if unicode(course_locator) not in courses:\n-                                courses[course_location] = course\n+                            course_locator = loc_mapper().translate_location_to_course_locator(course.id)\n+                            if course_locator in courses:\n+                                continue\n                         except ItemNotFoundError:\n-                            courses[course_location] = course\n-                    else:\n-                        courses[course_location] = course\n+                            # if there's no existing mapping, then the course can't have been in split\n+                            pass","path":"common/lib/xmodule/xmodule/modulestore/mixed.py","position":154,"original_position":154,"commit_id":"630534086af8667afd483aba92a36c32ea3c6f57","original_commit_id":"630534086af8667afd483aba92a36c32ea3c6f57","user":{"login":"cahrens","id":484484,"avatar_url":"https://avatars.githubusercontent.com/u/484484?","gravatar_id":"c61f992b89d30875e4626db835a07b77","url":"https://api.github.com/users/cahrens","html_url":"https://github.com/cahrens","followers_url":"https://api.github.com/users/cahrens/followers","following_url":"https://api.github.com/users/cahrens/following{/other_user}","gists_url":"https://api.github.com/users/cahrens/gists{/gist_id}","starred_url":"https://api.github.com/users/cahrens/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/cahrens/subscriptions","organizations_url":"https://api.github.com/users/cahrens/orgs","repos_url":"https://api.github.com/users/cahrens/repos","events_url":"https://api.github.com/users/cahrens/events{/privacy}","received_events_url":"https://api.github.com/users/cahrens/received_events","type":"User","site_admin":false},"body":"In the pass case for ItemNotFoundError, won't courses[courses.id] still get assigned below since it is outside of the if statement?","created_at":"2014-06-19T16:15:19Z","updated_at":"2014-06-19T16:15:19Z","html_url":"https://github.com/edx/edx-platform/pull/3915#discussion_r13977308","pull_request_url":"https://api.github.com/repos/edx/edx-platform/pulls/3915","_links":{"self":{"href":"https://api.github.com/repos/edx/edx-platform/pulls/comments/13977308"},"html":{"href":"https://github.com/edx/edx-platform/pull/3915#discussion_r13977308"},"pull_request":{"href":"https://api.github.com/repos/edx/edx-platform/pulls/3915"}}},"pull_request":{"url":"https://api.github.com/repos/edx/edx-platform/pulls/3915","id":16462039,"html_url":"https://github.com/edx/edx-platform/pull/3915","diff_url":"https://github.com/edx/edx-platform/pull/3915.diff","patch_url":"https://github.com/edx/edx-platform/pull/3915.patch","issue_url":"https://api.github.com/repos/edx/edx-platform/issues/3915","number":3915,"state":"open","title":"Studio/uses mixed modulestore","user":{"login":"nasthagiri","id":6772826,"avatar_url":"https://avatars.githubusercontent.com/u/6772826?","gravatar_id":"907b8b314ff2b32d218325077c45b6f0","url":"https://api.github.com/users/nasthagiri","html_url":"https://github.com/nasthagiri","followers_url":"https://api.github.com/users/nasthagiri/followers","following_url":"https://api.github.com/users/nasthagiri/following{/other_user}","gists_url":"https://api.github.com/users/nasthagiri/gists{/gist_id}","starred_url":"https://api.github.com/users/nasthagiri/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/nasthagiri/subscriptions","organizations_url":"https://api.github.com/users/nasthagiri/orgs","repos_url":"https://api.github.com/users/nasthagiri/repos","events_url":"https://api.github.com/users/nasthagiri/events{/privacy}","received_events_url":"https://api.github.com/users/nasthagiri/received_events","type":"User","site_admin":false},"body":"https://edx-wiki.atlassian.net/browse/STUD-1540\r\n\r\n**MixedModuleStore API**\r\n* Updated Studio and LMS so all callers, with a few exceptions, go through the `MixedModulestore`.\r\n  * to find the exceptions (a few tests and 2 management commands), search for callers to `_get_modulestore_by_type`.\r\n* Updated the `MixedModuleStore` API and all their callers to take in the `user_id` for all Edit operations.\r\n\r\n**Mongo Modulestore**\r\n* Callers should no longer distinguish between 'direct' and 'draft' Mongo modulestores.  All users of the Mongo store now always go through the `DraftModuleStore`.\r\n* Callers can specify whether they want only published versions in their runtime by configuring the modulestore's branch to 'published' (LMS).  The default setting is 'draft' (used by CMS and Preview) which prefers Draft versions over Published versions.\r\n* Since (most) test code now go through the Draft store, some tests needed to be updated to explicitly Publish their newly created items if they assumed they would be in that state.\r\n* API changes:\r\n  * Updated APIs for `has_item`, `get_parent_locations`, `get_items`, and `delete_item` to take in a `revision` parameter.  Refer to docstrings for more detail.\r\n* Improvements to the implementation of the following APIs:\r\n  * Publish - so it automatically publishes all the downstream children.\r\n  * Delete - so it does the \"right\" thing depending on which revision is deleted, etc.\r\n\r\n**Settings**\r\n* Changed the configuration setting format so the Stores listed in the Mixed configuration can be ordered by preference, via a `list` rather than a `dict`.\r\n* Updated the settings for all Test code so they should all go through Mixed, unless they are explicitly testing a particular store.\r\n* Introduced a new Setting called `MODULESTORE_BRANCH` that specifies the default \"branch\" (draft or published) to be used by relevant modulestore methods.  Currently, the `DraftModuleStore` uses this setting.  Eventually, the `SplitModuleStore` will also honor it.\r\n\r\n**Testing**\r\nThe following should be tested before Release:\r\n* Performance - new indirection through Mixed (for CMS) and through Draft (for LMS).\r\n* Preview from Studio - make sure both Draft and Published items are seen correctly.\r\n* Migration of very old roles since 0036_access_roles_orgless.py was changed.\r\n* Different configurations - devstack, non-devstack, sandbox, prod.\r\n* Which tests have multiple stores configured at once (xml and old mongo)?\r\n* Export/Import course - draft/published content\r\n\r\n**Reviewers**: @cahrens (Studio) @dianakhuang (LMS) @feanil (Devops)","created_at":"2014-05-29T01:28:24Z","updated_at":"2014-06-19T16:15:19Z","closed_at":null,"merged_at":null,"merge_commit_sha":"0e8fe1077a3502bc04ea7c79d50e114416ebe8ab","assignee":null,"milestone":null,"commits_url":"https://api.github.com/repos/edx/edx-platform/pulls/3915/commits","review_comments_url":"https://api.github.com/repos/edx/edx-platform/pulls/3915/comments","review_comment_url":"https://api.github.com/repos/edx/edx-platform/pulls/comments/{number}","comments_url":"https://api.github.com/repos/edx/edx-platform/issues/3915/comments","statuses_url":"https://api.github.com/repos/edx/edx-platform/statuses/630534086af8667afd483aba92a36c32ea3c6f57","head":{"label":"edx:studio/uses-mixed-modulestore","ref":"studio/uses-mixed-modulestore","sha":"630534086af8667afd483aba92a36c32ea3c6f57","user":{"login":"edx","id":3179841,"avatar_url":"https://avatars.githubusercontent.com/u/3179841?","gravatar_id":"e1f6c6dc8772c47c0261cd5df2ed2814","url":"https://api.github.com/users/edx","html_url":"https://github.com/edx","followers_url":"https://api.github.com/users/edx/followers","following_url":"https://api.github.com/users/edx/following{/other_user}","gists_url":"https://api.github.com/users/edx/gists{/gist_id}","starred_url":"https://api.github.com/users/edx/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/edx/subscriptions","organizations_url":"https://api.github.com/users/edx/orgs","repos_url":"https://api.github.com/users/edx/repos","events_url":"https://api.github.com/users/edx/events{/privacy}","received_events_url":"https://api.github.com/users/edx/received_events","type":"Organization","site_admin":false},"repo":{"id":10391073,"name":"edx-platform","full_name":"edx/edx-platform","owner":{"login":"edx","id":3179841,"avatar_url":"https://avatars.githubusercontent.com/u/3179841?","gravatar_id":"e1f6c6dc8772c47c0261cd5df2ed2814","url":"https://api.github.com/users/edx","html_url":"https://github.com/edx","followers_url":"https://api.github.com/users/edx/followers","following_url":"https://api.github.com/users/edx/following{/other_user}","gists_url":"https://api.github.com/users/edx/gists{/gist_id}","starred_url":"https://api.github.com/users/edx/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/edx/subscriptions","organizations_url":"https://api.github.com/users/edx/orgs","repos_url":"https://api.github.com/users/edx/repos","events_url":"https://api.github.com/users/edx/events{/privacy}","received_events_url":"https://api.github.com/users/edx/received_events","type":"Organization","site_admin":false},"private":false,"html_url":"https://github.com/edx/edx-platform","description":"Open edX, the edX platform that powers http://www.edx.org","fork":false,"url":"https://api.github.com/repos/edx/edx-platform","forks_url":"https://api.github.com/repos/edx/edx-platform/forks","keys_url":"https://api.github.com/repos/edx/edx-platform/keys{/key_id}","collaborators_url":"https://api.github.com/repos/edx/edx-platform/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/edx/edx-platform/teams","hooks_url":"https://api.github.com/repos/edx/edx-platform/hooks","issue_events_url":"https://api.github.com/repos/edx/edx-platform/issues/events{/number}","events_url":"https://api.github.com/repos/edx/edx-platform/events","assignees_url":"https://api.github.com/repos/edx/edx-platform/assignees{/user}","branches_url":"https://api.github.com/repos/edx/edx-platform/branches{/branch}","tags_url":"https://api.github.com/repos/edx/edx-platform/tags","blobs_url":"https://api.github.com/repos/edx/edx-platform/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/edx/edx-platform/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/edx/edx-platform/git/refs{/sha}","trees_url":"https://api.github.com/repos/edx/edx-platform/git/trees{/sha}","statuses_url":"https://api.github.com/repos/edx/edx-platform/statuses/{sha}","languages_url":"https://api.github.com/repos/edx/edx-platform/languages","stargazers_url":"https://api.github.com/repos/edx/edx-platform/stargazers","contributors_url":"https://api.github.com/repos/edx/edx-platform/contributors","subscribers_url":"https://api.github.com/repos/edx/edx-platform/subscribers","subscription_url":"https://api.github.com/repos/edx/edx-platform/subscription","commits_url":"https://api.github.com/repos/edx/edx-platform/commits{/sha}","git_commits_url":"https://api.github.com/repos/edx/edx-platform/git/commits{/sha}","comments_url":"https://api.github.com/repos/edx/edx-platform/comments{/number}","issue_comment_url":"https://api.github.com/repos/edx/edx-platform/issues/comments/{number}","contents_url":"https://api.github.com/repos/edx/edx-platform/contents/{+path}","compare_url":"https://api.github.com/repos/edx/edx-platform/compare/{base}...{head}","merges_url":"https://api.github.com/repos/edx/edx-platform/merges","archive_url":"https://api.github.com/repos/edx/edx-platform/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/edx/edx-platform/downloads","issues_url":"https://api.github.com/repos/edx/edx-platform/issues{/number}","pulls_url":"https://api.github.com/repos/edx/edx-platform/pulls{/number}","milestones_url":"https://api.github.com/repos/edx/edx-platform/milestones{/number}","notifications_url":"https://api.github.com/repos/edx/edx-platform/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/edx/edx-platform/labels{/name}","releases_url":"https://api.github.com/repos/edx/edx-platform/releases{/id}","created_at":"2013-05-30T20:20:38Z","updated_at":"2014-06-19T16:00:44Z","pushed_at":"2014-06-19T16:00:45Z","git_url":"git://github.com/edx/edx-platform.git","ssh_url":"git@github.com:edx/edx-platform.git","clone_url":"https://github.com/edx/edx-platform.git","svn_url":"https://github.com/edx/edx-platform","homepage":"http://code.edx.org/","size":1161502,"stargazers_count":1260,"watchers_count":1260,"language":"Python","has_issues":true,"has_downloads":true,"has_wiki":true,"forks_count":661,"mirror_url":null,"open_issues_count":131,"forks":661,"open_issues":131,"watchers":1260,"default_branch":"master"}},"base":{"label":"edx:master","ref":"master","sha":"4477c2857f7a62f2279ba34be465e1a87ed40c93","user":{"login":"edx","id":3179841,"avatar_url":"https://avatars.githubusercontent.com/u/3179841?","gravatar_id":"e1f6c6dc8772c47c0261cd5df2ed2814","url":"https://api.github.com/users/edx","html_url":"https://github.com/edx","followers_url":"https://api.github.com/users/edx/followers","following_url":"https://api.github.com/users/edx/following{/other_user}","gists_url":"https://api.github.com/users/edx/gists{/gist_id}","starred_url":"https://api.github.com/users/edx/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/edx/subscriptions","organizations_url":"https://api.github.com/users/edx/orgs","repos_url":"https://api.github.com/users/edx/repos","events_url":"https://api.github.com/users/edx/events{/privacy}","received_events_url":"https://api.github.com/users/edx/received_events","type":"Organization","site_admin":false},"repo":{"id":10391073,"name":"edx-platform","full_name":"edx/edx-platform","owner":{"login":"edx","id":3179841,"avatar_url":"https://avatars.githubusercontent.com/u/3179841?","gravatar_id":"e1f6c6dc8772c47c0261cd5df2ed2814","url":"https://api.github.com/users/edx","html_url":"https://github.com/edx","followers_url":"https://api.github.com/users/edx/followers","following_url":"https://api.github.com/users/edx/following{/other_user}","gists_url":"https://api.github.com/users/edx/gists{/gist_id}","starred_url":"https://api.github.com/users/edx/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/edx/subscriptions","organizations_url":"https://api.github.com/users/edx/orgs","repos_url":"https://api.github.com/users/edx/repos","events_url":"https://api.github.com/users/edx/events{/privacy}","received_events_url":"https://api.github.com/users/edx/received_events","type":"Organization","site_admin":false},"private":false,"html_url":"https://github.com/edx/edx-platform","description":"Open edX, the edX platform that powers http://www.edx.org","fork":false,"url":"https://api.github.com/repos/edx/edx-platform","forks_url":"https://api.github.com/repos/edx/edx-platform/forks","keys_url":"https://api.github.com/repos/edx/edx-platform/keys{/key_id}","collaborators_url":"https://api.github.com/repos/edx/edx-platform/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/edx/edx-platform/teams","hooks_url":"https://api.github.com/repos/edx/edx-platform/hooks","issue_events_url":"https://api.github.com/repos/edx/edx-platform/issues/events{/number}","events_url":"https://api.github.com/repos/edx/edx-platform/events","assignees_url":"https://api.github.com/repos/edx/edx-platform/assignees{/user}","branches_url":"https://api.github.com/repos/edx/edx-platform/branches{/branch}","tags_url":"https://api.github.com/repos/edx/edx-platform/tags","blobs_url":"https://api.github.com/repos/edx/edx-platform/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/edx/edx-platform/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/edx/edx-platform/git/refs{/sha}","trees_url":"https://api.github.com/repos/edx/edx-platform/git/trees{/sha}","statuses_url":"https://api.github.com/repos/edx/edx-platform/statuses/{sha}","languages_url":"https://api.github.com/repos/edx/edx-platform/languages","stargazers_url":"https://api.github.com/repos/edx/edx-platform/stargazers","contributors_url":"https://api.github.com/repos/edx/edx-platform/contributors","subscribers_url":"https://api.github.com/repos/edx/edx-platform/subscribers","subscription_url":"https://api.github.com/repos/edx/edx-platform/subscription","commits_url":"https://api.github.com/repos/edx/edx-platform/commits{/sha}","git_commits_url":"https://api.github.com/repos/edx/edx-platform/git/commits{/sha}","comments_url":"https://api.github.com/repos/edx/edx-platform/comments{/number}","issue_comment_url":"https://api.github.com/repos/edx/edx-platform/issues/comments/{number}","contents_url":"https://api.github.com/repos/edx/edx-platform/contents/{+path}","compare_url":"https://api.github.com/repos/edx/edx-platform/compare/{base}...{head}","merges_url":"https://api.github.com/repos/edx/edx-platform/merges","archive_url":"https://api.github.com/repos/edx/edx-platform/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/edx/edx-platform/downloads","issues_url":"https://api.github.com/repos/edx/edx-platform/issues{/number}","pulls_url":"https://api.github.com/repos/edx/edx-platform/pulls{/number}","milestones_url":"https://api.github.com/repos/edx/edx-platform/milestones{/number}","notifications_url":"https://api.github.com/repos/edx/edx-platform/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/edx/edx-platform/labels{/name}","releases_url":"https://api.github.com/repos/edx/edx-platform/releases{/id}","created_at":"2013-05-30T20:20:38Z","updated_at":"2014-06-19T16:00:44Z","pushed_at":"2014-06-19T16:00:45Z","git_url":"git://github.com/edx/edx-platform.git","ssh_url":"git@github.com:edx/edx-platform.git","clone_url":"https://github.com/edx/edx-platform.git","svn_url":"https://github.com/edx/edx-platform","homepage":"http://code.edx.org/","size":1161502,"stargazers_count":1260,"watchers_count":1260,"language":"Python","has_issues":true,"has_downloads":true,"has_wiki":true,"forks_count":661,"mirror_url":null,"open_issues_count":131,"forks":661,"open_issues":131,"watchers":1260,"default_branch":"master"}},"_links":{"self":{"href":"https://api.github.com/repos/edx/edx-platform/pulls/3915"},"html":{"href":"https://github.com/edx/edx-platform/pull/3915"},"issue":{"href":"https://api.github.com/repos/edx/edx-platform/issues/3915"},"comments":{"href":"https://api.github.com/repos/edx/edx-platform/issues/3915/comments"},"review_comments":{"href":"https://api.github.com/repos/edx/edx-platform/pulls/3915/comments"},"review_comment":{"href":"https://api.github.com/repos/edx/edx-platform/pulls/comments/{number}"},"commits":{"href":"https://api.github.com/repos/edx/edx-platform/pulls/3915/commits"},"statuses":{"href":"https://api.github.com/repos/edx/edx-platform/statuses/630534086af8667afd483aba92a36c32ea3c6f57"}}}}