Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Initial badge system implementation #2115

Merged
merged 1 commit into from Mar 16, 2014

Conversation

5 participants
Contributor

vikhyat commented Mar 12, 2014

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

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

Contributor

riking commented Mar 12, 2014

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.

Contributor

nschonni commented Mar 12, 2014

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.

Contributor

vikhyat commented Mar 13, 2014

@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?

Owner

SamSaffron commented Mar 13, 2014

what shoulda tests? that sounds like crazy talk

On Thu, Mar 13, 2014 at 9:21 PM, Vikhyat Korrapati <notifications@github.com

wrote:

@SamSaffron https://github.com/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?

Reply to this email directly or view it on GitHubhttps://github.com/discourse/discourse/pull/2115#issuecomment-37517681
.

Contributor

vikhyat commented Mar 13, 2014

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.

Contributor

vikhyat commented Mar 13, 2014

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.
Contributor

vikhyat commented Mar 13, 2014

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.

Contributor

vikhyat commented Mar 13, 2014

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

Contributor

vikhyat commented Mar 14, 2014

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.

Owner

SamSaffron commented Mar 16, 2014

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

@SamSaffron SamSaffron added a commit that referenced this pull request Mar 16, 2014

@SamSaffron SamSaffron Merge pull request #2115 from vikhyat/badge-system
Initial badge system implementation
fe63db7

@SamSaffron SamSaffron merged commit fe63db7 into discourse:master Mar 16, 2014

1 check passed

default The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment