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

[bug 916813][bug 916818] Implement KB and L10n automatic badges. #1645

Closed
wants to merge 1 commit into from

Conversation

rlr
Copy link
Contributor

@rlr rlr commented Sep 24, 2013

This is ready for a proper r? now.

r?

(@willkg, everybody)

from kitsune.wiki.models import Revision


WIKI_BADGES = {
Copy link
Member

Choose a reason for hiding this comment

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

I'd add a comment here saying something like this:

Yo HOMIES! These are year-agnostic badge templates which code uses to get-or-create the actual Badge instances. These strings should not be l10n-ized here--the badge title and description strings get l10n-ized elsewhere. Peace!

@willkg
Copy link
Member

willkg commented Sep 24, 2013

Other than the comment and the nit about the receiver signature, this looks good to me.

However, shouldn't land this until after we land the change for bug #920099. I'm working on that now.

@rlr
Copy link
Contributor Author

rlr commented Sep 24, 2013

Feedbacked and stuff ^

I'm going to leave this here and do the army of awesome badge badge branch off of this one.

Then finally I'll do a migration/script/whatever and land it all together once we have images.

from badger.models import Badge


def get_or_create_badge(slug, title, description):
Copy link
Member

Choose a reason for hiding this comment

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

What about badge image? Do we plan to manually upload the badge image every time? If so, then we have some recurring maintenance costs at the beginning of the year (which is fine by me).

Also, NURSE KITTY!

Copy link
Member

Choose a reason for hiding this comment

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

Actually, what I'd do here is:

def get_or_create_badge(slug, **kwargs):
   try:
       blah blah blah
   excpet Badge.HomieNotHome:
       return Badge.objects.create(slug=slug, **kwargs)

Copy link
Member

Choose a reason for hiding this comment

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

And by "here", I really mean "down 9 lines".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

neato

@rlr
Copy link
Contributor Author

rlr commented Sep 24, 2013

I rebased and squashed ^. Ready to merge when all the other pieces are ready.

rlr referenced this pull request in rlr/kitsune Sep 24, 2013
@willkg
Copy link
Member

willkg commented Sep 24, 2013

I landed my stuff. This looks fine to me. r+

At some point, we might want to pull some of the redundancy into kbadge and/or switch to progress badges that track progress for us. But... this is fine for now as we feel our way around.

@rlr
Copy link
Contributor Author

rlr commented Sep 30, 2013

2f250fa

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.

None yet

2 participants