The Checklist Extension

Review Request #5007 — Created Nov. 18, 2013 and submitted

LengMalit
rb-extension-pack
reviewboard, students

The Checklist extension allows users to create a checklist when they are reviewing a request. This is useful for cases when there are many things to check for, and the reviewer may want to keep track of them. The app allows:

  • auto creation of a new checklist upon viewing a new review request
  • adding new items to the checklist
  • editing existing items
  • deleting items
  • checking off items
  • undoing checked items
  • minimizing checklist
  • maximizing checklist
  • deleting checklist

The extension is a seperate Django app, and does not modify any of the Reviewboard code. It can be enabled through the Admin page, in the Extension tab.

Manual testing on Chrome and Firefox:
- created checklists by viewing new review requests
- added items on the checklist
- deleted items on checklist
- modified items on checklist
- checking off items
- undoing checked items
- tested on different file attachments but same requests
- tested on different review requests
- tested on different users

Loading file attachments...

Description From Last Updated

If I click this button, the checklist is permanently deleted. 1) When permanently deleting something, I think we should get ...

mike_conleymike_conley

Because all your view does is render a template, you can currently use the built-in template view: from django.conf.urls.defaults import ...

daviddavid

Why do you need this?

daviddavid

There's a bunch of cleanup you can do here: In order to properly handle local sites, you should use ReviewRequestResource.get_object ...

daviddavid

Django provides a nice facility for this: checklist, _ = ReviewChecklist.objects.get_or_create( user=this_user, review_request=this_request)

daviddavid

Left-over debug code?

daviddavid

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

daviddavid

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

daviddavid

Left-over debug code?

daviddavid

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

daviddavid

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

daviddavid

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

daviddavid

Trailing whitespace.

daviddavid

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

daviddavid

This should use === instead of ==

daviddavid

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

daviddavid

This looks like left-over debug code?

daviddavid

Left-over debug code?

daviddavid

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

daviddavid

Left-over debug code?

daviddavid

=== null

daviddavid

Left-over debug code?

daviddavid

Left-over debug code?

daviddavid

Left-over debug code?

daviddavid

Left-over debug code?

daviddavid

Left-over debug code?

daviddavid

Alignment is a little bit off here.

daviddavid

Leftover debug code.

daviddavid

This should also be decorated with @webapi_login_required

daviddavid

If you're not using @augment_method_from, you don't need to define this at all (the ones in the Review Board codebase ...

daviddavid

This should be decorated with @webapi_login_required and @webapi_check_local_site.

daviddavid

This should also be decorated with @webapi_check_local_site.

daviddavid

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

daviddavid

1 space between # and the comment.

daviddavid

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

daviddavid

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

daviddavid

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

daviddavid

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

daviddavid

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

daviddavid

Leftover debug code?

daviddavid

This should use ===

daviddavid

Leftover debug code?

daviddavid

Not being used...

mike_conleymike_conley

Not being used...

mike_conleymike_conley

Not being used...

mike_conleymike_conley

Not being used...

mike_conleymike_conley

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

mike_conleymike_conley

Not being used...

mike_conleymike_conley

Not being used...

mike_conleymike_conley

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

mike_conleymike_conley

Add another blank line here

daviddavid

Remove this blank line.

daviddavid

The alignment isn't quite right here. How about this: checklist = ReviewChecklist.objects.filter( user=user, review_request=review_request)

daviddavid

Indent this line one additional space.

daviddavid

Not being used

mike_conleymike_conley

Not being used

mike_conleymike_conley

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

daviddavid

URLHook is not being used.

mike_conleymike_conley

ChecklistResource is not being used.

mike_conleymike_conley

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

daviddavid

One additional blank line here.

daviddavid

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

daviddavid

These should use relative paths, in case static media is hosted at a different path. Given your current layout of ...

daviddavid

Remove this blank line.

daviddavid

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

daviddavid

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

daviddavid

Same here about method comments.

daviddavid

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

daviddavid

===

daviddavid

Same here w/ method comment.

daviddavid

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

daviddavid

Remove this line.

daviddavid

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

daviddavid

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

daviddavid

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

daviddavid

This should be you, not None. :)

mike_conleymike_conley

I think we can remove this newline.

mike_conleymike_conley

var saveOptions

mike_conleymike_conley

var saveOptions

mike_conleymike_conley

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

mike_conleymike_conley

This can just be: if (response) { }

mike_conleymike_conley

var saveOptions

mike_conleymike_conley

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

mike_conleymike_conley

Maybe format like: new Checklist.ChecklistView({ user_id: "{{user.pk}}", review_request_id: "{{review_request.id}}" });

mike_conleymike_conley

Space after the hashes.

chipx86chipx86

Blank line after.

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

This is assigning the Python builtin function called id, which isn't what you want. It should be id. However, this ...

chipx86chipx86

Make sure your code wraps to 79 chars.

chipx86chipx86

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

chipx86chipx86

Parens aren't needed.

chipx86chipx86

Same with parens here and below. Also, no blank lines between the conditionals when they're part of the same block. ...

chipx86chipx86

I'd put this up by has_access_permissions.

chipx86chipx86

The register_ and unregister_ should be aligned.

chipx86chipx86

Two blank lines.

chipx86chipx86

Space before {. Same with some rules below.

chipx86chipx86

Space after :. Same with some rules below.

chipx86chipx86

You can combine this into one statement.

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

No need to define options. Backbone will ignore it and overwrite it. Instead, what you should do is pull out ...

chipx86chipx86

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

chipx86chipx86

Should align these.

chipx86chipx86

Best to define a template on the view and use that: template: _.template([ '<p>...</p>', '<a>...</a>', ... ].join('')), ... this.$el.html(this.template());

chipx86chipx86

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

chipx86chipx86

No need to quote this.

chipx86chipx86

Missing a var statement.

chipx86chipx86

Blank line isn't needed.

chipx86chipx86

Should align these.

chipx86chipx86

You shoud be able to make use of the bundles now. I think you mentioned it during a meeting. Also, ...

chipx86chipx86

Spaces after the colon.

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

  2. 
      
david
  1. 
      
  2. checklist/checklist/admin_urls.py (Diff revision 1)
     
     

    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.

  3. checklist/checklist/checklistResource.py (Diff revision 1)
     
     
     
     
     
     

    Why do you need this?

    1. I'm not entirely sure why, but when I implement has_access_permissions to return True, doing a 'GET' results in an XML file download. This is how I tried to implement has_access_permissions:

      def has_access_permissions(self, request, checklist, *args, **kwags):
          this_user = User.objects.filter(pk=request.user.pk)[0]
          return checklist.user == this_user
      

      And then a 'GET' call would result in a file that looks like this: http://pastie.org/8499554

      The checklist itself doesn't require 'GET' to be implemented, but I'm just using 'GET' for debugging purposes, to see if my changes on the checklist are getting implemented.

    2. How did you do the GET?

      The API supports both XML and JSON formats. If you access it through something that prefers XML (say, just opening the page in a web browser) you'll get the XML.

    3. Ah yes, that's how I do my GET. I see, I understand now.

  4. checklist/checklist/checklistResource.py (Diff revision 1)
     
     
     
     
     

    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)

  5. checklist/checklist/checklistResource.py (Diff revision 1)
     
     
     
     
     
     
     
     
     
     
     
     

    Django provides a nice facility for this:

    checklist, _ = ReviewChecklist.objects.get_or_create(
        user=this_user,
        review_request=this_request)
    
  6. checklist/checklist/checklistResource.py (Diff revision 1)
     
     
     

    Left-over debug code?

    1. Thanks! I'll fix these soon. And I didn't realize so much of the debug code are still here. I removed them at school, but I must have forgotten to update my repo at home. =)

  7. checklist/checklist/checklistResource.py (Diff revision 1)
     
     
     
     
     

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

  8. checklist/checklist/checklistResource.py (Diff revision 1)
     
     
     

    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.

  9. Left-over debug code?

  10. checklist/checklist/checklistResource.py (Diff revision 1)
     
     
     

    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.

  11. checklist/checklist/extension.py (Diff revision 1)
     
     
     

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

  12. checklist/checklist/models.py (Diff revision 1)
     
     
     
     
     
     
     
     

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

  13. checklist/checklist/models.py (Diff revision 1)
     
     

    Trailing whitespace.

  14. checklist/checklist/static/js/models/checklistAPI.js (Diff revision 1)
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    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;
    },
    ````

    1. This gives me 400 Bad Request for some reason.

    2. There's a bunch of internal attributes as part of a model that will fail request parameter validation. You'll need to build it yourself.

      You shouldn't need all the conditionals, though. Shouldn't those have valid values?

      A trick for essentially turning a null to a non-entry (which I believe the AJAX code will skip when submitting the request) is to do:

      return {
          checklist_id: this.get('checklist_id') || undefined,
          ...
      }
      

      Still, though, I think it's best to ensure these all have valid values when making a request, and serializing all supported values.

    3. Yeah, sometimes I turn some of those variables to null on purpose. For example, if I edit the description of a checklist item, checklist_item_id would be set. But when I'm adding a new item, checklist_item_id should be reset to null, because the item hasn't been created yet.

      I'll try your suggestion.

  15. This should use === instead of ==

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

  17. checklist/checklist/static/js/views/checklistView.js (Diff revision 1)
     
     
     
     
     
     
     
     
     

    This looks like left-over debug code?

  18. Left-over debug code?

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

    1. Hmm... I'm not sure how to connect it exactly.

      I tried 'change this.model': 'render', but it never gets rendered after the item is modified.

    2. In your view's initialize function, you'd do something like this:

      this.listenTo(this.model, 'change', this.render);
      
  20. Left-over debug code?

  21. Left-over debug code?

  22. Left-over debug code?

  23. checklist/checklist/static/js/views/checklistView.js (Diff revision 1)
     
     
     
     
     
     
     
     
     

    Left-over debug code?

  24. Left-over debug code?

  25. Left-over debug code?

  26. 
      
LE
david
  1. Can you move the image files into "static/images/" ?

  2. checklist/checklist/checklistResource.py (Diff revision 2)
     
     
     
     

    Alignment is a little bit off here.

  3. Leftover debug code.

  4. This should also be decorated with @webapi_login_required

  5. checklist/checklist/checklistResource.py (Diff revision 2)
     
     
     
     

    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)

  6. This should be decorated with @webapi_login_required and @webapi_check_local_site.

  7. This should also be decorated with @webapi_check_local_site.

  8. checklist/checklist/checklistResource.py (Diff revision 2)
     
     
     

    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.

  9. 1 space between # and the comment.

  10. checklist/checklist/checklistResource.py (Diff revision 2)
     
     
     

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

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

  12. checklist/checklist/checklistResource.py (Diff revision 2)
     
     
     

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

    return checklist.user == request.user
    
  13. checklist/checklist/extension.py (Diff revision 2)
     
     
     

    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.

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

  15. Leftover debug code?

  16. This should use ===

  17. Leftover debug code?

  18. 
      
LE
david
  1. This review is mostly small style issues.

  2. Add another blank line here

  3. Remove this blank line.

  4. checklist/checklist/checklistResource.py (Diff revision 3)
     
     
     

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

    checklist = ReviewChecklist.objects.filter(
        user=user, review_request=review_request)
    
  5. Indent this line one additional space.

  6. checklist/checklist/extension.py (Diff revision 3)
     
     
     

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

  7. checklist/checklist/extension.py (Diff revision 3)
     
     

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

  8. checklist/checklist/models.py (Diff revision 3)
     
     

    One additional blank line here.

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

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

  11. Remove this blank line.

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

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

  14. Same here about method comments.

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

  16. Same here w/ method comment.

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

  18. Remove this line.

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

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

  21. checklist/setup.py (Diff revision 3)
     
     

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

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

  2. 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?

    1. 2) Hmm... yeah, I'm not sure whether my implementation for that is suave enough, but a checklist gets created or fetched from the backend every time the user tries to review a request. So a new checklist is created again when the user views a diff/attachment, or simply reloads the page. I think eventually there should be a way for the user to configure when or where he/she wants the checklist to appear.

  3. Not being used...

  4. Not being used...

  5. Not being used...

  6. Not being used...

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

  8. Not being used...

  9. Not being used...

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

  11. checklist/checklist/extension.py (Diff revision 3)
     
     

    Not being used

  12. checklist/checklist/extension.py (Diff revision 3)
     
     

    Not being used

  13. checklist/checklist/extension.py (Diff revision 3)
     
     

    URLHook is not being used.

  14. checklist/checklist/extension.py (Diff revision 3)
     
     

    ChecklistResource is not being used.

  15. checklist/setup.py (Diff revision 3)
     
     

    This should be you, not None. :)

  16. 
      
mike_conley
  1. 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

    1. Ah, yes I did forget! Ahh, omg, thanks for reminding me!!

  2. 
      
LE
LE
LE
mike_conley
  1. 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.

  2. I think we can remove this newline.

  3. 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.");

  4. This can just be:

    if (response) {
    }

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

    1. I couldn't get it working yet, so that's why this is still here.

  6. Maybe format like:

    new Checklist.ChecklistView({
    user_id: "{{user.pk}}",
    review_request_id: "{{review_request.id}}"
    });

  7. 
      
LE
LE
chipx86
  1. 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 :)

  2. checklist/README.md (Diff revision 6)
     
     

    Space after the hashes.

  3. checklist/README.md (Diff revision 6)
     
     

    Blank line after.

  4. checklist/README.md (Diff revision 6)
     
     

    Wrap to 79 columns.

    Also, typo on "reviewing requests" ("revewing requests").

  5. checklist/README.md (Diff revision 6)
     
     

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

  6. checklist/README.md (Diff revision 6)
     
     

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

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

  8. Make sure your code wraps to 79 chars.

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

  10. Parens aren't needed.

  11. checklist/checklist/checklistResource.py (Diff revision 6)
     
     
     

    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:
        ...
    
  12. checklist/checklist/checklistResource.py (Diff revision 6)
     
     
     

    I'd put this up by has_access_permissions.

  13. checklist/checklist/extension.py (Diff revision 6)
     
     
     

    The register_ and unregister_ should be aligned.

  14. checklist/checklist/extension.py (Diff revision 6)
     
     
     
     

    Two blank lines.

  15. Space before {.

    Same with some rules below.

  16. Space after :. Same with some rules below.

  17. You can combine this into one statement.

  18. checklist/checklist/static/js/models/checklistAPI.js (Diff revision 6)
     
     
     
     
     
     
     

    You can inline this:

    return {
        user_id: ...,
        review_request_id: ...,
        ...
    };
    
  19. 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.

  20. checklist/checklist/static/js/views/checklistView.js (Diff revision 6)
     
     
     
     
     
     

    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.

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

  22. Should align these.

  23. checklist/checklist/static/js/views/checklistView.js (Diff revision 6)
     
     
     
     
     

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

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

    ...
    

    this.$el.html(this.template());
    
  24. You can use method chaining:

    $('#checklist_toggle_size')
        .css(...)
        .attr(...);
    
  25. No need to quote this.

  26. Missing a var statement.

  27. Blank line isn't needed.

  28. Should align these.

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

    1. Okay, I'll do the static one for now, because I can't get the bundles working, but I'll return to it a bit later.

  30. Spaces after the colon.

  31. 
      
LE
LE
david
  1. 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.

  2. 
      
LE
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to rb-extension-pack/checklist (c97ce7b). Thanks!
Loading...