-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Add donation button to the enrollment success message #5502
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
Conversation
6442e87 to
bbaab59
Compare
lms/envs/common.py
Outdated
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.
missing comma at end?
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.
Good catch! Fixed.
bbaab59 to
404f255
Compare
|
@wedaly thanks for the tag. After this gets merged in, it'll likely cause some merge conflicts in the shoppingcart djangoapp regarding what @asadiqbal08 @muhhshoaib are working on (cdodge/shopping-cart-rewrite). So they might need help resolving conflicts later on in this week. |
|
Couple of things I'm noticing while testing in a sandbox.
|
|
We do trap 0.00 value shopping carts in the views, but - yes - as you indicate, once it gets to CyberSource processors, there's no enforcement for a minimum value. |
7071835 to
146ae7f
Compare
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 fyi you may be able to use locator.CourseLocator() instead of SlashSeparatedCourseKey (which I think is deprecated)
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.
Yup, SlashSeparatedCourseKey is deprecated, and I think it makes sense to fix it here.
|
👍 looks really good. |
146ae7f to
dba4652
Compare
|
According to Griff and Manali, any amount > 0.00 is okay. Added client- and server-side validation for this edge case. |
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.
I like this convention in the absence of RequireJS. I think we'll adopt this on T&L too. @cahrens, what do you think?
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.
Yes, I like that as well.
|
I'm not a reviewer, but looks great to me. I'm really happy to see the excellent Jasmine tests. |
|
@andy-armstrong Thanks for the review. Updated with another commit to address your feedback, and I'll squash before merging. |
d6b97a5 to
fc30245
Compare
|
Rebased to get the enrollment messaging style changes @stephensanchez just merged. |
Make donations configurable Added donation button to dashboard Generalize merchant defined data for payment processor
fc30245 to
f8365a2
Compare
|
Updated to replace deprecated |
|
Question: are we expected to A/B test this at all? It seems a little weird to do that, but I figure it's worth asking. |
|
Otherwise, the code looks reasonable. |
|
I believe the AB testing got removed from the requirements. On Tue, Oct 7, 2014 at 3:09 PM, Diana Huang notifications@github.com
|
|
Thanks everyone for the quick reviews. @dianakhuang We're not going to A/B test this feature. |
Add donation button to the enrollment success message
Changes:
Notes:
DonationConfigurationandDashboardConfiguration(enrollment message timeout) in Django admin.edx.bi.user.payment_processor.visitedfired when the user clicks the "donate" button, right before we send them to the external payment processor.Reviewers: @dianakhuang @stephensanchez @AlasdairSwan
FYI: @andy-armstrong @chrisndodge
ECOM-445