Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Initial badge system implementation #2115

Merged
merged 1 commit into from

5 participants

@vikhyat

Sorry about the huge diff, will be sure to keep it smaller in the future.

This implements:

  • Backend models and stuff
  • API for granting / revoking badges
  • Admin UI for creating badges
@discoursebot

You've signed the CLA, vikhyat. Thank you! This pull request is ready for review.

@riking

The travis error is that JSHint failed. @nschonni is there any way to tell travis that JSHint failing should be a fail, not an error?

adminjs/routes/admin_badges_route.js: line 7, col 46, 'model' is defined but never used.

app/assets/javascripts/admin/routes/admin_badges_route.js: line 7, col 46, 'model' is defined but never used.

@nschonni

If the linting is moved to the script section then it will mark it as a failure rather than an error. I put it into the before_install to act as a fast fail since the gem install process is rather long.

@vikhyat

@SamSaffron I had not null constraints on those fields originally but they were causing the shoulda tests to fail... Should I get rid of the shoulda tests then?

@SamSaffron
Owner
@vikhyat

This was the error: thoughtbot/shoulda-matchers#194

Though it looks like I misunderstood the cause of the error, should be able to resolve by creating a badge using the fabricator before each test.

@vikhyat

Just made the following changes:

  • Added JSDoc comments.
  • Added tests for the AdminBadgesController.
  • Made .references explicit.
  • Added NOT NULL constraints.
  • Added "experimental" text to the site setting description.
@vikhyat

Pushed up all of the changes mentioned. I haven't squashed it into the previous commit right now since it would erase all of the previous line comments, will squash when this is ready to be merged in.

@vikhyat

Forgot to get rid of the part where we set selected on the Badge model, will fix tomorrow.

@vikhyat

I merged the two badge revoke specs, got rid of the duplicate index and the index on granted_by_id, and changed it to use an itemController instead of setting selected on the model. Also squashed everything into a single commit.

@SamSaffron
Owner

Thanks heaps, lets merge this and start playing with it, smaller PRs next time :)

@SamSaffron SamSaffron merged commit fe63db7 into discourse:master
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Mar 14, 2014
  1. @vikhyat
Something went wrong with that request. Please try again.