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

Support team UI for regenerating certificates #9240

Merged
merged 1 commit into from
Aug 12, 2015
Merged

Conversation

wedaly
Copy link
Contributor

@wedaly wedaly commented Aug 7, 2015

Allow support staff to view and regenerate certificates.

  • Add new role for support staff.
  • Move dashboard/support functionality into a new Django app called "support".
  • Add support view for searching and regenerating certificates.
  • Refactor certificates views into separate files.

JIRA: ECOM-1781

Screenshots

  • Support index page:
    image
  • Certificates page
    image
  • After a search
    image

Reviewers

Apologies in advance for the large PR! I couldn't think of a reasonable way to break these changes into smaller PRs, so I'm going to assign people to review particular parts of this PR.

@AlasdairSwan could you please review the JavaScript and underscore templates in the support app?
@rlucioni could you please review the Python changes in the certificates app?

FYI: @mattdrayer I refactored the certificates views into separate files, since the views.py file was getting pretty crowded. I'm pretty sure this UI will work for webview certificates, with the exception of the "download URL" in the search results. I'm going to set up a sandbox to test this with webview certs before merging.

@@ -0,0 +1,11 @@
;(function (define) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@AlasdairSwan These are the files I need you to review. Hooray for RequireJS optimizer in the LMS!

@AlasdairSwan
Copy link
Contributor

Looks good 👍

@wedaly
Copy link
Contributor Author

wedaly commented Aug 10, 2015

Tested this on a sandbox for both XQueue and webview certificates. Everything seems to be working as expected.

@rlucioni when you have a chance, could you please review the Python changes?

return response

# Check that the course exists
course = modulestore().get_course(params["course_key"])
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you use the CourseOverview cache here instead of hitting the modulestore directly?

* Add new role for support staff.
* Move dashboard/support functionality into a new Django app called "support".
* Add support view for searching and regenerating certificates.
* Refactor certificates views into separate files.
@wedaly
Copy link
Contributor Author

wedaly commented Aug 12, 2015

@rlucioni Unfortunately, the course gets passed down to grades.grade() which complains when it uses a course loaded from the CourseOverview cache:

 AttributeError: 'CourseOverview' object has no attribute 'block_types_affecting_grading'

@rlucioni
Copy link
Contributor

@wedaly that's likely because the CourseOverview object being passed down doesn't cache the block_types_affecting_grading attribute. Updating CourseOverview to cache that attribute should be easy: you would add to the CourseOverview model's fields, then make sure that field is populated when a new CourseOverview is instantiated.

With that said, I can see how this would be outside the scope of your work here. If you choose not to address it now, it might be worth adding a ticket to the backlog for. Whichever you choose, 👍 .

wedaly pushed a commit that referenced this pull request Aug 12, 2015
Support team UI for regenerating certificates
@wedaly wedaly merged commit 5b9b0a8 into master Aug 12, 2015
@wedaly wedaly deleted the will/ecom-1781-rebase branch August 12, 2015 16:09
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.

3 participants