Skip to content
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

Add donation button to the enrollment success message #5502

Merged
merged 1 commit into from
Oct 8, 2014
Merged

Add donation button to the enrollment success message #5502

merged 1 commit into from
Oct 8, 2014

Conversation

wedaly
Copy link
Contributor

@wedaly wedaly commented Oct 7, 2014

Changes:

  • Add donation end-point
  • Make donations configurable
  • Added donation button to dashboard
  • Generalize merchant defined data for payment processor

Notes:

  • To enable this, you need to use model-based configuration. Enable DonationConfiguration and DashboardConfiguration (enrollment message timeout) in Django admin.
  • This PR includes client-side validation and introduces a new client-side analytics event edx.bi.user.payment_processor.visited fired when the user clicks the "donate" button, right before we send them to the external payment processor.
  • This PR does NOT include any style changes.

Reviewers: @dianakhuang @stephensanchez @AlasdairSwan
FYI: @andy-armstrong @chrisndodge

ECOM-445

@wedaly wedaly force-pushed the will/per-course-donation-button branch from 6442e87 to bbaab59 Compare October 7, 2014 12:10
chrisndodge
chrisndodge reviewed Oct 7, 2014
View reviewed changes
lms/envs/common.py
@@ -1008,7 +1008,7 @@
'js/vendor/ova/video.dev.js',
'js/vendor/ova/vjs.youtube.js',
'js/vendor/ova/rangeslider.js',
'js/vendor/ova/share-annotator.js',
'js/vendor/ova/share-annotator.js'
Copy link
Contributor

Choose a reason for hiding this comment

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

missing comma at end?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! Fixed.

@wedaly wedaly force-pushed the will/per-course-donation-button branch from bbaab59 to 404f255 Compare October 7, 2014 12:13
@chrisndodge
Copy link
Contributor

@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.

@wedaly
Copy link
Contributor Author

wedaly commented Oct 7, 2014

Couple of things I'm noticing while testing in a sandbox.

  1. The new JS doesn't seem to be getting minified, despite going through the Django asset pipeline. @andy-armstrong is there something else I need to do to make this happen?

  2. The client-side validation allows "0.00" as a valid donation amount, and the back-end has no problem sending that to CyberSource. I'm assuming we want to set a minimum amount and include client/server side validation. I'll coordinate with Griff to figure out what that amount should be.

@chrisndodge
Copy link
Contributor

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.

@wedaly wedaly force-pushed the will/per-course-donation-button branch 2 times, most recently from 7071835 to 146ae7f Compare October 7, 2014 12:57
stephensanchez
stephensanchez reviewed Oct 7, 2014
View reviewed changes
lms/djangoapps/shoppingcart/tests/test_models.py
@@ -612,6 +607,11 @@ def test_confirmation_email(self):
self.assertEquals('Order Payment Confirmation', email.subject)
self.assertIn("tax deductible", email.body)

def test_donate_no_such_course(self):
fake_course_id = SlashSeparatedCourseKey("edx", "fake", "course")
Copy link
Contributor

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)

Copy link
Contributor

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.

@stephensanchez
Copy link
Contributor

👍 looks really good.

@wedaly wedaly force-pushed the will/per-course-donation-button branch from 146ae7f to dba4652 Compare October 7, 2014 14:05
@wedaly
Copy link
Contributor Author

wedaly commented Oct 7, 2014

According to Griff and Manali, any amount > 0.00 is okay. Added client- and server-side validation for this edge case.

andy-armstrong
andy-armstrong reviewed Oct 7, 2014
View reviewed changes
lms/static/js/dashboard/donation.js
@@ -0,0 +1,245 @@
var edx = edx || {};
Copy link
Contributor

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?

Copy link

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.

@andy-armstrong
Copy link
Contributor

I'm not a reviewer, but looks great to me. I'm really happy to see the excellent Jasmine tests.

@wedaly
Copy link
Contributor Author

wedaly commented Oct 7, 2014

@andy-armstrong Thanks for the review. Updated with another commit to address your feedback, and I'll squash before merging.

@wedaly wedaly force-pushed the will/per-course-donation-button branch 2 times, most recently from d6b97a5 to fc30245 Compare October 7, 2014 17:37
@wedaly
Copy link
Contributor Author

wedaly commented Oct 7, 2014

Rebased to get the enrollment messaging style changes @stephensanchez just merged.

Add donation end-point
f8365a2 
Make donations configurable

Added donation button to dashboard

Generalize merchant defined data for payment processor
@wedaly wedaly force-pushed the will/per-course-donation-button branch from fc30245 to f8365a2 Compare October 7, 2014 18:23
@wedaly
Copy link
Contributor Author

wedaly commented Oct 7, 2014

Updated to replace deprecated SlashSeparatedCourseKey in shoppingcart model tests.

@dianakhuang
Copy link
Contributor

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.

@dianakhuang
Copy link
Contributor

Otherwise, the code looks reasonable.

@stephensanchez
Copy link
Contributor

I believe the AB testing got removed from the requirements.

On Tue, Oct 7, 2014 at 3:09 PM, Diana Huang notifications@github.com
wrote:

Otherwise, the code looks reasonable.

—
Reply to this email directly or view it on GitHub
https://github.com/edx/edx-platform/pull/5502#issuecomment-58242231.

@wedaly
Copy link
Contributor Author

wedaly commented Oct 8, 2014

Thanks everyone for the quick reviews. @dianakhuang We're not going to A/B test this feature.

wedaly pushed a commit that referenced this pull request Oct 8, 2014
Merge pull request #5502 from edx/will/per-course-donation-button
c4eee8e 
Add donation button to the enrollment success message
@wedaly wedaly merged commit c4eee8e into master Oct 8, 2014
@wedaly wedaly deleted the will/per-course-donation-button branch October 8, 2014 10:05
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.
6 participants
@wedaly @chrisndodge @stephensanchez @andy-armstrong @dianakhuang @cahrens