[WIP] Add archived state to review requests

Review Request #6830 — Created Jan. 25, 2015 and discarded

Information

Review Board
master

Reviewers

Orig: January 25, 2015
- added visibility field to ReviewRequestVisit with visible (V) and archived (A) states
- added show_archived attribute to DashboardDataGrid
- added ReviewRequestVisitManager

January 31, 2015
- scrapped ReviewRequestVisitManager idea
- added a query that filters archived requests to ReviewRequestManager
- wrote and applied a evolution for visibility in the ReviewRequestVisit table

February 11, 2015
- added archive and mute buttons in template with handlers
- added a reviewRequestVisit JavaScript resource that talks to the webAPI
- began review_request_visit API

February 13, 2015
- scrapped review_request_visit API and decided on two separate archived_request_visit and muted_request_visit APIs to prevent visits from being exposed
- added archive/unarchive/mute/unmute functions in reviewRequestVisit model
- began archive banner

February 16, 2015
- url() and archive() in archivedReviewRequestModel.js
- added decorators in the archived_review_request webAPI resource

February 17, 2015
- finished implementing query that filters archived review requests
- added more functional hyperlink that toggles review request visibility with url query
- works as it should if there are no other existing url queries, but still need to figure out how to append to existing, non-"show-archived" url queries

February 19, 2015
- fixed archive query
- completed POST request for archiving
- made "Hide" button disappear when review request is already archived

February 28, 2015


 
Description From Last Updated

'ConcurrencyManager' imported but unused

reviewbotreviewbot

undefined name 'ReviewRequestVisit'

reviewbotreviewbot

The first line of this file should be from __future__ import unicode_literals. With that, you can change initial=u'V' to initial='V'.

daviddavid

Col: 80 E501 line too long (109 > 79 characters)

reviewbotreviewbot

Col: 2 W292 no newline at end of file

reviewbotreviewbot

You should revert the change to this line.

daviddavid

'ReviewRequestVisit' imported but unused

reviewbotreviewbot

You don't need the '' default in the get statement. It would return a None which would be a false …

VT VTL-Developer

I think that we want this to be a filter on the existing queryset (rather than replacing it entirely). Something …

daviddavid

Col: 5 E101 indentation contains mixed spaces and tabs

reviewbotreviewbot

Col: 5 W191 indentation contains tabs

reviewbotreviewbot

Col: 6 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

undefined name 'ReviewRequestVisit'

reviewbotreviewbot

Col: 1 W191 indentation contains tabs

reviewbotreviewbot

Col: 1 W191 indentation contains tabs

reviewbotreviewbot

Col: 1 W191 indentation contains tabs

reviewbotreviewbot

Col: 1 W191 indentation contains tabs

reviewbotreviewbot

Col: 1 W191 indentation contains tabs

reviewbotreviewbot

Col: 1 W191 indentation contains tabs

reviewbotreviewbot

Col: 1 W191 indentation contains tabs

reviewbotreviewbot

Col: 1 W191 indentation contains tabs

reviewbotreviewbot

Col: 1 W191 indentation contains tabs

reviewbotreviewbot

Col: 1 W191 indentation contains tabs

reviewbotreviewbot

Col: 1 W191 indentation contains tabs

reviewbotreviewbot

Col: 1 W191 indentation contains tabs

reviewbotreviewbot

Col: 1 W191 indentation contains tabs

reviewbotreviewbot

Col: 1 W191 indentation contains tabs

reviewbotreviewbot

Col: 61 W292 no newline at end of file

reviewbotreviewbot

Col: 9 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 33 W291 trailing whitespace

reviewbotreviewbot

'User' imported but unused

reviewbotreviewbot

'augment_method_from' imported but unused

reviewbotreviewbot

'webapi_check_local_site' imported but unused

reviewbotreviewbot

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

local variable 'user' is assigned to but never used

reviewbotreviewbot

local variable 'review_request' is assigned to but never used

reviewbotreviewbot

undefined name 'DOES_NOT_EXIST'

reviewbotreviewbot

local variable 'user' is assigned to but never used

reviewbotreviewbot

local variable 'review_request' is assigned to but never used

reviewbotreviewbot

undefined name 'DOES_NOT_EXIST'

reviewbotreviewbot

'augment_method_from' imported but unused

reviewbotreviewbot

'webapi_check_local_site' imported but unused

reviewbotreviewbot

'resources' imported but unused

reviewbotreviewbot

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

Col: 9 E128 continuation line under-indented for visual indent

reviewbotreviewbot

These sorts of methods don't really help that much. It's nicer to just have the calling code set the field …

daviddavid

I'd not sure if this is temporary test code or not, but we should just be passing filter_archived as a …

daviddavid

This isn't quite right. Please see my earlier suggestion at https://reviews.reviewboard.org/r/6830/#comment20262 In this context, it would end up looking something …

daviddavid

'User' imported but unused

reviewbotreviewbot

'augment_method_from' imported but unused

reviewbotreviewbot

Col: 60 E231 missing whitespace after ','

reviewbotreviewbot

local variable 'user' is assigned to but never used

reviewbotreviewbot

This should probably be more like: visit, is_new = ReviewRequestVisit.objects.get_or_create( user=user, review_request=review_request) visit.visible = ReviewRequestVisit.ARCHIVED visit.save()

daviddavid

undefined name 'archived_review_request'

reviewbotreviewbot

Col: 26 E225 missing whitespace around operator

reviewbotreviewbot

Col: 5 W604 backticks are deprecated, use 'repr()'

reviewbotreviewbot

Col: 7 E226 missing whitespace around arithmetic operator

reviewbotreviewbot

Col: 1 E112 expected an indented block

reviewbotreviewbot

Col: 9 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 9 E128 continuation line under-indented for visual indent

reviewbotreviewbot

This isn't used anymore.

daviddavid

Col: 19 E702 multiple statements on one line (semicolon)

reviewbotreviewbot

Should have the model name before it: "reviewrequestvisit_visibility" I don't see any profile settings in here?

chipx86chipx86

Blank line between these.

chipx86chipx86

Should use one of the constants instead of hard-coding V. I'm not sure we want to index this here. If …

chipx86chipx86

This is already defined in ReviewRequestDataGrid.

chipx86chipx86

I feel like we're doing a bunch of unnecessary work for this 'show_archived' flag. I also think we can't assume …

chipx86chipx86

Best named hidden_q or just queryset.

chipx86chipx86

Instead of the ~Q(...), use Q(visibility__ne=...)

chipx86chipx86

Should use the consatnt instead of "P".

chipx86chipx86

I don't think we need this anymore?

chipx86chipx86

Should be added alphabetically.

chipx86chipx86

Col: 19 E702 multiple statements on one line (semicolon)

reviewbotreviewbot

'Q' imported but unused

reviewbotreviewbot

Rather than using a continuation character here, can you put the whole expression in parens? if (review_request.public and review_request.status == …

daviddavid

These should probably be defined inside the UserSession object (right after this.watchedGroups = ...). We should also wrap these strings …

daviddavid

Same comment as above.

daviddavid

Same comment as above.

daviddavid

Same comment as above.

daviddavid

Same comment as above.

daviddavid

Same comment as above.

daviddavid

This should say "review request" instead of "change"

daviddavid

This should say "review request" instead of "change"

daviddavid

No blank line here (django and djblets are both "3rd party" libraries from Review Board's point of view).

daviddavid

Can you add docstrings for the class and method?

daviddavid

Please add visibility=self.model.ARCHIVED to the filter call (we don't want to update muted review requests).

daviddavid

Col: 21 E126 continuation line over-indented for hanging indent

reviewbotreviewbot

Please add a blank line after the docstring.

daviddavid

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

review_published and reply_published pass a Review instance as their argument rather than a ReviewRequest. You'll need a separate handler for …

daviddavid

Col: 80 E501 line too long (82 > 79 characters)

reviewbotreviewbot

Since you have parens, put the opening paren on the previous line instead of using \\

daviddavid

I think djblets.util.http.get_url_params_except will do what you want.

daviddavid

Col: 7 E111 indentation is not a multiple of four

reviewbotreviewbot

Please don't change the signatures of these, and instead have a separate handler.

daviddavid

I don't understand what the visibility attribute is used for. Can you explain?

daviddavid

Please use "review request" instead of "change"

daviddavid

Please use "review request" instead of "change"

daviddavid
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/datagrids/grids.py
        reviewboard/accounts/models.py
        reviewboard/reviews/managers.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/datagrids/grids.py
        reviewboard/accounts/models.py
        reviewboard/reviews/managers.py
    
    
  2. reviewboard/accounts/models.py (Diff revision 1)
     
     
    Show all issues
     'ConcurrencyManager' imported but unused
    
  3. reviewboard/datagrids/grids.py (Diff revision 1)
     
     
    Show all issues
     undefined name 'ReviewRequestVisit'
    
  4. 
      
CR
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/datagrids/grids.py
        reviewboard/accounts/evolutions/__init__.py
        reviewboard/accounts/evolutions/visibility.py
        reviewboard/accounts/models.py
        reviewboard/reviews/managers.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/datagrids/grids.py
        reviewboard/accounts/evolutions/__init__.py
        reviewboard/accounts/evolutions/visibility.py
        reviewboard/accounts/models.py
        reviewboard/reviews/managers.py
    
    
  2. Show all issues
    Col: 80
     E501 line too long (109 > 79 characters)
    
  3. Show all issues
    Col: 2
     W292 no newline at end of file
    
  4. reviewboard/datagrids/grids.py (Diff revision 2)
     
     
    Show all issues
     'ReviewRequestVisit' imported but unused
    
  5. 
      
CR
VT
  1. 
      
  2. reviewboard/datagrids/grids.py (Diff revision 2)
     
     
    Show all issues

    You don't need the '' default in the get statement. It would return a None which would be a false for your if statement.

  3. 
      
david
  1. 
      
  2. Show all issues

    The first line of this file should be from __future__ import unicode_literals. With that, you can change initial=u'V' to initial='V'.

  3. reviewboard/accounts/models.py (Diff revision 2)
     
     
    Show all issues

    You should revert the change to this line.

  4. reviewboard/datagrids/grids.py (Diff revision 2)
     
     
     
     
    Show all issues

    I think that we want this to be a filter on the existing queryset (rather than replacing it entirely). Something like this?

    if not show_archived:
        hidden = ReviewRequestVisit.objects.filter(
            user=user, visibility__ne=ReviewRequestVisit.VISIBLE)
        hidden = hidden.values_list('review_request_id', flat=True)
    
        self.queryset = self.queryset.exclude(pk__in=hidden)
    

    In fact, it might be even more appropriate to plumb the show_archived field through and do this at the bottom of ReviewRequestManager._query, which already has some similar code (which filters out review requests that are "private")

    1. For your latter suggestion, how would my ReviewRequestManager.archived method be able to catch any other URL parameters? Like if I wanted to see all review requests that are archived AND to-me, how would ReviewRequestManager.archived get ReviewrequestManager.to_user_directly's extra query?

  5. 
      
CR
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/accounts/evolutions/visibility.py
        reviewboard/accounts/evolutions/__init__.py
        reviewboard/datagrids/grids.py
        reviewboard/webapi/resources/review_request_visit.py
        reviewboard/accounts/models.py
        reviewboard/reviews/managers.py
    
    Ignored Files:
        reviewboard/static/rb/js/resources/models/reviewRequestVisitModel.js
        reviewboard/templates/reviews/review_request_actions_secondary.html
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/accounts/evolutions/visibility.py
        reviewboard/accounts/evolutions/__init__.py
        reviewboard/datagrids/grids.py
        reviewboard/webapi/resources/review_request_visit.py
        reviewboard/accounts/models.py
        reviewboard/reviews/managers.py
    
    Ignored Files:
        reviewboard/static/rb/js/resources/models/reviewRequestVisitModel.js
        reviewboard/templates/reviews/review_request_actions_secondary.html
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
    
    
  2. Show all issues
    Col: 5
     E101 indentation contains mixed spaces and tabs
    
  3. Show all issues
    Col: 5
     W191 indentation contains tabs
    
  4. Show all issues
    Col: 6
     E128 continuation line under-indented for visual indent
    
  5. Show all issues
    Col: 1
     E302 expected 2 blank lines, found 1
    
  6. Show all issues
     undefined name 'ReviewRequestVisit'
    
  7. Show all issues
    Col: 1
     W191 indentation contains tabs
    
  8. Show all issues
    Col: 1
     W191 indentation contains tabs
    
  9. Show all issues
    Col: 1
     W191 indentation contains tabs
    
  10. Show all issues
    Col: 1
     W191 indentation contains tabs
    
  11. Show all issues
    Col: 1
     W191 indentation contains tabs
    
  12. Show all issues
    Col: 1
     W191 indentation contains tabs
    
  13. Show all issues
    Col: 1
     W191 indentation contains tabs
    
  14. Show all issues
    Col: 1
     W191 indentation contains tabs
    
  15. Show all issues
    Col: 1
     W191 indentation contains tabs
    
  16. Show all issues
    Col: 1
     W191 indentation contains tabs
    
  17. Show all issues
    Col: 1
     W191 indentation contains tabs
    
  18. Show all issues
    Col: 1
     W191 indentation contains tabs
    
  19. Show all issues
    Col: 1
     W191 indentation contains tabs
    
  20. Show all issues
    Col: 1
     W191 indentation contains tabs
    
  21. Show all issues
    Col: 61
     W292 no newline at end of file
    
  22. 
      
CR
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/managers.py
        reviewboard/accounts/evolutions/visibility.py
        reviewboard/accounts/evolutions/__init__.py
        reviewboard/datagrids/grids.py
        reviewboard/webapi/resources/muted_review_request.py
        reviewboard/accounts/models.py
        reviewboard/staticbundles.py
        reviewboard/webapi/resources/user.py
        reviewboard/webapi/resources/archived_review_request.py
    
    Ignored Files:
        reviewboard/templates/reviews/review_request_actions_secondary.html
        reviewboard/static/rb/js/resources/models/archivedReviewRequestModel.js
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/managers.py
        reviewboard/accounts/evolutions/visibility.py
        reviewboard/accounts/evolutions/__init__.py
        reviewboard/datagrids/grids.py
        reviewboard/webapi/resources/muted_review_request.py
        reviewboard/accounts/models.py
        reviewboard/staticbundles.py
        reviewboard/webapi/resources/user.py
        reviewboard/webapi/resources/archived_review_request.py
    
    Ignored Files:
        reviewboard/templates/reviews/review_request_actions_secondary.html
        reviewboard/static/rb/js/resources/models/archivedReviewRequestModel.js
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
    
    
  2. Show all issues
    Col: 9
     E128 continuation line under-indented for visual indent
    
  3. reviewboard/datagrids/grids.py (Diff revision 4)
     
     
    Show all issues
    Col: 33
     W291 trailing whitespace
    
  4. Show all issues
     'User' imported but unused
    
  5. Show all issues
     'augment_method_from' imported but unused
    
  6. Show all issues
     'webapi_check_local_site' imported but unused
    
  7. Show all issues
    Col: 1
     E302 expected 2 blank lines, found 1
    
  8. Show all issues
     local variable 'user' is assigned to but never used
    
  9. Show all issues
     local variable 'review_request' is assigned to but never used
    
  10. Show all issues
     undefined name 'DOES_NOT_EXIST'
    
  11. Show all issues
     local variable 'user' is assigned to but never used
    
  12. Show all issues
     local variable 'review_request' is assigned to but never used
    
  13. Show all issues
     undefined name 'DOES_NOT_EXIST'
    
  14. Show all issues
     'augment_method_from' imported but unused
    
  15. Show all issues
     'webapi_check_local_site' imported but unused
    
  16. Show all issues
     'resources' imported but unused
    
  17. Show all issues
    Col: 1
     E302 expected 2 blank lines, found 1
    
  18. 
      
CR
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/managers.py
        reviewboard/accounts/evolutions/visibility.py
        reviewboard/accounts/evolutions/__init__.py
        reviewboard/datagrids/grids.py
        reviewboard/webapi/resources/muted_review_request.py
        reviewboard/accounts/models.py
        reviewboard/staticbundles.py
        reviewboard/webapi/resources/user.py
        reviewboard/webapi/resources/archived_review_request.py
    
    Ignored Files:
        reviewboard/templates/reviews/review_request_actions_secondary.html
        reviewboard/static/rb/js/resources/models/archivedReviewRequestModel.js
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/managers.py
        reviewboard/accounts/evolutions/visibility.py
        reviewboard/accounts/evolutions/__init__.py
        reviewboard/datagrids/grids.py
        reviewboard/webapi/resources/muted_review_request.py
        reviewboard/accounts/models.py
        reviewboard/staticbundles.py
        reviewboard/webapi/resources/user.py
        reviewboard/webapi/resources/archived_review_request.py
    
    Ignored Files:
        reviewboard/templates/reviews/review_request_actions_secondary.html
        reviewboard/static/rb/js/resources/models/archivedReviewRequestModel.js
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
    
    
  2. Show all issues
    Col: 9
     E128 continuation line under-indented for visual indent
    
  3. Show all issues
     'User' imported but unused
    
  4. Show all issues
     'augment_method_from' imported but unused
    
  5. Show all issues
    Col: 60
     E231 missing whitespace after ','
    
  6. Show all issues
     local variable 'user' is assigned to but never used
    
  7. Show all issues
     undefined name 'archived_review_request'
    
  8. 
      
david
  1. I haven't looked too much at the javascript code because it looks very preliminary.

    I'd suggest setting the archive flag for a review request in the database admin and using that to test that the dashboard queries are working correctly.

  2. reviewboard/accounts/models.py (Diff revision 5)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    These sorts of methods don't really help that much. It's nicer to just have the calling code set the field directly and call save() when appropriate.

  3. reviewboard/datagrids/grids.py (Diff revision 5)
     
     
     
     
    Show all issues

    I'd not sure if this is temporary test code or not, but we should just be passing filter_archived as a parameter to the existing ReviewRequest.objects.*() methods, rather than replacing the queryset. With this as-is, all the pages of the dashboard will look the same.

  4. reviewboard/reviews/managers.py (Diff revision 5)
     
     
     
    Show all issues

    This isn't quite right. Please see my earlier suggestion at https://reviews.reviewboard.org/r/6830/#comment20262

    In this context, it would end up looking something like this:

    query = self.filter(query).distinct()
    
    if filter_archived:
        hidden = ReviewRequestVisit.objects.filter(
            user=user, visibility__ne=ReviewRequestVisit.VISIBLE)
        hidden = hidden.values_list('review_request_id', flat=True)
    
        query = query.exclude(pk__in=hidden)
    
    if with_counts:
    ...
    
    1. Sorry, only just saw this and I had since found another solution (which seems to be working, please see diff revision 6), but please let me know if I should revert to what you posted in this review.

    2. Please let me know if revision 6 is okay. I put the query directly into load_extra_state in grids.py, which avoids having to put "filter_archived=self.filter_archived" in every ReviewRequest.objects.()* method

    3. So putting it into load_extra_state would work, but the query you have (self.queryset.filter(**{ self.visibility_query_field: 'V', })) won't do the right thing because of the particular kind of join we need to do. Just querying visits__visibility won't take into account the user relation on the ReviewRequestVisit model, and we need to do that. Do you understand what my above code snippit is doing and how it differs from what you have?

    4. Oh okay, yes I see the difference. I'll have it in my next diff

  5. Show all issues

    This should probably be more like:

    visit, is_new = ReviewRequestVisit.objects.get_or_create(
        user=user, review_request=review_request)
    visit.visible = ReviewRequestVisit.ARCHIVED
    visit.save()
    
    1. Thanks, this makes things a lot easier. One thing though, for some reason, create isn't even being reached when I try to run things in pdb. From my understanding, I'm calling it with this.save in archiveRequestModel.js, is this correct?

  6. 
      
CR
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/accounts/evolutions/visibility.py
        profile_show_archived.py
        reviewboard/accounts/evolutions/__init__.py
        reviewboard/datagrids/grids.py
        reviewboard/webapi/resources/muted_review_request.py
        reviewboard/accounts/models.py
        reviewboard/reviews/managers.py
        reviewboard/staticbundles.py
        reviewboard/webapi/resources/user.py
        reviewboard/webapi/resources/archived_review_request.py
    
    Ignored Files:
        reviewboard/static/rb/js/resources/models/archivedReviewRequestModel.js
        reviewboard/templates/reviews/review_request_actions_secondary.html
        reviewboard/static/rb/css/common.less
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
    
    
  2. profile_show_archived.py (Diff revision 6)
     
     
    Show all issues
    Col: 26
     E225 missing whitespace around operator
    
  3. profile_show_archived.py (Diff revision 6)
     
     
    Show all issues
    Col: 5
     W604 backticks are deprecated, use 'repr()'
    
  4. profile_show_archived.py (Diff revision 6)
     
     
    Show all issues
    Col: 7
     E226 missing whitespace around arithmetic operator
    
  5. profile_show_archived.py (Diff revision 6)
     
     
    Show all issues
    Col: 1
     E112 expected an indented block
    
  6. Show all issues
    Col: 9
     E128 continuation line under-indented for visual indent
    
  7. 
      
CR
CR
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/accounts/evolutions/visibility.py
        reviewboard/accounts/evolutions/__init__.py
        reviewboard/datagrids/grids.py
        reviewboard/webapi/resources/muted_review_request.py
        reviewboard/accounts/models.py
        reviewboard/staticbundles.py
        reviewboard/webapi/resources/user.py
        reviewboard/webapi/resources/archived_review_request.py
    
    Ignored Files:
        reviewboard/static/rb/js/resources/models/archivedReviewRequestModel.js
        reviewboard/templates/reviews/review_request_actions_secondary.html
        reviewboard/static/rb/css/common.less
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/accounts/evolutions/visibility.py
        reviewboard/accounts/evolutions/__init__.py
        reviewboard/datagrids/grids.py
        reviewboard/webapi/resources/muted_review_request.py
        reviewboard/accounts/models.py
        reviewboard/staticbundles.py
        reviewboard/webapi/resources/user.py
        reviewboard/webapi/resources/archived_review_request.py
    
    Ignored Files:
        reviewboard/static/rb/js/resources/models/archivedReviewRequestModel.js
        reviewboard/templates/reviews/review_request_actions_secondary.html
        reviewboard/static/rb/css/common.less
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
    
    
  2. Show all issues
    Col: 9
     E128 continuation line under-indented for visual indent
    
  3. 
      
david
  1. 
      
  2. reviewboard/datagrids/grids.py (Diff revision 7)
     
     
    Show all issues

    This isn't used anymore.

  3. 
      
CR
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/accounts/evolutions/visibility.py
        reviewboard/accounts/evolutions/__init__.py
        reviewboard/datagrids/grids.py
        reviewboard/webapi/resources/muted_review_request.py
        reviewboard/accounts/models.py
        reviewboard/staticbundles.py
        reviewboard/webapi/resources/user.py
        reviewboard/webapi/resources/archived_review_request.py
    
    Ignored Files:
        reviewboard/templates/reviews/reviewable_page_data.js
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
        reviewboard/templates/reviews/review_header.html
        reviewboard/static/rb/css/common.less
        reviewboard/templates/reviews/review_request_actions_secondary.html
        reviewboard/static/rb/js/resources/models/archivedReviewRequestModel.js
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/accounts/evolutions/visibility.py
        reviewboard/accounts/evolutions/__init__.py
        reviewboard/datagrids/grids.py
        reviewboard/webapi/resources/muted_review_request.py
        reviewboard/accounts/models.py
        reviewboard/staticbundles.py
        reviewboard/webapi/resources/user.py
        reviewboard/webapi/resources/archived_review_request.py
    
    Ignored Files:
        reviewboard/templates/reviews/reviewable_page_data.js
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
        reviewboard/templates/reviews/review_header.html
        reviewboard/static/rb/css/common.less
        reviewboard/templates/reviews/review_request_actions_secondary.html
        reviewboard/static/rb/js/resources/models/archivedReviewRequestModel.js
    
    
  2. Show all issues
    Col: 19
     E702 multiple statements on one line (semicolon)
    
  3. 
      
CR
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/accounts/evolutions/visibility.py
        reviewboard/accounts/evolutions/__init__.py
        reviewboard/datagrids/grids.py
        reviewboard/webapi/resources/muted_review_request.py
        reviewboard/accounts/models.py
        reviewboard/staticbundles.py
        reviewboard/webapi/resources/user.py
        reviewboard/webapi/resources/archived_review_request.py
    
    Ignored Files:
        reviewboard/templates/base.html
        reviewboard/static/rb/js/resources/models/archivedReviewRequestModel.js
        reviewboard/templates/reviews/reviewable_page_data.js
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
        reviewboard/static/rb/css/common.less
        reviewboard/templates/reviews/review_header.html
        reviewboard/templates/reviews/review_request_actions_secondary.html
        reviewboard/static/rb/js/resources/models/baseResourceModel.js
        reviewboard/static/rb/js/models/userSessionModel.js
    
    
  2. Show all issues
    Col: 19
     E702 multiple statements on one line (semicolon)
    
  3. 
      
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/accounts/evolutions/visibility.py
        reviewboard/accounts/evolutions/__init__.py
        reviewboard/datagrids/grids.py
        reviewboard/webapi/resources/muted_review_request.py
        reviewboard/accounts/models.py
        reviewboard/staticbundles.py
        reviewboard/webapi/resources/user.py
        reviewboard/webapi/resources/archived_review_request.py
    
    Ignored Files:
        reviewboard/templates/base.html
        reviewboard/static/rb/js/resources/models/archivedReviewRequestModel.js
        reviewboard/templates/reviews/reviewable_page_data.js
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
        reviewboard/static/rb/css/common.less
        reviewboard/templates/reviews/review_header.html
        reviewboard/templates/reviews/review_request_actions_secondary.html
        reviewboard/static/rb/js/resources/models/baseResourceModel.js
        reviewboard/static/rb/js/models/userSessionModel.js
    
    
  2. 
      
chipx86
  1. 
      
  2. Show all issues

    Should have the model name before it: "reviewrequestvisit_visibility"

    I don't see any profile settings in here?

  3. Show all issues

    Blank line between these.

  4. reviewboard/accounts/models.py (Diff revision 9)
     
     
    Show all issues

    Should use one of the constants instead of hard-coding V.

    I'm not sure we want to index this here. If there's 500 archived review requests, then the index will contain 500 entries under 'A', which isn't that useful. The user's own archived ones will still need to be searched within that 500.

    If we want to index something, it's probably best to do an index_together = ('user', 'visibility').

  5. reviewboard/datagrids/grids.py (Diff revision 9)
     
     
    Show all issues

    This is already defined in ReviewRequestDataGrid.

  6. reviewboard/datagrids/grids.py (Diff revision 9)
     
     
     
     
     
    Show all issues

    I feel like we're doing a bunch of unnecessary work for this 'show_archived' flag. I also think we can't assume that profile is defined here yet. Also, we need to record whether we've changed a profile setting.

    How about we instead do:

    profile_changed = False
    
    ...
    
    default_show_archived = (profile and profile.extra_data.get('show_archived', False))
    
    try:
        self.show_archived = ...
    except ValueError:
        self.show_archived = default_show_archived
    
    ...
    
    if profile and self.show_archived != default_show_archived:
        profile.extra_data[...] = ...
        profile_changed = True
    
    parent_profile_changed = super(...).load_extra_state(...)
    
    return profile_changed or parent_profile_changed
    
  7. reviewboard/datagrids/grids.py (Diff revision 9)
     
     
     
     
    Show all issues

    Best named hidden_q or just queryset.

  8. reviewboard/datagrids/grids.py (Diff revision 9)
     
     
    Show all issues

    Instead of the ~Q(...), use Q(visibility__ne=...)

    1. This is giving me a FieldError: Join on field 'visibility' not permitted. Did you misspell 'ne' for the lookup type?

    2. Unfortunately Christian is making up the __ne operator (see https://code.djangoproject.com/ticket/5763).

      This could be ReviewRequestVisit.filter(user=user).exclude(visibility=ReviewRequestVisit.VISIBLE), though.

  9. reviewboard/reviews/views.py (Diff revision 9)
     
     
    Show all issues

    Should use the consatnt instead of "P".

  10. Show all issues

    I don't think we need this anymore?

  11. reviewboard/staticbundles.py (Diff revision 9)
     
     
    Show all issues

    Should be added alphabetically.

  12. 
      
CR
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/webapi/resources/muted_review_request.py
        reviewboard/accounts/evolutions/__init__.py
        reviewboard/accounts/evolutions/reviewrequestvisit_visibility.py
        reviewboard/datagrids/grids.py
        reviewboard/accounts/models.py
        reviewboard/webapi/resources/user.py
        reviewboard/webapi/resources/archived_review_request.py
    
    Ignored Files:
        reviewboard/static/rb/js/resources/models/reviewGroupModel.js
        reviewboard/templates/base.html
        reviewboard/templates/reviews/review_header.html
        reviewboard/templates/reviews/reviewable_page_data.js
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
        reviewboard/static/rb/css/pages/reviews.less
        reviewboard/static/rb/js/resources/models/reviewRequestModel.js
        reviewboard/static/rb/css/common.less
        reviewboard/templates/reviews/review_request_actions_secondary.html
        reviewboard/static/rb/js/models/userSessionModel.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/webapi/resources/muted_review_request.py
        reviewboard/accounts/evolutions/__init__.py
        reviewboard/accounts/evolutions/reviewrequestvisit_visibility.py
        reviewboard/datagrids/grids.py
        reviewboard/accounts/models.py
        reviewboard/webapi/resources/user.py
        reviewboard/webapi/resources/archived_review_request.py
    
    Ignored Files:
        reviewboard/static/rb/js/resources/models/reviewGroupModel.js
        reviewboard/templates/base.html
        reviewboard/templates/reviews/review_header.html
        reviewboard/templates/reviews/reviewable_page_data.js
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
        reviewboard/static/rb/css/pages/reviews.less
        reviewboard/static/rb/js/resources/models/reviewRequestModel.js
        reviewboard/static/rb/css/common.less
        reviewboard/templates/reviews/review_request_actions_secondary.html
        reviewboard/static/rb/js/models/userSessionModel.js
    
    
  2. reviewboard/datagrids/grids.py (Diff revision 10)
     
     
    Show all issues
     'Q' imported but unused
    
  3. 
      
david
  1. This is getting to be a pretty big change. How would you feel about splitting this up into several review requests? One to add the database stuff, one for the dashboard queries, one to add API, and one for frontend?

  2. reviewboard/reviews/views.py (Diff revision 10)
     
     
     
    Show all issues

    Rather than using a continuation character here, can you put the whole expression in parens?

    if (review_request.public and
        review_request.status == review_request.PENDING_REVIEW):
    
  3. Show all issues

    These should probably be defined inside the UserSession object (right after this.watchedGroups = ...). We should also wrap these strings in gettext().

  4. Show all issues

    Same comment as above.

  5. Show all issues

    Same comment as above.

  6. Show all issues

    Same comment as above.

  7. Show all issues

    Same comment as above.

  8. Show all issues

    Same comment as above.

  9. Show all issues

    This should say "review request" instead of "change"

  10. Show all issues

    This should say "review request" instead of "change"

  11. 
      
CR
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/datagrids/views.py
        reviewboard/webapi/resources/muted_review_request.py
        reviewboard/datagrids/grids.py
        reviewboard/accounts/evolutions/__init__.py
        reviewboard/accounts/evolutions/reviewrequestvisit_visibility.py
        reviewboard/accounts/managers.py
        reviewboard/accounts/models.py
        reviewboard/webapi/resources/user.py
        reviewboard/webapi/resources/archived_review_request.py
        reviewboard/reviews/signals.py
        reviewboard/reviews/models/review.py
    
    Ignored Files:
        reviewboard/templates/base.html
        reviewboard/templates/reviews/review_header.html
        reviewboard/templates/datagrids/hideable_review_request_listview.html
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
        reviewboard/static/rb/css/pages/reviews.less
        reviewboard/templates/reviews/reviewable_page_data.js
        reviewboard/static/rb/css/common.less
        reviewboard/templates/reviews/review_request_actions_secondary.html
        reviewboard/static/rb/js/models/userSessionModel.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/datagrids/views.py
        reviewboard/webapi/resources/muted_review_request.py
        reviewboard/datagrids/grids.py
        reviewboard/accounts/evolutions/__init__.py
        reviewboard/accounts/evolutions/reviewrequestvisit_visibility.py
        reviewboard/accounts/managers.py
        reviewboard/accounts/models.py
        reviewboard/webapi/resources/user.py
        reviewboard/webapi/resources/archived_review_request.py
        reviewboard/reviews/signals.py
        reviewboard/reviews/models/review.py
    
    Ignored Files:
        reviewboard/templates/base.html
        reviewboard/templates/reviews/review_header.html
        reviewboard/templates/datagrids/hideable_review_request_listview.html
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
        reviewboard/static/rb/css/pages/reviews.less
        reviewboard/templates/reviews/reviewable_page_data.js
        reviewboard/static/rb/css/common.less
        reviewboard/templates/reviews/review_request_actions_secondary.html
        reviewboard/static/rb/js/models/userSessionModel.js
    
    
  2. reviewboard/accounts/managers.py (Diff revision 11)
     
     
    Show all issues
    Col: 21
     E126 continuation line over-indented for hanging indent
    
  3. reviewboard/accounts/models.py (Diff revision 11)
     
     
    Show all issues
    Col: 1
     E302 expected 2 blank lines, found 1
    
  4. reviewboard/datagrids/grids.py (Diff revision 11)
     
     
    Show all issues
    Col: 80
     E501 line too long (82 > 79 characters)
    
  5. reviewboard/datagrids/views.py (Diff revision 11)
     
     
    Show all issues
    Col: 7
     E111 indentation is not a multiple of four
    
  6. 
      
david
  1. 
      
  2. reviewboard/accounts/managers.py (Diff revision 11)
     
     
    Show all issues

    No blank line here (django and djblets are both "3rd party" libraries from Review Board's point of view).

  3. reviewboard/accounts/managers.py (Diff revision 11)
     
     
     
    Show all issues

    Can you add docstrings for the class and method?

  4. reviewboard/accounts/managers.py (Diff revision 11)
     
     
    Show all issues

    Please add visibility=self.model.ARCHIVED to the filter call (we don't want to update muted review requests).

  5. reviewboard/accounts/models.py (Diff revision 11)
     
     
    Show all issues

    Please add a blank line after the docstring.

  6. reviewboard/accounts/models.py (Diff revision 11)
     
     
    Show all issues

    review_published and reply_published pass a Review instance as their argument rather than a ReviewRequest. You'll need a separate handler for those signals which accesses review.review_request

  7. reviewboard/datagrids/grids.py (Diff revision 11)
     
     
     
    Show all issues

    Since you have parens, put the opening paren on the previous line instead of using \\

  8. reviewboard/datagrids/views.py (Diff revision 11)
     
     
     
     
    Show all issues

    I think djblets.util.http.get_url_params_except will do what you want.

  9. reviewboard/reviews/signals.py (Diff revision 11)
     
     
     
     
     
     
    Show all issues

    Please don't change the signatures of these, and instead have a separate handler.

    1. I don't think I understand how parameters are passed. I get a TypeError: _call_unarchive_all_for_review() takes exactly 2 arguments (1 given) in my @receiver that looks like:

      @receiver([review_published, reply_published])
      def _call_unarchive_all_for_review(sender, review, **kwargs):
      ReviewRequestVisit.objects.unarchive_all(review.review_request)

    2. nevermind, I got it to work it with separate handlers. The only thing now is that replying does not auto-refresh, so the "archived" banner does not disappear when you reply to something, only when the page is refreshed, is this okay?

  10. Show all issues

    I don't understand what the visibility attribute is used for. Can you explain?

    1. visibility is used on line 75 to distinguish between watched items and hidden items for the trigger

  11. Show all issues

    Please use "review request" instead of "change"

  12. Show all issues

    Please use "review request" instead of "change"

  13. 
      
CR
Review request changed
Status:
Discarded