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.

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

Added admin-only bulk enrollment form #178

Merged
merged 1 commit into from
Apr 29, 2019
Merged

Conversation

gsidebo
Copy link
Contributor

@gsidebo gsidebo commented Apr 24, 2019

Pre-Flight checklist

  • Screenshots and design review for any changes that affect layout or styling
  • Migrations
    • Migration is backwards-compatible with current production code
  • Testing
    • Code is tested
    • Changes have been manually tested

What are the relevant tickets?

Closes #112

What's this PR do?

  • Adds the bulk enrollment form for admin use
  • Fixes the email debugger page
  • Adds a very basic listing page for ecommerce admin pages (/ecommerce/admin/)

How should this be manually tested?

  • Create some 100% off coupons. It involves a lot of different objects, so here's a gist to help with that if you want it.
  • Create a CSV file with one email address per line. Ensure that those email addresses will end up in your mailbox, e.g.: myactualemail+1@mit.edu, myactualemail+2@mit.edu, ...
  • Navigate to /ecommerce/admin/enroll/, select your CSV file, choose one of your coupons in the dropdown, and click the enroll button.

Where should the reviewer start?

Probably static/js/containers/pages/admin/BulkEnrollmentPage.js

Any background context you want to provide?

  • The wireframe calls for a separate dropdown for program/course/course run, but a CouponEligibility object is all we need to map a coupon to those objects. We didn't need the separate dropdown
  • I included a dummy url/view for handling the URL that a user is sent to when they click the "Enroll" button in the email. Actual enrollment handling will be done in a different issue
  • I made some minor refactoring changes to the mail API. It was not very friendly for sending a message to a set of recipients with the same context in each message

Screenshots (if appropriate)

ss 2019-04-26 at 17 13 12
ss 2019-04-26 at 17 13 19
ss 2019-04-26 at 17 13 28

ss 2019-04-24 at 11 26 46
ss 2019-04-23 at 12 57 13
ss 2019-04-23 at 12 57 23

@gsidebo
Copy link
Contributor Author

gsidebo commented Apr 24, 2019

@pdpinch Screenshots ^^^

@codecov-io
Copy link

codecov-io commented Apr 24, 2019

Codecov Report

Merging #178 into master will increase coverage by 0.59%.
The diff coverage is 93.05%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #178      +/-   ##
==========================================
+ Coverage   92.69%   93.28%   +0.59%     
==========================================
  Files         134      142       +8     
  Lines        3845     4143     +298     
  Branches      222      232      +10     
==========================================
+ Hits         3564     3865     +301     
+ Misses        240      235       -5     
- Partials       41       43       +2
Impacted Files Coverage Δ
static/js/lib/urls.js 100% <ø> (ø) ⬆️
mitxpro/urls.py 80% <ø> (ø) ⬆️
ecommerce/urls.py 100% <ø> (ø) ⬆️
ecommerce/api.py 99.25% <100%> (+0.04%) ⬆️
.../containers/pages/admin/BulkEnrollmentPage_test.js 100% <100%> (ø)
mail/constants.py 100% <100%> (ø)
static/js/constants.js 100% <100%> (ø) ⬆️
static/js/containers/pages/CheckoutPage_test.js 100% <100%> (ø) ⬆️
mail/forms.py 100% <100%> (+100%) ⬆️
mitxpro/test_utils.py 100% <100%> (ø) ⬆️
... and 33 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 601276d...9ba33e0. Read the comment docs.

@mbertrand mbertrand self-assigned this Apr 24, 2019
Copy link
Member

@mbertrand mbertrand left a comment

Choose a reason for hiding this comment

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

I think this is meant to be used for Coupons related to CouponPaymentVersion objects with coupon_type='single-use'. There should be n number of these Coupon objects equal to CouponPaymentVersion.num_coupon_codes, each with its own unique code. Each user should get sent a different coupon code, no two users should get the same code. So there will also need to be a check that there are at least as many coupon codes as there are emails in the csv file.

There is a form you can use to create these coupons here: /ecommerce/admin/coupons

@mbertrand
Copy link
Member

Also heads up that there may be some changes to what's allowed to be a product (maybe just Program and CourseRun, instead of Program and Course - formerly it was all 3), will be determined tomorrow.

@mbertrand
Copy link
Member

Might be good to also track which coupon is sent to which user. I've got a DataConsentUser model in PR #163 that includes a foreign key to Coupon, maybe that could be split up into two models, DataConsentUser and CouponUser? Or just renamed to something more generic.

@Ferdi
Copy link

Ferdi commented Apr 24, 2019

Might be good to also track which coupon is sent to which user.
I think that would be useful /cc @rhysyngsun

@gsidebo
Copy link
Contributor Author

gsidebo commented Apr 24, 2019

@mbertrand Sounds good. Tracking the sending of coupons to users should be easy enough. I'll follow up with you about sending each user a different code

@gsidebo
Copy link
Contributor Author

gsidebo commented Apr 25, 2019

@mbertrand Ready for another look

ecommerce/api.py Outdated
.filter(coupon__enabled=True, coupon__payment=coupon_payment)
.distinct("product")
)
if product_coupons.count() != 1:
Copy link
Member

@mbertrand mbertrand Apr 25, 2019

Choose a reason for hiding this comment

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

It's possible to have multiple CouponEligibility objects for the same coupon. I made my 100% off coupon apply to 3 products so I always get the message "In order to perform bulk enrollment, there must be 100%-off coupons applied to products. None were found."

Copy link
Member

Choose a reason for hiding this comment

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

Never mind, if a coupon has more than 1 product it shouldn't be listed

return Response(
status=status.HTTP_400_BAD_REQUEST,
data={
"errors": [
Copy link
Member

Choose a reason for hiding this comment

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

This error is correctly returned when the csv file has more emails than there are coupons, but is not displayed on the page.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, forgot about this one. Will fix


send_bulk_enroll_emails(emails, product_coupons)

return Response(
Copy link
Member

Choose a reason for hiding this comment

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

Response is returned but nothing on the page indicates success

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmm, looks like I messed up the success message after my most recent commit. Will fix

ecommerce/api.py Outdated
)
if product_coupons.count() != 1:
continue
yield coupon_payment, product_coupons[0].product
Copy link
Member

Choose a reason for hiding this comment

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

The dropdown list only shows one of the products that a coupon is valid for, there may be more than one. I made a coupon valid for 3 products, the dropdown showed one of those products but then different emails got sent different products that the coupon applied to:

Screen Shot 2019-04-25 at 8 08 16 AM

Screen Shot 2019-04-25 at 8 18 02 AM

Screen Shot 2019-04-25 at 8 03 52 AM

Screen Shot 2019-04-25 at 8 17 54 AM

This should probably be changed to show the coupon for every product that it is applicable to, and then emails should get sent for that particular product to all user emails if selected:

for product_coupon in product_coupons:
    yield coupon_payment, product_coupon.product

Though maybe no one will ever create a 100% off coupon that applies to multiple products (nothing on the coupon creation form prevents you from doing so however) - @pdpinch what do you think?

Copy link
Member

@mbertrand mbertrand Apr 25, 2019

Choose a reason for hiding this comment

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

PS this is after I removed the if product_coupons.count() != 1 condition to see what would happen.

Copy link
Member

Choose a reason for hiding this comment

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

Our primary use case is where bulk enrollment apply to one product (program, course, or courserun). Can we limit the form to only coupons of that type?

Copy link
Member

Choose a reason for hiding this comment

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

Limiting the bulk enrollment to one product also will simplify the mailing: just one one email for each email address uploaded.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, it already is here, so good as is @gsidebo!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I meant to be more explicit about that in the message and in the code comments. I'll add some clarification

@gsidebo
Copy link
Contributor Author

gsidebo commented Apr 26, 2019

@mbertrand Ready for you again. I'll work on rebasing

@gsidebo
Copy link
Contributor Author

gsidebo commented Apr 26, 2019

@Ferdi @pdpinch Screenshots updated ^^^

ecommerce/views.py Outdated Show resolved Hide resolved
@gsidebo
Copy link
Contributor Author

gsidebo commented Apr 29, 2019

@mbertrand I ended up just solving #188 and preventing repeated invites for the same product coupons in one shot. That's what made the most sense. Give it another try. You should see and error when num_coupon_codes is less than the # of emails, and when the available unsent product coupons are less than the # of emails.

I'll rebase after your review so I can squash the commits first

Copy link
Member

@mbertrand mbertrand left a comment

Choose a reason for hiding this comment

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

👍 works great

@gsidebo gsidebo merged commit f5143e2 into master Apr 29, 2019
@gsidebo gsidebo deleted the 112_bulk_enrollments_ui branch April 29, 2019 17:45
@gsidebo gsidebo mentioned this pull request Apr 29, 2019
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bulk Enrollments
6 participants