Skip to content

Make split mongo read-only API consistent with other modulestores. #667

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

Jump to bottom
Merged
cahrens merged 5 commits into from
Aug 15, 2013
Merged

Make split mongo read-only API consistent with other modulestores. #667

cahrens merged 5 commits into from
Aug 15, 2013

Conversation

@cahrens
Copy link

@cahrens cahrens commented Aug 13, 2013

https://edx-wiki.atlassian.net/browse/STUD-617

Some details:

  1. modulestore().get_containing_courses was only used in one place, and wasn't actually needed there. I deleted it.
  2. get_errored_courses only makes sense for XML modulestore (it wasn't a method of the base class) and was only used in one place. I made that place check if the modulestore has the method. Deleted dummy implementations from the other modulestores.
  3. mongo's get_course_for_item method is just a helper function internal to that modulestore. Renamed it to make it more obvious, and deleted dummy implementation in split.
  4. Had to add arguments to some of the split methods to make the API consistent. Documented when these arguments are not used.

@cahrens
Copy link
Author

cahrens commented Aug 14, 2013

@dmitchell please review

dmitchell
dmitchell reviewed Aug 14, 2013
View reviewed changes
cms/djangoapps/contentstore/utils.py
Copy link
Contributor

@dmitchell dmitchell Aug 14, 2013

Choose a reason for hiding this comment

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

Should perhaps check that location.category == 'course' otherwise the course_id is not really the course_id. I was actually wondering whether we should change the course_id property to error if the locator is not a course.

Copy link
Author

@cahrens cahrens Aug 15, 2013

Choose a reason for hiding this comment

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

I am throwing an exception now in location.course_id if category is not 'course'. Also added unit test for it.

@dmitchell
Copy link
Contributor

dmitchell commented Aug 14, 2013

Looks reasonable. I haven't thought whether this is complete. 👍

chrisndodge
chrisndodge reviewed Aug 14, 2013
View reviewed changes
common/djangoapps/student/views.py Outdated
Copy link
Contributor

@chrisndodge chrisndodge Aug 14, 2013

Choose a reason for hiding this comment

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

This is going to be a problem for MixedModuleStore

Make get_errored_courses part of the modulestore API (with default im…
9f229a4 
…plementation).

Also clarifies the contraction of location.course_id by throwing an exception for lcoations that are not of category course.

Add test for course_id method.
@cahrens
Copy link
Author

cahrens commented Aug 15, 2013

@chrisndodge @dmitchell Please review again. I put get_errored_courses into ModuleStoreBase and changed to throwing InvalidLocationException in location.course_id if location is not of category 'course'.

cahrens pushed a commit that referenced this pull request Aug 15, 2013
Merge pull request #667 from edx/christina/read-only-api
03b6050 
Make split mongo read-only API consistent with other modulestores.
@cahrens cahrens merged commit 03b6050 into master Aug 15, 2013
@cahrens cahrens deleted the christina/read-only-api branch August 15, 2013 20:06
jenkins-ks pushed a commit to nttks/edx-platform that referenced this pull request Mar 30, 2016
@yokose-ks
Fix bugs openedx#653, openedx#667
09a5e09
bradenmacdonald referenced this pull request in open-craft/openedx-platform Apr 11, 2016
@msaqib52
Merge pull request #667 from edx-solutions/msaqib52/YONK-294
d6ba608 
Update user detail API to add last_login
dfrojas pushed a commit to eduNEXT/edx-platform that referenced this pull request Aug 31, 2017
@caseylitton
Merge pull request openedx#667 from caseylitton/revert-merge-657-commits
9500da1 
Revert merge 657 commits
jfavellar90 pushed a commit to eduNEXT/edx-platform that referenced this pull request Apr 11, 2018
Merge pull request openedx#667 from proversity-org/master
f859872 
Fixes
Sujeet1379 pushed a commit to chandrudev/edx-platform that referenced this pull request Nov 17, 2022
@Tj-Tracy
[feat] Add notices redirect to learning MFE (openedx#667)
3fcc0d8 
* [feat] MICROBA-1523 Add notices redirect to learning MFE

To support the notices plugin on platform, this adds a redirect to the
course home page. If a user lands on that page and has not acknowledged
a notice, the user will be redirected to notice instead of the course
home.
DanielVZ96 referenced this pull request in open-craft/openedx-platform Jan 31, 2024
@timmc-edx
Document usual devstack workflow (#667)
deca8c5 
This incorporates some material removed in commit 9a94a27, added to
the Variations section and a new Notices section in the main README.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@cahrens @dmitchell @chrisndodge