I haven't looked at any of the code yet, but you can close /r/4738 and /r/4668 as "Discarded" now.

Because all your view does is render a template, you can currently use the built-in template view:

from django.conf.urls.defaults import patterns, url
from django.views.generic import TemplateView

urlpatterns = patterns(
    '',

   url(r'^$', TemplateView.as_view(template_name="configure.html")),
)


You can then get rid of your views.py entirely.

Why do you need this?

There's a bunch of cleanup you can do here:


In order to properly handle local sites, you should use

  ReviewRequestResource.get_object to fetch the ReviewRequest.

  This means you'll have to import that resource.

ReviewRequestResource.get_object takes in review_request_id

  and local_site_name parameters, which are helpfully already

  in your kwargs. You can leave them in there and just pass the

  whole kwargs to get_object().

The user_id field can be a parameter to your function.
I'd rename 'this_user' and 'this_request' to be 'user' and 'review_request', respectively.

The new code might look something like this:
```python
... way up top:

from reviewboard.webapi.resources import resources
... and down here:
def create(self, request, user_id=None, args, kwargs):

    user = User.objects.get(pk=user_id)

    review_request = resources.review_request.get_object(

        request, args, **kwargs)

Django provides a nice facility for this:

checklist, _ = ReviewChecklist.objects.get_or_create(
    user=this_user,
    review_request=this_request)

Left-over debug code?

Similar comments with how to fetch and name user and review_request as the create method above.

Instead of pulling these from kwargs, you can define them in your method signature as optional parameters and then check against None:

def update(self, request, checklist_id,
           checklist_item_id=None, item_description=None):
    ...

    if (checklist_item_id is None and
            item_description is not None):
        checklist.add_item(item_description)


Similar for the other things below that you use kwargs for.

Left-over debug code?

This should probably check that request.user is the same as the checklist user. Right now this allows anyone to delete anyone else's checklist.

There should be a blank line between these, since 'checklist' is the local module.

Instead of having these in the docstring, perhaps define them as help_text on each of the fields?

Trailing whitespace.

Because your attributes have the same name on the client side as on the server, this should be pretty trivial:
```javascript

toJSON: function() {

    return this.attributes;

},

````

This should use === instead of ==

I'm not sure why you have the cehcklist_id field when 'id' will be the same.

This looks like left-over debug code?

Left-over debug code?

Could you just connect the 'change' event on the model to this.render()?

Left-over debug code?

=== null

Left-over debug code?

Left-over debug code?

Left-over debug code?

Left-over debug code?

Left-over debug code?

Can you move the image files into "static/images/" ?

Alignment is a little bit off here.

Leftover debug code.

This should also be decorated with @webapi_login_required

If you're not using @augment_method_from, you don't need to define this at all (the ones in the Review Board codebase that use augment use it to override the docstring)

This should be decorated with @webapi_login_required and @webapi_check_local_site.

This should also be decorated with @webapi_check_local_site.

When things are in parentheses, they don't need continuation lines (the ). Your code is also wider than 80 columns here, which we try to avoid.

1 space between # and the comment.

If you wrap the whole conditional in parentheses, you can get rid of the .

Space between the # and the comment. Also "checklist" is tyoped.

Hmm. Actually, request.user is already a User object. How about this?

return checklist.user == request.user

Alignment is a bit off here. Should be like this:

TemplateHook(self, 'base-scripts-post', 'checklist/template.html',
             apply_to=[...])


Feel free to wrap the items in the apply_to list to keep things under 80 columns.

We don't put spaces between "function" and "(". Please change that here and in the rest of the implementation.

Leftover debug code?

This should use ===

Leftover debug code?

This review is mostly small style issues.

Add another blank line here

Remove this blank line.

The alignment isn't quite right here. How about this:

checklist = ReviewChecklist.objects.filter(
    user=user, review_request=review_request)

Indent this line one additional space.

Can you wrap these in () instead of using the  character?

Remove the space between the key name and the :, and deindent the next two lines one space to match.

One additional blank line here.

I suggest changing this to use LESS instead of CSS. Then you could nest some of these rules.

These should use relative paths, in case static media is hosted at a different path. Given your current layout of static media, probably url('../plus.png')
I still think you should move images into a static/images/ directory.

Remove this blank line.

Can you add SITE_ROOT + in front of the URL here? SITE_ROOT should already be in the global javascript namespace.

Comments about methods should be before the method instead of inside it.

Same here about method comments.

Because you're passing this as the context for _.each, you shouldn't need to use self.

===

Same here w/ method comment.

This doesn't just "render", it also creates.

Remove this line.

If this doesn't have any configuration, and you're not planning on adding it right away, you should clean this up and remove this, the url, and the is_configurable flag.

The new js_bundles stuff might be nice to use here, instead of including them by hand.

This should probably be something like "Extension for Review Board which adds checklists for reviewers"

I've been fiddling with your extension this week, Elaine. Very cool. :) Just some notes...

If I click this button, the checklist is permanently deleted.
1) When permanently deleting something, I think we should get the user to confirm this choice before carrying it out.
2) How do I use a checklist again if I've deleted it? How can I start using a checklist again?

Not being used...

Not being used...

Not being used...

Not being used...

You don't need Djblets' webapi_login_required, since you already get it from reviewboard.webapi.decorators.

Not being used...

Not being used...

Whoa, conflict. I think you just need reviewboard.webapi.base WebAPIResource - you don't need djblets.webapi.resource's WebAPIResource too.

Not being used

Not being used

URLHook is not being used.

ChecklistResource is not being used.

This should be you, not None. :)

Hey Elaine,
I know you've closed a bunch of these issues, but your posted revision still has them. Have you forgotten to publish your update?
-Mike

Really minor nits from me now, but this looks really good. There are some whitespace issues that you need to clean-up though - just check for the large red blobs in the diff.

I think we can remove this newline.

var saveOptions

var saveOptions

I think you should break this up with string concatenation, like:
var response = confirm("This action will delete the entire checklist." +

                       "This cannot be undone.");

This can just be:
if (response) {

}

var saveOptions

With the js_bundle stuff, does this still need to be in here?

Maybe format like:
new Checklist.ChecklistView({

    user_id: "{{user.pk}}",

    review_request_id: "{{review_request.id}}"

});

Looking good. Got some style stuff to fix up. Sorry I didn't get that feedback to you sooner (my fault).
Something I recommend is to run the 'pep8' and 'pyflakes' tools on your Python codebase, and the 'jshint' tool on your JavaScript codebase. These are great for finding common style issues and telling you what needs fixing.
Overall though, this is looking like a valuable extension! Good job :)

Space after the hashes.

Blank line after.

Wrap to 79 columns.
Also, typo on "reviewing requests" ("revewing requests").

It's going to require 2.0, because of all the new bundle stuff.

Let's call it an alpha for now, since it's not used in production and would still need real-world testing.

This is assigning the Python builtin function called id, which isn't what you want. It should be id. However, this is the default, so I think you can just remove the line.

Make sure your code wraps to 79 chars.

No need to do the +. Python will intelligently merge those strings together.

Parens aren't needed.

Same with parens here and below.
Also, no blank lines between the conditionals when they're part of the same block. So:
if foo:
    ...
elif bar:
    ...

I'd put this up by has_access_permissions.

The register_ and unregister_ should be aligned.

Two blank lines.

Space before {.
Same with some rules below.

Space after :. Same with some rules below.

You can combine this into one statement.

You can inline this:
return {
    user_id: ...,
    review_request_id: ...,
    ...
};

Why did you need to override parse, instead of just providing a parseResourceData? If there's a special reason, it's best to document it.

No need to define options. Backbone will ignore it and overwrite it.
Instead, what you should do is pull out every option you care about inside initialize below. It'll be passed in an options variable containing what was passed to the view. You can set each of these on this.
The next version of Backbone.js will remove the automatic assignment of this.options, so it's important. (We still need to update our code to do this as well).
Same for other views.

Remove the trailing comma. Some browsers will complain about this otherwise.

Should align these.

Best to define a template on the view and use that:

template: _.template([
    '<p>...</p>',
    '<a>...</a>',
    ...
].join('')),


...


this.$el.html(this.template());

You can use method chaining:
$('#checklist_toggle_size')
    .css(...)
    .attr(...);

No need to quote this.

Missing a var statement.

Blank line isn't needed.

Should align these.

You shoud be able to make use of the bundles now. I think you mentioned it during a meeting.
Also, /static/ isn't always going to be a valid path. Bundles will take care of the path for you, but if you were to use your own script and link tags, you'd need to do:
{% static "ext/checklist.extension.Checklist/..." %}

That will ensure you have the right path for the configuration.

Spaces after the colon.

I'm going to push this to a branch in the rb-extension-pack repo so that it's easier for future contributors to work on it.