[WIP] Action feed prototype using a separate table for storing actions

Review Request #3151 — Created June 19, 2012 and discarded

Information

Review Board

Reviewers

In the current stage, the action feed is the default dashboard view and it displays info related to [almost] any update to the requests the user has previously visited.
The action-boxes contain the review request id and summary, the name of the user who performed the action and the type of action performed.

Still to-do:
   * restyle the dashboard entries box
   * make the action boxes expandable and implement ajax calls to display their content
No tests yet - will start running them/ adding new ones once this is a bit more advanced.
Description From Last Updated

Alphabetical order.

ME medanat

Alphabetical order.

ME medanat

the "review_published.send()" can be taken out of the if statement since it will be called anyway. So: verb = 'published …

ME medanat

intermediate variable again.

ME medanat

I believe this fits on one line.

ME medanat

Alphabetical order.

ME medanat

Keep this empty line before the return.

ME medanat

No need for this empty line.

ME medanat

No need for this empty line.

ME medanat

No need for this empty line.

ME medanat

No need for this empty line.

ME medanat

May I ask why you are using !important here?

ME medanat

No need for those empty lines.

ME medanat

No need for the empty lines here.

ME medanat

Indent all by one.

ME medanat

No need for this extra line.

ME medanat

indent "compressed_css"

ME medanat

indent "include" and "{{datagrid...}}"

ME medanat

No need for space between 'view' and :

ME medanat

Remove space before ':'

ME medanat

Space after ':'

ME medanat

Remove space before the ':'.

ME medanat

Indent inside the '{%', not outside. i.e.: {% for {% if {% definevar ... {% endif {% endfor

ME medanat

One more level of indentation here.

ME medanat

Alphabetical order.

ME medanat

No need for this empty line.

ME medanat

Indent within the '{%'.

ME medanat

Indent within the '{%'.

ME medanat

Indent within the '{%'.

ME medanat

Tiny optimization: _request = self.request Since self.request is being called three times within the method body. Less .'s means faster …

ME medanat

Add an empty line here.

ME medanat

Wouldn't it be easier to do this calculation before hand and leave the reasoning in a comment? i.e.: = 31536000 …

ME medanat

Alphabetical order.

ME medanat

Translate: Review request. Remove the trailing white space.

ME medanat

You can shift the the {% %} logic all the way to the left. That saves on white space being …

ME medanat

Translate: Posted.

ME medanat

Translate: Updated.

ME medanat

Translate: Ago.

ME medanat

We're probably not doing great about alphabetical order here, but might as well place this above the others.

chipx86chipx86

Just for readability, put each parameter on its own line (and start with the Action.objects.create) line.

chipx86chipx86

Same as above. Actually, given that we do this twice, can we just set a couple common variables and then …

chipx86chipx86

Same here.

chipx86chipx86

Same here. Blank line after this. I prefer blank lines between blocks and other code (like around an if, for, …

chipx86chipx86

Was this intentional? It looks like a mistake.

chipx86chipx86

Same.

chipx86chipx86

Generally, (and I know a lot of older code doesn't do this), docs should be in this format: """One-line summary. …

chipx86chipx86

No blank line.

chipx86chipx86

Instead of this, do this above: @property def request_display_id(self): ...

chipx86chipx86

Are we expecting that the action feed will now be the default view? I'd be a little concerned about that, …

chipx86chipx86

No blank line. This should subclass from object.

chipx86chipx86

Can you document why we need this?

chipx86chipx86

In general, we should use "pk" instead of "id" when referring to the primary key of the model. Same with …

chipx86chipx86

Add a trailing comma.

chipx86chipx86

Alignment is off.

chipx86chipx86

Space before {

chipx86chipx86

All indentation should be 2 lines instead of 4.

chipx86chipx86

Another case of {. Same with others in this file.

chipx86chipx86

No blank line.

chipx86chipx86

No spaces around =

chipx86chipx86

One space indentation.

chipx86chipx86

No spaces inside {{..}}

chipx86chipx86

Let's go ahead and use 'action-feed'. '-' is nicer in URLs.

chipx86chipx86

No blank line.

chipx86chipx86

We should avoid queries on these objects. So instead, do: if self.review_id: return self.review_id elif self.changedesc_id: return self.changedesc_id This should …

chipx86chipx86

No blank line.

chipx86chipx86

Should also check review_id and changedesc_id, and return some default.

chipx86chipx86

action-feed.

chipx86chipx86

Add a trailing comma.

chipx86chipx86

local_site_id

chipx86chipx86

Indentation looks wrong. This map should only be defined once, outside the function.

chipx86chipx86

You should only have one var statement, formatted like: var body = ..., request_url = ...;

chipx86chipx86

Best as: var changeitems = $('') .appendTo(body);

chipx86chipx86

This var needs to also be before. In JavaScript, all variables when compiling in the VM are moved to the …

chipx86chipx86

One var statemnet.

chipx86chipx86

Best to append changeitem after you've completely populated it, at the end of the loop.

chipx86chipx86

Best done as: field_name = field_name_map[field] || field

chipx86chipx86

Should be: changeitem.append($('').text(field_name)); This will do the right thing if there are characters in field_name that need escaping (such as …

chipx86chipx86

In general, instead of 'in', it's best to do field_changed.hasOwnProperty('added'). The reason being that if anything were to populate the …

chipx86chipx86

Hmm, not sure what you're doing here? I don't know that an object for added/removed is correct.

chipx86chipx86

In JavaScript, always use braces. Given that we do field_changed[type] a lot, I would just pull that value out once …

chipx86chipx86

vars must be at the top of the scope.

chipx86chipx86

===. Avoids a cast.

chipx86chipx86

Build this with jQuery's methods to ensure there aren't HTML encoding issues.

chipx86chipx86

No blank line.

chipx86chipx86

Braces. Why the length check?

chipx86chipx86

One var, top of scope.

chipx86chipx86

No blank lines. For the field_changed length check, you'll want to pull the length out into a variable first. Saves …

chipx86chipx86

hasOwnProperty.

chipx86chipx86

Build with jQuery.

chipx86chipx86

hasOwnProperty.

chipx86chipx86

Build with jQuery.

chipx86chipx86

Blank line.

chipx86chipx86

hasOwnProperty.

chipx86chipx86

Alignment for multiline is wrong. Should use === for comparison. One set of var statements.

chipx86chipx86

Maybe oldValue/newValue?

chipx86chipx86

===

chipx86chipx86

All this should be built with jQuery.

chipx86chipx86

=== Alignment is wrong.

chipx86chipx86

Move var out of the for, since it's really going to be promoted to the top of the "else if" …

chipx86chipx86

Build with jQuery. Make sure to always use braces.

chipx86chipx86

If so, maybe we should log that.

chipx86chipx86

Blank line.

chipx86chipx86

Build with jQuery.

chipx86chipx86

Blank line.

chipx86chipx86

Alignment is off. The whole function should build with jQuery.

chipx86chipx86

Same.

chipx86chipx86

One var.

chipx86chipx86

Same.

chipx86chipx86

Alignment is off.

chipx86chipx86

$('') .appendTo(body);

chipx86chipx86

One var.

chipx86chipx86

Move this into the declaration above, before my proposed appendTo.

chipx86chipx86

Move var up, and pull the length out.

chipx86chipx86

vars go first. comment can probably just be defined in each loop that needs it.

chipx86chipx86

Build with jQuery. These comments apply to everywhere else in the JavaScript.

chipx86chipx86

No blank line.

chipx86chipx86

No blank line. The resource needs a description. This will be auto-generated for our API docs page. Make sure to …

chipx86chipx86

I've noticed this a couple times now. Instead of repeating it, you should define a Manager class that the Action …

chipx86chipx86

You can combine these.

chipx86chipx86

You can do: if field in ('screenshot_captions, 'file_captions'):

chipx86chipx86
ME
  1. I know its a WIP. Quick review, mostly style issues.
  2. reviewboard/reviews/admin.py (Diff revision 1)
     
     
    Show all issues
    Alphabetical order.
  3. reviewboard/reviews/admin.py (Diff revision 1)
     
     
    Show all issues
    Alphabetical order.
  4. reviewboard/reviews/models.py (Diff revision 1)
     
     
     
     
     
    Is there a need for the intermediate variable "action"? I know it looks better, but you can just attach .save to the Action() init.
    1. No, there's no need for the extra variable.
      However, I think doing it this way makes the code more readable. 
      Do you mind if we get a third opinion before changing this? :)
    2. I think it makes it more readable as well. I didn't mark it as an issue for that reason. I don't see a real need to change this.
    3. In the case where you're just creating and saving, Django actually has a method for this:
      
      Action.objects.create(review_request=self, .....)
      
      It returns the action in case you need to make use of it still.
  5. reviewboard/reviews/models.py (Diff revision 1)
     
     
     
     
     
    Same thing here.
  6. reviewboard/reviews/models.py (Diff revision 1)
     
     
     
     
     
    Same thing.
  7. reviewboard/reviews/models.py (Diff revision 1)
     
     
     
     
     
     
     
     
     
    Show all issues
    the "review_published.send()" can be taken out of the if statement since it will be called anyway.
    
    So:
    
    verb = 'published a review'
    
    if self.is_reply():
        verb = 'published a reply'
    
    reply_published.send(sender=self.__class__, user=user, reply=self)
    1. I just realized That reply_published != review_published. Please drop this issue.
  8. reviewboard/reviews/models.py (Diff revision 1)
     
     
     
     
     
    Show all issues
    intermediate variable again.
  9. reviewboard/reviews/models.py (Diff revision 1)
     
     
     
     
    Show all issues
    I believe this fits on one line.
    1. I planned on adding more comprehensive docstring later on. So for now, I'll just leave this issue open.
  10. reviewboard/reviews/views.py (Diff revision 1)
     
     
    I don't see why this import was removed, was it unused?
    1. Yes, it was unused. I might have gotten caught up in a cleaning frenzy for a few minutes there.
  11. reviewboard/reviews/views.py (Diff revision 1)
     
     
    Show all issues
    Alphabetical order.
  12. reviewboard/reviews/views.py (Diff revision 1)
     
     
    Show all issues
    Keep this empty line before the return.
  13. reviewboard/reviews/views.py (Diff revision 1)
     
     
    Show all issues
    No need for this empty line.
  14. reviewboard/reviews/views.py (Diff revision 1)
     
     
    Show all issues
    No need for this empty line.
  15. reviewboard/reviews/views.py (Diff revision 1)
     
     
    Show all issues
    No need for this empty line.
  16. Show all issues
    No need for this empty line.
  17. Show all issues
    May I ask why you are using !important here?
    1. This was a copy-pasta fail. :) 
      I was focusing more on the backend, so I just copied CSS pieces in order to make a basic UI to reflect the changes.
  18. reviewboard/static/rb/css/action-feed.less (Diff revision 1)
     
     
     
     
    Show all issues
    No need for those empty lines.
  19. reviewboard/static/rb/css/action-feed.less (Diff revision 1)
     
     
     
    Show all issues
    No need for the empty lines here.
  20. reviewboard/templates/reviews/action_feed.html (Diff revision 1)
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues
    Indent all by one.
  21. Show all issues
    No need for this extra line.
  22. Show all issues
    indent "compressed_css"
  23. reviewboard/templates/reviews/dashboard.html (Diff revision 1)
     
     
     
     
     
     
    Show all issues
    indent "include" and "{{datagrid...}}"
  24. 
      
BO
ME
  1. A few more style issues.
  2. reviewboard/reviews/views.py (Diff revision 2)
     
     
    Show all issues
    No need for space between 'view' and :
  3. reviewboard/settings.py (Diff revision 2)
     
     
    Show all issues
    Remove space before ':'
  4. reviewboard/settings.py (Diff revision 2)
     
     
    Show all issues
    Space after ':'
  5. reviewboard/settings.py (Diff revision 2)
     
     
     
    Show all issues
    Remove space before the ':'.
  6. reviewboard/templates/reviews/action_feed.html (Diff revision 2)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues
    Indent inside the '{%', not outside.
    
    i.e.:
    
    {% for
    {%  if
    {%    definevar ...
    {%  endif
    {% endfor
  7. reviewboard/templates/reviews/action_feed.html (Diff revision 2)
     
     
     
     
     
    Show all issues
    One more level of indentation here.
  8. Show all issues
    Alphabetical order.
  9. Show all issues
    No need for this empty line.
  10. reviewboard/templates/reviews/dashboard.html (Diff revision 2)
     
     
     
     
    Show all issues
    Indent within the '{%'.
  11. reviewboard/templates/reviews/dashboard.html (Diff revision 2)
     
     
     
     
    Show all issues
    Indent within the '{%'.
  12. reviewboard/templates/reviews/dashboard.html (Diff revision 2)
     
     
     
     
    Show all issues
    Indent within the '{%'.
  13. 
      
BO
BO
BO
ME
  1. Quick review, mostly style.
  2. reviewboard/reviews/views.py (Diff revision 5)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues
    Tiny optimization:
    
    _request = self.request
    
    Since self.request is being called three times within the method body.
    
    Less .'s means faster code.
    1. I don't know if I like the idea of an extra variable, just for a very small optimization in attribute lookup. I don't think you'd see a measurable speed increase with this optimization.
    2. What's the problem with an extra variable?
    3. It's just additional complexity (albeit very very little extra), for an optimization that I don't think will be a benefit. I haven't measured it myself though, so I'm just guessing at it's effectiveness.
    4. We need a tie breaker on this one.
  3. reviewboard/reviews/views.py (Diff revision 5)
     
     
     
    Show all issues
    Add an empty line here.
  4. reviewboard/settings.py (Diff revision 5)
     
     
    Show all issues
    Wouldn't it be easier to do this calculation before hand and leave the reasoning in a comment?
    
    i.e.:
    
     = 31536000 # 365 * 24 * 60 * 60  (1 year)
    1. I disagree. With Bogdana's way it's easy to see SESSION_COOKIE_AGE is the number of seconds in a year, even without the comment. It would be quite easy to remove a zero, or change the fully calculated number without noticing, and the CPU cycles saved here would be negligible.
    2. I just feel this is an unnecessary calculation.
      
      It's not actually part of the change, I think Bogdana just added a space after the calculation for some reason.
      
      If this isn't going to be pre-calculated, please remove the additional space.
    3. Calculation aside, the additional space is a PEP8 thing: "Inline comments should be separated by at least two spaces from the statement."
      
      Whether that should be included with this change, I'm not sure, but it is proper PEP8 clean-up :D
    4. That makes sense. Cool!
  5. reviewboard/templates/reviews/action_feed.html (Diff revision 5)
     
     
     
     
     
     
     
     
    Show all issues
    Alphabetical order.
  6. Show all issues
    Translate: Review request.
    
    Remove the trailing white space.
  7. reviewboard/templates/reviews/action_feed.html (Diff revision 5)
     
     
     
     
     
     
    Show all issues
    You can shift the the {% %} logic all the way to the left. That saves on white space being sent to the client.
  8. Show all issues
    Translate: Posted.
  9. Show all issues
    Translate: Updated.
  10. Show all issues
    Translate: Ago.
  11. 
      
BO
BO
BO
BO
chipx86
  1. 
      
  2. reviewboard/reviews/admin.py (Diff revision 7)
     
     
    Show all issues
    We're probably not doing great about alphabetical order here, but might as well place this above the others.
  3. reviewboard/reviews/models.py (Diff revision 7)
     
     
     
     
     
    Show all issues
    Just for readability, put each parameter on its own line (and start with the Action.objects.create) line.
  4. reviewboard/reviews/models.py (Diff revision 7)
     
     
     
     
     
     
    Show all issues
    Same as above.
    
    Actually, given that we do this twice, can we just set a couple common variables and then create the action after this if/else?
  5. reviewboard/reviews/models.py (Diff revision 7)
     
     
     
     
     
     
    Show all issues
    Same here.
  6. reviewboard/reviews/models.py (Diff revision 7)
     
     
     
     
     
    Show all issues
    Same here.
    
    Blank line after this. I prefer blank lines between blocks and other code (like around an if, for, etc.)
  7. reviewboard/reviews/models.py (Diff revision 7)
     
     
    Show all issues
    Was this intentional? It looks like a mistake.
  8. reviewboard/reviews/models.py (Diff revision 7)
     
     
     
     
     
     
    Show all issues
    Same.
  9. reviewboard/reviews/models.py (Diff revision 7)
     
     
     
     
    Show all issues
    Generally, (and I know a lot of older code doesn't do this), docs should be in this format:
    
    
        """One-line summary.
    
        Multi-line content.
        """
    
    Or in this case, just """One-line summary."""
  10. reviewboard/reviews/models.py (Diff revision 7)
     
     
     
     
    Show all issues
    No blank line.
  11. reviewboard/reviews/models.py (Diff revision 7)
     
     
    Show all issues
    Instead of this, do this above:
    
    
    @property
    def request_display_id(self):
        ...
  12. reviewboard/reviews/views.py (Diff revision 7)
     
     
    Show all issues
    Are we expecting that the action feed will now be the default view?
    
    I'd be a little concerned about that, as it changes everybody's workflow. Instead, I'd rather we keep the existing default, and then later we should add some easier way to select a default or quickly jump to the desired node from any page.
  13. reviewboard/reviews/views.py (Diff revision 7)
     
     
     
     
    Show all issues
    No blank line.
    
    This should subclass from object.
  14. reviewboard/reviews/views.py (Diff revision 7)
     
     
     
     
     
     
     
    Show all issues
    Can you document why we need this?
  15. reviewboard/reviews/views.py (Diff revision 7)
     
     
    Show all issues
    In general, we should use "pk" instead of "id" when referring to the primary key of the model.
    
    Same with the user above.
  16. reviewboard/reviews/views.py (Diff revision 7)
     
     
    Show all issues
    Add a trailing comma.
  17. reviewboard/reviews/views.py (Diff revision 7)
     
     
     
    Show all issues
    Alignment is off.
  18. Show all issues
    Space before {
  19. Show all issues
    All indentation should be 2 lines instead of 4.
  20. Show all issues
    Another case of {.
    
    Same with others in this file.
  21. reviewboard/static/rb/css/action-feed.less (Diff revision 7)
     
     
     
     
    Show all issues
    No blank line.
  22. Show all issues
    No spaces around =
  23. Show all issues
    One space indentation.
  24. Show all issues
    No spaces inside {{..}}
  25. 
      
BO
BO
BO
chipx86
  1. 
      
  2. reviewboard/reviews/datagrids.py (Diff revision 11)
     
     
    Show all issues
    Let's go ahead and use 'action-feed'. '-' is nicer in URLs.
  3. reviewboard/reviews/models.py (Diff revision 11)
     
     
     
    Show all issues
    No blank line.
  4. reviewboard/reviews/models.py (Diff revision 11)
     
     
     
     
     
    Show all issues
    We should avoid queries on these objects. So instead, do:
    
    if self.review_id:
        return self.review_id
    elif self.changedesc_id:
        return self.changedesc_id
    
    This should return something when neither of the above conditionals match.
  5. reviewboard/reviews/models.py (Diff revision 11)
     
     
     
    Show all issues
    No blank line.
  6. reviewboard/reviews/models.py (Diff revision 11)
     
     
     
     
     
    Show all issues
    Should also check review_id and changedesc_id, and return some default.
  7. Show all issues
    action-feed.
  8. reviewboard/reviews/views.py (Diff revision 11)
     
     
    Show all issues
    Add a trailing comma.
  9. reviewboard/reviews/views.py (Diff revision 11)
     
     
     
    Show all issues
    local_site_id
  10. reviewboard/static/rb/js/action-feed.js (Diff revision 11)
     
     
     
     
     
    Show all issues
    Indentation looks wrong.
    
    This map should only be defined once, outside the function.
  11. reviewboard/static/rb/js/action-feed.js (Diff revision 11)
     
     
     
     
     
    Show all issues
    You should only have one var statement, formatted like:
    
    
    var body = ...,
        request_url = ...;
  12. reviewboard/static/rb/js/action-feed.js (Diff revision 11)
     
     
    Show all issues
    Best as:
    
    var changeitems = $('<ul/>')
        .appendTo(body);
  13. reviewboard/static/rb/js/action-feed.js (Diff revision 11)
     
     
    Show all issues
    This var needs to also be before. In JavaScript, all variables when compiling in the VM are moved to the top of the scope.
  14. reviewboard/static/rb/js/action-feed.js (Diff revision 11)
     
     
     
     
    Show all issues
    One var statemnet.
  15. reviewboard/static/rb/js/action-feed.js (Diff revision 11)
     
     
    Show all issues
    Best to append changeitem after you've completely populated it, at the end of the loop.
  16. reviewboard/static/rb/js/action-feed.js (Diff revision 11)
     
     
     
     
     
    Show all issues
    Best done as:
    
    field_name = field_name_map[field] || field
  17. reviewboard/static/rb/js/action-feed.js (Diff revision 11)
     
     
    Show all issues
    Should be:
    
        changeitem.append($('<label/>').text(field_name));
    
    This will do the right thing if there are characters in field_name that need escaping (such as when localized).
  18. reviewboard/static/rb/js/action-feed.js (Diff revision 11)
     
     
    Show all issues
    In general, instead of 'in', it's best to do field_changed.hasOwnProperty('added').
    
    The reason being that if anything were to populate the base object with 'added' or whatever, this check could fail. By using hasOwnProperty, you ensure you're only looking up in field_changed. It also won't nest into the prototype, so it's a little quicker.
  19. reviewboard/static/rb/js/action-feed.js (Diff revision 11)
     
     
    Show all issues
    Hmm, not sure what you're doing here? I don't know that an object for added/removed is correct.
  20. reviewboard/static/rb/js/action-feed.js (Diff revision 11)
     
     
     
    Show all issues
    In JavaScript, always use braces.
    
    Given that we do field_changed[type] a lot, I would just pull that value out once up-front and reuse it.
  21. reviewboard/static/rb/js/action-feed.js (Diff revision 11)
     
     
    Show all issues
    vars must be at the top of the scope.
  22. reviewboard/static/rb/js/action-feed.js (Diff revision 11)
     
     
    Show all issues
    ===. Avoids a cast.
  23. reviewboard/static/rb/js/action-feed.js (Diff revision 11)
     
     
     
     
    Show all issues
    Build this with jQuery's methods to ensure there aren't HTML encoding issues.
  24. reviewboard/static/rb/js/action-feed.js (Diff revision 11)
     
     
     
    Show all issues
    No blank line.
  25. reviewboard/static/rb/js/action-feed.js (Diff revision 11)
     
     
     
    Show all issues
    Braces.
    
    Why the length check?
  26. reviewboard/static/rb/js/action-feed.js (Diff revision 11)
     
     
     
     
    Show all issues
    One var, top of scope.
  27. reviewboard/static/rb/js/action-feed.js (Diff revision 11)
     
     
     
     
     
     
    Show all issues
    No blank lines.
    
    For the field_changed length check, you'll want to pull the length out into a variable first. Saves on a recompute every iteration.
  28. reviewboard/static/rb/js/action-feed.js (Diff revision 11)
     
     
    Show all issues
    hasOwnProperty.
  29. reviewboard/static/rb/js/action-feed.js (Diff revision 11)
     
     
     
     
    Show all issues
    Build with jQuery.
  30. reviewboard/static/rb/js/action-feed.js (Diff revision 11)
     
     
    Show all issues
    hasOwnProperty.
  31. reviewboard/static/rb/js/action-feed.js (Diff revision 11)
     
     
     
    Show all issues
    Build with jQuery.
  32. reviewboard/static/rb/js/action-feed.js (Diff revision 11)
     
     
     
    Show all issues
    Blank line.
  33. reviewboard/static/rb/js/action-feed.js (Diff revision 11)
     
     
    Show all issues
    hasOwnProperty.
  34. reviewboard/static/rb/js/action-feed.js (Diff revision 11)
     
     
     
     
     
     
    Show all issues
    Alignment for multiline is wrong.
    
    Should use === for comparison.
    
    One set of var statements.
  35. reviewboard/static/rb/js/action-feed.js (Diff revision 11)
     
     
     
    Show all issues
    Maybe oldValue/newValue?
  36. reviewboard/static/rb/js/action-feed.js (Diff revision 11)
     
     
    Show all issues
    ===
  37. reviewboard/static/rb/js/action-feed.js (Diff revision 11)
     
     
     
    Show all issues
    All this should be built with jQuery.
  38. reviewboard/static/rb/js/action-feed.js (Diff revision 11)
     
     
     
    Show all issues
    ===
    
    Alignment is wrong.
  39. reviewboard/static/rb/js/action-feed.js (Diff revision 11)
     
     
    Show all issues
    Move var out of the for, since it's really going to be promoted to the top of the "else if" block anyway.
  40. reviewboard/static/rb/js/action-feed.js (Diff revision 11)
     
     
     
    Show all issues
    Build with jQuery.
    
    Make sure to always use braces.
  41. reviewboard/static/rb/js/action-feed.js (Diff revision 11)
     
     
    Show all issues
    If so, maybe we should log that.
  42. reviewboard/static/rb/js/action-feed.js (Diff revision 11)
     
     
     
    Show all issues
    Blank line.
  43. reviewboard/static/rb/js/action-feed.js (Diff revision 11)
     
     
     
     
     
    Show all issues
    Build with jQuery.
  44. reviewboard/static/rb/js/action-feed.js (Diff revision 11)
     
     
     
    Show all issues
    Blank line.
  45. reviewboard/static/rb/js/action-feed.js (Diff revision 11)
     
     
     
    Show all issues
    Alignment is off.
    
    The whole function should build with jQuery.
  46. reviewboard/static/rb/js/action-feed.js (Diff revision 11)
     
     
     
    Show all issues
    Same.
  47. reviewboard/static/rb/js/action-feed.js (Diff revision 11)
     
     
     
    Show all issues
    One var.
  48. reviewboard/static/rb/js/action-feed.js (Diff revision 11)
     
     
     
    Show all issues
    Same.
  49. reviewboard/static/rb/js/action-feed.js (Diff revision 11)
     
     
     
    Show all issues
    Alignment is off.
  50. reviewboard/static/rb/js/action-feed.js (Diff revision 11)
     
     
     
    Show all issues
    $('<dl/>')
        .appendTo(body);
  51. reviewboard/static/rb/js/action-feed.js (Diff revision 11)
     
     
     
     
    Show all issues
    One var.
  52. reviewboard/static/rb/js/action-feed.js (Diff revision 11)
     
     
    Show all issues
    Move this into the declaration above, before my proposed appendTo.
  53. reviewboard/static/rb/js/action-feed.js (Diff revision 11)
     
     
    Show all issues
    Move var up, and pull the length out.
  54. reviewboard/static/rb/js/action-feed.js (Diff revision 11)
     
     
     
     
     
    Show all issues
    vars go first.
    
    comment can probably just be defined in each loop that needs it.
  55. reviewboard/static/rb/js/action-feed.js (Diff revision 11)
     
     
     
     
     
     
     
     
    Show all issues
    Build with jQuery.
    
    These comments apply to everywhere else in the JavaScript.
  56. reviewboard/static/rb/js/datastore.js (Diff revision 11)
     
     
     
     
    Show all issues
    No blank line.
  57. reviewboard/webapi/resources.py (Diff revision 11)
     
     
     
     
    Show all issues
    No blank line.
    
    The resource needs a description. This will be auto-generated for our API docs page. Make sure to follow the styles for other resources.
  58. reviewboard/webapi/resources.py (Diff revision 11)
     
     
     
     
     
     
     
    Show all issues
    I've noticed this a couple times now. Instead of repeating it, you should define a Manager class that the Action uses, which will define a function for this.
    
    So:
    
    class Action(...):
        ...
        objects = ActionManager()
    
    
    Then in managers.py:
    
    class ActionManager(...):
        def from_visits(self):
            # That query here
    1. This won't be repeated if we use the ActionResource for generating the action feed (see r/3260).
      If that approach isn't ok, I'll just go ahead and add the manager.
    2. Even if we only query it once, logic like this should exist solely in the Manager.
      
      I'd like to see this change made, but it can be a separate change.
  59. reviewboard/webapi/resources.py (Diff revision 11)
     
     
     
    Show all issues
    You can combine these.
  60. reviewboard/webapi/resources.py (Diff revision 11)
     
     
    Show all issues
    You can do:
    
    if field in ('screenshot_captions, 'file_captions'):
  61. 
      
BO
BO
chipx86
  1. This looks fine with the exception being the one I noted about the Manager (I don't mind this being a separate cleanup change on top of this), and the unit test failures where Action.submitter doesn't allow null. That needs to be fixed before this goes in.
    
    I'll be committing all these changes when done onto an action-feed branch, which will later be merged into master.
  2. 
      
BO
  1. Continued with Review:3292.
  2. 
      
BO
Review request changed
Status:
Discarded