-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Stanford paid course registration #1118
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.
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -58,6 +58,7 @@ | |
| import external_auth.views | ||
|
|
||
| from bulk_email.models import Optout | ||
| import shoppingcart | ||
|
|
||
| import track.views | ||
|
|
||
|
|
@@ -405,6 +406,19 @@ def change_enrollment(request): | |
|
|
||
| return HttpResponse() | ||
|
|
||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The comment should make this clear, but this code is to handle the case where AnonymousUser tries to add a course to the cart, so they must register and then |
||
| elif action == "add_to_cart": | ||
| # Pass the request handling to shoppingcart.views | ||
| # The view in shoppingcart.views performs error handling and logs different errors. But this elif clause | ||
| # is only used in the "auto-add after user reg/login" case, i.e. it's always wrapped in try_change_enrollment. | ||
| # This means there's no good way to display error messages to the user. So we log the errors and send | ||
| # the user to the shopping cart page always, where they can reasonably discern the status of their cart, | ||
| # whether things got added, etc | ||
|
|
||
| shoppingcart.views.add_course_to_cart(request, course_id) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I didn't catch this before, but I'm not sure it makes sense to push this through the views. Would it be possible to do the same sort of error handling in the models? Fat models, skinny views, etc.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not quite sure I understand the point you're objecting to here. This particular line of code is calling another view (in shoppingcart). Do you mean that it should be calling a model function instead? Or do you mean that all the clauses in shoppingcart.views.add_course_to_cart should themselves be in the model? In that case I don't think the model function should be returning HttpResponse and its relatives, which doesn't seem like right layer to me.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Or do you mean that the model function PaidCourseRegistration.add_to_order should be raising various subtypes of InvalidCartItem, and shoppingcart.views.add_course_to_cart should be handling those by translating them to HttpResponse codes? I think I'm most amenable to that, though it then doesn't change anything about this particular call. |
||
| return HttpResponse( | ||
| reverse("shoppingcart.views.show_cart") | ||
| ) | ||
|
|
||
| elif action == "unenroll": | ||
| try: | ||
| CourseEnrollment.unenroll(user, course_id) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -36,6 +36,7 @@ | |
| from xmodule.modulestore.exceptions import InvalidLocationError, ItemNotFoundError, NoPathToItem | ||
| from xmodule.modulestore.search import path_to_location | ||
| from xmodule.course_module import CourseDescriptor | ||
| import shoppingcart | ||
|
|
||
| import comment_client | ||
|
|
||
|
|
@@ -604,10 +605,27 @@ def course_about(request, course_id): | |
| show_courseware_link = (has_access(request.user, course, 'load') or | ||
| settings.MITX_FEATURES.get('ENABLE_LMS_MIGRATION')) | ||
|
|
||
| # Note: this is a flow for payment for course registration, not the Verified Certificate flow. | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This might be the best place to start review? Basically the course_about page will have an "add-to-cart" button instead of a register button. |
||
| registration_price = 0 | ||
| in_cart = False | ||
| reg_then_add_to_cart_link = "" | ||
| if settings.MITX_FEATURES.get('ENABLE_PAID_COURSE_REGISTRATION'): | ||
| registration_price = CourseMode.min_course_price_for_currency(course_id, | ||
| settings.PAID_COURSE_REGISTRATION_CURRENCY[0]) | ||
| if request.user.is_authenticated(): | ||
| cart = shoppingcart.models.Order.get_cart_for_user(request.user) | ||
| in_cart = shoppingcart.models.PaidCourseRegistration.contained_in_order(cart, course_id) | ||
|
|
||
| reg_then_add_to_cart_link = "{reg_url}?course_id={course_id}&enrollment_action=add_to_cart".format( | ||
| reg_url=reverse('register_user'), course_id=course.id) | ||
|
|
||
| return render_to_response('courseware/course_about.html', | ||
| {'course': course, | ||
| 'registered': registered, | ||
| 'course_target': course_target, | ||
| 'registration_price': registration_price, | ||
| 'in_cart': in_cart, | ||
| 'reg_then_add_to_cart_link': reg_then_add_to_cart_link, | ||
| 'show_courseware_link': show_courseware_link}) | ||
|
|
||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious, were there import problems that keep you from using the shorter versions of the names?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, there were. So I went back to the safest thing of just importing the module.