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

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

Bogdana Popa
Review Board
reviewboard
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.
  • 0
  • 0
  • 116
  • 4
  • 120
Description From Last Updated
Yazan Medanat
  1. I know its a WIP. Quick review, mostly style issues.
  2. reviewboard/reviews/admin.py (Diff revision 1)
     
     
    Alphabetical order.
  3. reviewboard/reviews/admin.py (Diff revision 1)
     
     
    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)
     
     
     
     
     
     
     
     
     
    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)
     
     
     
     
     
    intermediate variable again.
  9. reviewboard/reviews/models.py (Diff revision 1)
     
     
     
     
    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)
     
     
    Alphabetical order.
  12. reviewboard/reviews/views.py (Diff revision 1)
     
     
    Keep this empty line before the return.
  13. reviewboard/reviews/views.py (Diff revision 1)
     
     
    No need for this empty line.
  14. reviewboard/reviews/views.py (Diff revision 1)
     
     
    No need for this empty line.
  15. reviewboard/reviews/views.py (Diff revision 1)
     
     
    No need for this empty line.
  16. No need for this empty line.
  17. 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)
     
     
     
     
    No need for those empty lines.
  19. reviewboard/static/rb/css/action-feed.less (Diff revision 1)
     
     
     
    No need for the empty lines here.
  20. reviewboard/templates/reviews/action_feed.html (Diff revision 1)
     
     
     
     
     
     
     
     
     
     
     
     
    Indent all by one.
  21. No need for this extra line.
  22. indent "compressed_css"
  23. reviewboard/templates/reviews/dashboard.html (Diff revision 1)
     
     
     
     
     
     
    indent "include" and "{{datagrid...}}"
  24. 
      
Bogdana Popa
Yazan Medanat
  1. A few more style issues.
  2. reviewboard/reviews/views.py (Diff revision 2)
     
     
    No need for space between 'view' and :
  3. reviewboard/settings.py (Diff revision 2)
     
     
    Remove space before ':'
  4. reviewboard/settings.py (Diff revision 2)
     
     
    Space after ':'
  5. reviewboard/settings.py (Diff revision 2)
     
     
     
    Remove space before the ':'.
  6. reviewboard/templates/reviews/action_feed.html (Diff revision 2)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Indent inside the '{%', not outside.
    
    i.e.:
    
    {% for
    {%  if
    {%    definevar ...
    {%  endif
    {% endfor
  7. reviewboard/templates/reviews/action_feed.html (Diff revision 2)
     
     
     
     
     
    One more level of indentation here.
  8. Alphabetical order.
  9. No need for this empty line.
  10. reviewboard/templates/reviews/dashboard.html (Diff revision 2)
     
     
     
     
    Indent within the '{%'.
  11. reviewboard/templates/reviews/dashboard.html (Diff revision 2)
     
     
     
     
    Indent within the '{%'.
  12. reviewboard/templates/reviews/dashboard.html (Diff revision 2)
     
     
     
     
    Indent within the '{%'.
  13. 
      
Bogdana Popa
Bogdana Popa
Bogdana Popa
Yazan Medanat
  1. Quick review, mostly style.
  2. reviewboard/reviews/views.py (Diff revision 5)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    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)
     
     
     
    Add an empty line here.
  4. reviewboard/settings.py (Diff revision 5)
     
     
    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)
     
     
     
     
     
     
     
     
    Alphabetical order.
  6. Translate: Review request.
    
    Remove the trailing white space.
  7. reviewboard/templates/reviews/action_feed.html (Diff revision 5)
     
     
     
     
     
     
    You can shift the the {% %} logic all the way to the left. That saves on white space being sent to the client.
  8. Translate: Posted.
  9. Translate: Updated.
  10. Translate: Ago.
  11. 
      
Bogdana Popa
Bogdana Popa
Bogdana Popa
Bogdana Popa
Christian Hammond
  1. 
      
  2. reviewboard/reviews/admin.py (Diff revision 7)
     
     
    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)
     
     
     
     
     
    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)
     
     
     
     
     
     
    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)
     
     
     
     
     
     
    Same here.
  6. reviewboard/reviews/models.py (Diff revision 7)
     
     
     
     
     
    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)
     
     
    Was this intentional? It looks like a mistake.
  8. reviewboard/reviews/models.py (Diff revision 7)
     
     
     
     
     
     
  9. reviewboard/reviews/models.py (Diff revision 7)
     
     
     
     
    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)
     
     
     
     
    No blank line.
  11. reviewboard/reviews/models.py (Diff revision 7)
     
     
    Instead of this, do this above:
    
    
    @property
    def request_display_id(self):
        ...
  12. reviewboard/reviews/views.py (Diff revision 7)
     
     
    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)
     
     
     
     
    No blank line.
    
    This should subclass from object.
  14. reviewboard/reviews/views.py (Diff revision 7)
     
     
     
     
     
     
     
    Can you document why we need this?
  15. reviewboard/reviews/views.py (Diff revision 7)
     
     
    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)
     
     
    Add a trailing comma.
  17. reviewboard/reviews/views.py (Diff revision 7)
     
     
     
    Alignment is off.
  18. Space before {
  19. All indentation should be 2 lines instead of 4.
  20. Another case of {.
    
    Same with others in this file.
  21. reviewboard/static/rb/css/action-feed.less (Diff revision 7)
     
     
     
     
    No blank line.
  22. No spaces around =
  23. One space indentation.
  24. No spaces inside {{..}}
  25. 
      
Bogdana Popa
Bogdana Popa
Bogdana Popa
Christian Hammond
  1. 
      
  2. reviewboard/reviews/datagrids.py (Diff revision 11)
     
     
    Let's go ahead and use 'action-feed'. '-' is nicer in URLs.
  3. reviewboard/reviews/models.py (Diff revision 11)
     
     
     
    No blank line.
  4. reviewboard/reviews/models.py (Diff revision 11)
     
     
     
     
     
    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)
     
     
     
    No blank line.
  6. reviewboard/reviews/models.py (Diff revision 11)
     
     
     
     
     
    Should also check review_id and changedesc_id, and return some default.
  7. action-feed.
  8. reviewboard/reviews/views.py (Diff revision 11)
     
     
    Add a trailing comma.
  9. reviewboard/reviews/views.py (Diff revision 11)
     
     
     
    local_site_id
  10. reviewboard/static/rb/js/action-feed.js (Diff revision 11)
     
     
     
     
     
    Indentation looks wrong.
    
    This map should only be defined once, outside the function.
  11. reviewboard/static/rb/js/action-feed.js (Diff revision 11)
     
     
     
     
     
    You should only have one var statement, formatted like:
    
    
    var body = ...,
        request_url = ...;
  12. reviewboard/static/rb/js/action-feed.js (Diff revision 11)
     
     
    Best as:
    
    var changeitems = $('<ul/>')
        .appendTo(body);
  13. reviewboard/static/rb/js/action-feed.js (Diff revision 11)
     
     
    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)
     
     
     
     
    One var statemnet.
  15. reviewboard/static/rb/js/action-feed.js (Diff revision 11)
     
     
    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)
     
     
     
     
     
    Best done as:
    
    field_name = field_name_map[field] || field
  17. reviewboard/static/rb/js/action-feed.js (Diff revision 11)
     
     
    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)
     
     
    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)
     
     
    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)
     
     
     
    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)
     
     
    vars must be at the top of the scope.
  22. reviewboard/static/rb/js/action-feed.js (Diff revision 11)
     
     
    ===. Avoids a cast.
  23. reviewboard/static/rb/js/action-feed.js (Diff revision 11)
     
     
     
     
    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)
     
     
     
    No blank line.
  25. reviewboard/static/rb/js/action-feed.js (Diff revision 11)
     
     
     
    Braces.
    
    Why the length check?
  26. reviewboard/static/rb/js/action-feed.js (Diff revision 11)
     
     
     
     
    One var, top of scope.
  27. reviewboard/static/rb/js/action-feed.js (Diff revision 11)
     
     
     
     
     
     
    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)
     
     
    hasOwnProperty.
  29. reviewboard/static/rb/js/action-feed.js (Diff revision 11)
     
     
     
     
    Build with jQuery.
  30. reviewboard/static/rb/js/action-feed.js (Diff revision 11)
     
     
    hasOwnProperty.
  31. reviewboard/static/rb/js/action-feed.js (Diff revision 11)
     
     
     
    Build with jQuery.
  32. reviewboard/static/rb/js/action-feed.js (Diff revision 11)
     
     
     
    Blank line.
  33. reviewboard/static/rb/js/action-feed.js (Diff revision 11)
     
     
    hasOwnProperty.
  34. reviewboard/static/rb/js/action-feed.js (Diff revision 11)
     
     
     
     
     
     
    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)
     
     
     
    Maybe oldValue/newValue?
  36. reviewboard/static/rb/js/action-feed.js (Diff revision 11)
     
     
     
    All this should be built with jQuery.
  37. reviewboard/static/rb/js/action-feed.js (Diff revision 11)
     
     
     
    ===
    
    Alignment is wrong.
  38. reviewboard/static/rb/js/action-feed.js (Diff revision 11)
     
     
    Move var out of the for, since it's really going to be promoted to the top of the "else if" block anyway.
  39. reviewboard/static/rb/js/action-feed.js (Diff revision 11)
     
     
     
    Build with jQuery.
    
    Make sure to always use braces.
  40. reviewboard/static/rb/js/action-feed.js (Diff revision 11)
     
     
    If so, maybe we should log that.
  41. reviewboard/static/rb/js/action-feed.js (Diff revision 11)
     
     
     
    Blank line.
  42. reviewboard/static/rb/js/action-feed.js (Diff revision 11)
     
     
     
     
     
    Build with jQuery.
  43. reviewboard/static/rb/js/action-feed.js (Diff revision 11)
     
     
     
    Blank line.
  44. reviewboard/static/rb/js/action-feed.js (Diff revision 11)
     
     
     
    Alignment is off.
    
    The whole function should build with jQuery.
  45. reviewboard/static/rb/js/action-feed.js (Diff revision 11)
     
     
     
  46. reviewboard/static/rb/js/action-feed.js (Diff revision 11)
     
     
     
    One var.
  47. reviewboard/static/rb/js/action-feed.js (Diff revision 11)
     
     
     
  48. reviewboard/static/rb/js/action-feed.js (Diff revision 11)
     
     
     
    Alignment is off.
  49. reviewboard/static/rb/js/action-feed.js (Diff revision 11)
     
     
     
    $('<dl/>')
        .appendTo(body);
  50. reviewboard/static/rb/js/action-feed.js (Diff revision 11)
     
     
     
     
    One var.
  51. reviewboard/static/rb/js/action-feed.js (Diff revision 11)
     
     
    Move this into the declaration above, before my proposed appendTo.
  52. reviewboard/static/rb/js/action-feed.js (Diff revision 11)
     
     
    Move var up, and pull the length out.
  53. reviewboard/static/rb/js/action-feed.js (Diff revision 11)
     
     
     
     
     
    vars go first.
    
    comment can probably just be defined in each loop that needs it.
  54. reviewboard/static/rb/js/action-feed.js (Diff revision 11)
     
     
     
     
     
     
     
     
    Build with jQuery.
    
    These comments apply to everywhere else in the JavaScript.
  55. reviewboard/static/rb/js/datastore.js (Diff revision 11)
     
     
     
     
    No blank line.
  56. reviewboard/webapi/resources.py (Diff revision 11)
     
     
     
     
    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.
  57. reviewboard/webapi/resources.py (Diff revision 11)
     
     
     
     
     
     
     
    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.
  58. reviewboard/webapi/resources.py (Diff revision 11)
     
     
     
    You can combine these.
  59. reviewboard/webapi/resources.py (Diff revision 11)
     
     
    You can do:
    
    if field in ('screenshot_captions, 'file_captions'):
  60. 
      
Bogdana Popa
Bogdana Popa
Christian Hammond
  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. 
      
Bogdana Popa
  1. Continued with Review:3292.
  2. 
      
Bogdana Popa
Review request changed

Status: Discarded

Loading...