Skip to content

Badging: Core Refactor - #10498

Merged
Kelketek merged 19 commits into
feature/badges-v2from
badges-v2
Dec 3, 2015
Merged

Badging: Core Refactor#10498
Kelketek merged 19 commits into
feature/badges-v2from
badges-v2

Conversation

@Kelketek

@Kelketek Kelketek commented Nov 3, 2015

Copy link
Copy Markdown
Contributor

Background: This implements the a refactoring of Badging to be more flexible, creating a framework for developers to make different badges for different events and situations. Previously, a badge could only be awarded on course completion.
Discussions/Specs: https://docs.google.com/document/d/1ob-leRHV97agPGw5ys9uASXrcsU8qf51bzYlKH3mBEM/edit?usp=sharing
Jira Ticket: https://openedx.atlassian.net/browse/SOL-1362
Partner Information: 3rd party-hosted open edX instance, for an edX solutions client.
Sandbox: LMS at http://pr10556.sandbox.opencraft.com CMS at http://studio.pr10556.sandbox.opencraft.com

Deviations from spec:

As is usually the case, there are a few deviations from the spec that came up in implementation. Here are the most notable:

  1. The get_badge_class function is on BadgeClass and not on a custom manager. It did not appear to require that much hardware.
  2. A few extra fields have been added to BadgeClass and BadgeAssertion. BadgeClass now has 'mode' for backward compatibility, and a 'criteria' field, which is required by the OpenBadges spec. BadgeAssertions now have an 'assertion_url' field that points to the hosted JSON assertion.
  3. SVG support is not here-- this wasn't in the spec but in the comments. As this implementation arrived before Badgr's support for SVG landed, it is not currently supported in the LMS.

Testing instructions:

This version of edx-platform should behave identically on the outside to how it has before. The demo course is currently set up on the sandbox to make it easy to pass. Just run through the exam section and answer those questions right. You will need to re-open the course by changing its close date so that it's in the future, enroll a user, answer the questions in the exam, then set the schedule to be closed, and run the certificate generation command as root:

sudo -u edxapp /edx/bin/python.edxapp /edx/app/edxapp/edx-platform/manage.py lms --settings aws ungenerated_certs -c course-v1:edX+DemoX+Demo_Course --insecure

This will generate a certificate for your user, which you should then be able to see in the dashboard. Click view for the certificate, and attempt to download the badge.

@openedx-webhooks

Copy link
Copy Markdown

Thanks for the pull request, @Kelketek! It looks like you're a member of a company that does contract work for edX. If you're doing this work as part of a paid contract with edX, you should talk to edX about who will review this pull request. If this work is not part of a paid contract with edX, then you should ensure that there is an OSPR issue to track this work in JIRA, so that we don't lose track of your pull request.

To automatically create an OSPR issue for this pull request, just visit this link: https://openedx-webhooks.herokuapp.com/github/process_pr?repo=edx%2Fedx-platform&number=10498

@Kelketek
Kelketek force-pushed the badges-v2 branch 8 times, most recently from f121c2d to d359af6 Compare November 6, 2015 03:15
@Kelketek

Kelketek commented Nov 6, 2015

Copy link
Copy Markdown
Contributor Author

@e-kolpakov This isn't done yet-- I need to write more tests and create a publicly available sandbox with the changes and Badgr. But could you take a look at the code so far?

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.

@Kelketek there are lots of changes in this file, and some of those does not seem related to badges/certificates. Might make sense to take another look if those changes are needed.

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.

Ah, I removed the circuit djangoapp from LMS in #10324.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@e-kolpakov @stvstnfrd The tests will not run without the schema/data update. This is because models are removed, but have data in them within the fixtures. I tried just changing the data and putting the schema back into place, but this did not work on its own.

@Kelketek

Kelketek commented Nov 6, 2015

Copy link
Copy Markdown
Contributor Author

@e-kolpakov @antoviaque I'd like to wait on making a sandbox for this until this initial PR is merged in. That way, I can open the feature branch PR against master, and use that generated sandbox for demonstration, since it will have to undergo functional review anyway. It takes a good long time to set up the badging environment and I'd rather not spend double the time for something that can be done once.

This PR should change nothing on the front-end, and would require a good bit of fiddling just to see work on the back.

It is ready for review, however. I'm updating the description now.

@antoviaque

Copy link
Copy Markdown
Contributor

@Kelketek I would suggest opening the PR from the feature branch to master now, this way you'll get a sandbox from that PR, and you can list the remaining TODOs (a bit like for the content libraries PR for example: https://github.com/edx/edx-platform/pull/6459 ). The principle for reviews of feature branches is that reviews still happen on individual PRs - once they are all merged into the feature branch, the feature branch PR isn't reviewed again. So to get the reviews and functional testing, reviewers will need a sandbox now - but if you create the feature branch PR now, you can simply progressively update its sandbox to include new work. Note that if several PRs end up being reviewed simultaneously, you won't be able to escape setting up multiple sandboxes - you'll need one per review in any case.

@Kelketek

Kelketek commented Nov 9, 2015

Copy link
Copy Markdown
Contributor Author

@antoviaque I'll start working on that, then. It won't be done before our sprint is over, mind. But I'll do what I can.

@Kelketek

Copy link
Copy Markdown
Contributor Author

@e-kolpakov This should be ready for you, now.

@e-kolpakov

Copy link
Copy Markdown
Contributor

@Kelketek 👍 conditional tests pass. Also, it would be great if you could take another look at bok_choy_schema.sql - I still think there are some unwanted/unintentional changes there (i.e. adding course_groups_cohortmembership and friends)

@Kelketek Kelketek changed the title (WIP) Badging V2 Badging: Core Refactor Nov 12, 2015
@Kelketek

Copy link
Copy Markdown
Contributor Author

@nedbat This is ready for your review.

@Kelketek Kelketek mentioned this pull request Nov 25, 2015
Comment thread lms/djangoapps/badges/backends/badgr.py Outdated

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.

This could use a nicer error message, perhaps with an "if" instead of an "assert". Also, there are other settings.BADGR_* settings here, why do we only check for one?

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.

You should call the super __init__.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@nedbat The other settings have defaults. Agreed-- will make this raise ImproperlyConfigured with an error message.

@Kelketek

Kelketek commented Dec 2, 2015

Copy link
Copy Markdown
Contributor Author

@nedbat Same question for this branch. Is there anything else you need from me in order to give this a +1?

@nedbat

nedbat commented Dec 3, 2015

Copy link
Copy Markdown
Contributor

👍

@Kelketek
Kelketek merged this pull request into feature/badges-v2 Dec 3, 2015
@Kelketek
Kelketek deleted the badges-v2 branch December 3, 2015 16:58
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.

9 participants