Skip to content

Credit eligibility/provider refactor - #8700

Merged
wedaly merged 1 commit into
masterfrom
will/remove-provider-course-m2m-relation
Jul 2, 2015
Merged

Credit eligibility/provider refactor#8700
wedaly merged 1 commit into
masterfrom
will/remove-provider-course-m2m-relation

Conversation

@wedaly

@wedaly wedaly commented Jun 27, 2015

Copy link
Copy Markdown
Contributor
  • Remove m2m relation between credit course and credit providers.
  • Separate eligibility and provider APIs into different modules.
  • Add API call for retrieving a user's eligibilities.
  • Cache credit course list.
  • Style the dashboard purchase button.
  • Display a link for the credit provider on the dashboard.
  • Add analytics events for clicks on the purchase button.
  • Expose more credit models to Django admin and add search functionality.

This completes the work for ECOM-1524 (credit display on the dashboard) and makes integration with the course admin tool easier (since the tool won't need to associate courses with providers on the LMS side)

@clintonb please review when you have a moment. This PR looks bigger than it actually is; I split api.py into two modules, so all of those changes are showing up in the diff.

@srpearce There are user-facing messages in this PR that need review. Screenshots below.

Screenshots

  • Eligible for credit:
    image
  • Eligible for credit, deadline is approaching:
    image
  • Purchased credit, request is pending:
    image
  • Purchased credit, request is approved:
    image
  • Purchased credit, request was rejected:
    image
  • Unexpected error occurred (in this case, something got messed up when fulfilling the user's order):
    image

"staff" is the username, and the email will be set to "support@edx.org" in production.

@wedaly
wedaly force-pushed the will/remove-provider-course-m2m-relation branch from 94e61cd to 653d21d Compare June 27, 2015 22:52

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.

Any reason these statuses aren't constants? They are used throughout the code.

@clintonb

Copy link
Copy Markdown
Contributor

Holy moly! 👍

@wedaly
wedaly force-pushed the will/remove-provider-course-m2m-relation branch from 653d21d to bc81a1e Compare July 1, 2015 15:42
@wedaly

wedaly commented Jul 1, 2015

Copy link
Copy Markdown
Contributor Author

@srpearce When you get a chance, could you please review the strings in the screenshots?

@clytwynec The Jenkins build is failing in mysterious ways after I rebased onto master. It seems to not be running a new migration to create the social auth tables that was merged recently. Has anyone else run into this? Is there a workaround?

@srpearce

srpearce commented Jul 1, 2015

Copy link
Copy Markdown
Contributor

@wedaly Working on them as we speak.

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.

Purchase Course Credit

@srpearce

srpearce commented Jul 1, 2015

Copy link
Copy Markdown
Contributor

OK, I've made some suggestions and I have a question inline.

@clytwynec

Copy link
Copy Markdown
Contributor

@wedaly I haven't heard anything for this issue. Have you tried running the tests locally? Where is the migration you are expecting to be run?

@wedaly
wedaly force-pushed the will/remove-provider-course-m2m-relation branch from bc81a1e to 280d6b7 Compare July 2, 2015 13:35
@wedaly

wedaly commented Jul 2, 2015

Copy link
Copy Markdown
Contributor Author

Further investigation into the test issue: I'm able to reproduce this locally on my branch, but not on master. I think the tables that need to be created were introduced in #8262, but I'm not sure why my branch would cause these failures. Maybe a test ordering issue?

@wedaly

wedaly commented Jul 2, 2015

Copy link
Copy Markdown
Contributor Author

OK, I think I figured out the test issue. There was a circular import that was causing syncdb to silently fail.

@wedaly
wedaly force-pushed the will/remove-provider-course-m2m-relation branch 3 times, most recently from 40f10e3 to eddfdee Compare July 2, 2015 15:53
* Remove m2m relation between credit course and credit providers.
* Separate eligibility and provider APIs into different modules.
* Add API call for retrieving a user's eligibilities.
* Cache credit course list.
* Style the dashboard purchase button.
* Display a link for the credit provider on the dashboard.
* Add analytics events for clicks on the purchase button.
* Expose more credit models to Django admin and add search functionality.
@wedaly
wedaly force-pushed the will/remove-provider-course-m2m-relation branch from eddfdee to e2acf3a Compare July 2, 2015 17:04
wedaly pushed a commit that referenced this pull request Jul 2, 2015
…ation

Credit eligibility/provider refactor
@wedaly
wedaly merged commit bfe0160 into master Jul 2, 2015
@wedaly
wedaly deleted the will/remove-provider-course-m2m-relation branch July 2, 2015 19:36
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