Skip to content

Conversation

@Kelketek
Copy link
Contributor

@Kelketek Kelketek commented Nov 9, 2015

Description: This PR is the next iteration of the Badging feature in the LMS
Discussions/Specs: https://docs.google.com/document/d/1ob-leRHV97agPGw5ys9uASXrcsU8qf51bzYlKH3mBEM/edit?usp=sharing
Jira Ticket: https://openedx.atlassian.net/browse/SOL-1325
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

Components:


Settings

# Use configuration branch that enables swap on the instance to avoid ooming.
edx_ansible_source_repo: 'https://github.com/open-craft/configuration.git'
configuration_version: 'rebased-optional-swap-role'

@openedx-webhooks
Copy link

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=10556

@Kelketek Kelketek force-pushed the feature/badges-v2 branch 3 times, most recently from 419f77e to 1817d9b Compare November 16, 2015 16:11
@Kelketek Kelketek force-pushed the feature/badges-v2 branch 2 times, most recently from 0563546 to 1b618aa Compare November 20, 2015 16:37
@Kelketek Kelketek force-pushed the feature/badges-v2 branch 2 times, most recently from 98f1635 to 1b618aa Compare December 3, 2015 17:21
@stroilova
Copy link
Contributor

@Kelketek Kelketek force-pushed the feature/badges-v2 branch 4 times, most recently from cf5ab04 to 3758dac Compare December 21, 2015 20:51
@Kelketek
Copy link
Contributor Author

@antoviaque @bradenmacdonald @e-kolpakov @smagoun The last PR looks like it will merge soon. Am I clear to merge this feature branch if it does, or does anything else need to be done?

Copy link
Contributor

Choose a reason for hiding this comment

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

@Kelketek image_url appears twice

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@catong The first image_url is the URL to the badge class's source image. The second one is the baked badge assertion image generated by Badgr, with all the metadata concerning the user's award.

@Kelketek Kelketek force-pushed the feature/badges-v2 branch 2 times, most recently from 9a7c2cf to d48e093 Compare December 22, 2015 19:53
@Kelketek Kelketek changed the title (WIP) Badges V2 Feature Branch Badges V2 Feature Branch Dec 30, 2015
@Kelketek
Copy link
Contributor Author

@antoviaque @bradenmacdonald @smagoun @pbaruah

This is ready to merge, assuming the tests pass.

@Kelketek Kelketek force-pushed the feature/badges-v2 branch 5 times, most recently from d675798 to 65ef341 Compare March 17, 2016 23:33
assertion.save()
# Would be overwritten by the first save.
assertion.created = datetime.fromtimestamp(
time.mktime(time.strptime(badge.data['created_at'], "%Y-%m-%dT%H:%M:%S"))
Copy link
Contributor

Choose a reason for hiding this comment

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

@Kelketek I get an error when I run this:

*** ValueError: unconverted data remains: .979502Z

It works if I truncate the date string:

badge.data['created_at'][:19]

Also, this is a shorter way of writing this:

assertion.created = datetime.strptime(badge.data['created_at'][:19], "%Y-%m-%dT%H:%M:%S")

@Kelketek Kelketek force-pushed the feature/badges-v2 branch from 65ef341 to 18e9daa Compare March 22, 2016 16:45
@Kelketek
Copy link
Contributor Author

jenkins run bokchoy

@Kelketek
Copy link
Contributor Author

@omarkhan I've been instructed to ignore the coveralls test for now-- could you take a look over the latest changes? If they're good I'll put them up on the sandbox.

@Kelketek
Copy link
Contributor Author

Actually, went ahead and updated it now.

@andy-armstrong
Copy link
Contributor

👍 Great stuff, @Kelketek. Don't forget to squash and fix the quality failure, then merge away!

@Kelketek
Copy link
Contributor Author

Kelketek commented Apr 1, 2016

@catong I will be cleaning this up, squashing, and merging tomorrow morning. So you'll want to get the docs in order :)

@Kelketek Kelketek force-pushed the feature/badges-v2 branch 3 times, most recently from 7475cce to 27957a6 Compare April 1, 2016 20:49
@mattdrayer
Copy link
Contributor

@Kelketek these most recent two commits LGTM -- 👍

@Kelketek Kelketek force-pushed the feature/badges-v2 branch from 27957a6 to f84f95c Compare April 1, 2016 21:06
@Kelketek
Copy link
Contributor Author

Kelketek commented Apr 1, 2016

@nedbat Disregard previous statement about email-- a recent tweak to the migration should remove the need for any special action.

@Kelketek Kelketek merged commit ab0cbff into master Apr 1, 2016
@Kelketek Kelketek deleted the feature/badges-v2 branch April 1, 2016 22:06
@bradenmacdonald
Copy link
Contributor

@catong In the end this merged after the release cut, so we are targeting next week's release for docs etc.

@catong
Copy link
Contributor

catong commented Apr 4, 2016

@bradenmacdonald Sounds good. Thanks!

@bradenmacdonald
Copy link
Contributor

@edx/devops Heads up that if you're cutting a release tomorrow it will include this PR for new badging features, which has migrations.

Much of the new functionality is behind a feature flag though, and won't be enabled on edx.org.

@feanil
Copy link
Contributor

feanil commented Apr 13, 2016

@mattdrayer @Kelketek @bradenmacdonald this change had a migration that deleted a column. This is not backwards compatible with the running software and resulted in errors during the deploy today. The model change and the migration need to be in separate deploys to not break users in production.

@bradenmacdonald
Copy link
Contributor

@feanil Sorry about that. As you can see above I did ping devops about the migration, although I was not aware of the potential issues around deleting columns in particular. Have you made a fix for this or is further action required?

@feanil
Copy link
Contributor

feanil commented Apr 13, 2016

At this point I think we can push it through, but I mostly wanted to leave a note to say do not do this on a table that is more highly trafficked in the future.

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.