Skip to content

Auto-enroll course staff to fix "View Live". - #578

Merged
cahrens merged 2 commits into
masterfrom
christina/autoenroll
Aug 6, 2013
Merged

Auto-enroll course staff to fix "View Live".#578
cahrens merged 2 commits into
masterfrom
christina/autoenroll

Conversation

@cahrens

@cahrens cahrens commented Aug 5, 2013

Copy link
Copy Markdown

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think this would be easier to read and understand:

def _get_course_id(self, test_course_data):
    """Returns the course ID (org/number/run)."""
    return "{org}/{number}/{run}".format(**test_course_data)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Will do.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

STUD-554

Code review feedback.
@cahrens

cahrens commented Aug 5, 2013

Copy link
Copy Markdown
Author

@chrisndodge I added the helper methods. I decided to preface them with "_", though I'm not entirely sure that is appropriate.

@chrisndodge

Copy link
Copy Markdown
Contributor

@cahrens I believe the '' is to mark a method as 'private', but I'm not sure. So if it is being called from an external python module, then my sense is that it shouldn't have a ''. @nedbat is probably the right perform to ask about that.

@cahrens

cahrens commented Aug 5, 2013

Copy link
Copy Markdown
Author

@chrisndodge I believe you are correct. We just sometimes use "_" for "private to us/I'm feeling a little dirty" methods. But I went ahead and removed the _ prefix (and rebased).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I see a try/catch in the LMS side call to this method. Do you know why it is needed there but not similarly in the CMS?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

This is the comment in the catch statement--

        # If we've already created this enrollment in a separate transaction,
        # then just continue

This certainly won't be the case in the CMS code path. In reality, I don't see how it is ever an issue because the method get_or_create returns the registration if the user is already registered, and else creates the registration. In other words, it is robust to call the method even if the user is already registered for the course. In fact, I have a unit test that does this to make sure there is no error.

@markchang

Copy link
Copy Markdown
Contributor

I just tested this on local dev and seemed to work as expected. New courses created get auto enroll.

@chrisndodge

Copy link
Copy Markdown
Contributor

Thanks! Nice work!

+1

@singingwolfboy

Copy link
Copy Markdown
Contributor

👍

cahrens pushed a commit that referenced this pull request Aug 6, 2013
Auto-enroll course staff to fix "View Live".
@cahrens
cahrens merged commit 4c95078 into master Aug 6, 2013
@cahrens
cahrens deleted the christina/autoenroll branch August 6, 2013 19:33
chrisrossi pushed a commit to jazkarta/edx-platform that referenced this pull request Mar 31, 2014
Add a wsgi file for the lms that loads the modulestores immediately afte...
itsjeyd referenced this pull request in open-craft/openedx-platform Dec 23, 2015
…tic-feedback

Adding XBlock Diagnostic-Feedback
jenkins-ks pushed a commit to nttks/edx-platform that referenced this pull request Mar 30, 2016
jenkins-ks pushed a commit to nttks/edx-platform that referenced this pull request Mar 30, 2016
caesar2164 added a commit to caesar2164/edx-platform that referenced this pull request Apr 27, 2017
…l-centering

Giulio/staff modal centering
rediris pushed a commit to gymnasium/edx-platform that referenced this pull request Feb 25, 2021
mariajgrimaldi pushed a commit to eduNEXT/edx-platform that referenced this pull request Nov 23, 2021
BC-19: upgrade eox-core to latest version
Sujeet1379 pushed a commit to chandrudev/edx-platform that referenced this pull request Nov 17, 2022
…x#578)

This adds some links to the docs for general MFE instructions,
plus documents the learning MFE specific ones.

Also fixes the README badge links.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants