Disable front-end features for read-only mode

Review Request #8810 - Created March 11, 2017 and updated

Kanghee Park
Review Board
master
8657, 8648
8861, 8811, 8812, 8824, 8847
reviewboard, students

Read-only mode is a setting an admin can enable to prevent writes to
the database. This can be used when the site is under maintenence or
being upgraded. This commit disables most UI features which would
normally affect Review Board, but under read-only mode, does not.
Disabling some features such as login and redirection when attempting
to view a read-only restricted page is contained in the next commit.

This commit contains changes to the datagrid so changes to it does not
change profile settings, navbar (hide My Account and New Review Request
buttons), ETag, ReviewRequestEditor(including actions, comments, and
the editor itself),

Other changes include adding read-only mode setting to the context
processor and adding a utility file containing the logic to determine
whether a user is in read-only mode or not.

  • Check that caching of read-only mode is done correctly by changing
    modes and refreshing pages
  • Check that profile doesn't save in datagrid when in read-only mode by
    ensuring code doesn't reach the save method
  • Check that the review request actions available are appropriately
    changed when in read-only mode
  • Check that comments can't be added or replied to in read-only mode
  • Check that review requests can not be modified in read-only mode
  • Check that modifying review requests (such as bugs or branch) when in
    read-only mode while the site in browser is still old displayed an
    appropriate error message
  • Check that modifying anything to the webapi doesn't send a request at
    all when in read-only mode
  • Check that trying to add a comment through clicking on a line in diff
    displays appropriate warning about read-only mode
  • Check that clicking on the Star does nothing in read-only mode
  • Check that the above does not occur for admins
  • 0
  • 0
  • 32
  • 13
  • 45
Description From Last Updated
Review Bot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/admin/read_only.py
        reviewboard/reviews/models/base_comment.py
        reviewboard/webapi/base.py
        reviewboard/reviews/default_actions.py
        reviewboard/reviews/models/review_request.py
        reviewboard/datagrids/grids.py
        reviewboard/admin/context_processors.py
        reviewboard/settings.py
        reviewboard/reviews/templatetags/reviewtags.py
    
    Ignored Files:
        reviewboard/static/rb/js/pages/views/dashboardView.js
        reviewboard/static/rb/js/views/starManagerView.js
        reviewboard/templates/reviews/review_reply_section.html
        reviewboard/templates/base/navbar.html
        reviewboard/static/rb/js/utils/apiUtils.es6.js
        reviewboard/templates/base.html
        reviewboard/templates/base/_nav_support_menu.html
        reviewboard/static/rb/js/models/userSessionModel.js
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
        reviewboard/static/rb/js/models/reviewRequestEditorModel.js
        reviewboard/static/rb/js/views/commentDialogView.js
        reviewboard/static/rb/js/models/commentEditorModel.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/admin/read_only.py
        reviewboard/reviews/models/base_comment.py
        reviewboard/webapi/base.py
        reviewboard/reviews/default_actions.py
        reviewboard/reviews/models/review_request.py
        reviewboard/datagrids/grids.py
        reviewboard/admin/context_processors.py
        reviewboard/settings.py
        reviewboard/reviews/templatetags/reviewtags.py
    
    Ignored Files:
        reviewboard/static/rb/js/pages/views/dashboardView.js
        reviewboard/static/rb/js/views/starManagerView.js
        reviewboard/templates/reviews/review_reply_section.html
        reviewboard/templates/base/navbar.html
        reviewboard/static/rb/js/utils/apiUtils.es6.js
        reviewboard/templates/base.html
        reviewboard/templates/base/_nav_support_menu.html
        reviewboard/static/rb/js/models/userSessionModel.js
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
        reviewboard/static/rb/js/models/reviewRequestEditorModel.js
        reviewboard/static/rb/js/views/commentDialogView.js
        reviewboard/static/rb/js/models/commentEditorModel.js
    
    
  2. reviewboard/reviews/views.py (Diff revision 1)
     
     
     'reverse' imported but unused
    
  3. reviewboard/settings.py (Diff revision 1)
     
     
     'django_reset' imported but unused
    
  4. reviewboard/settings.py (Diff revision 1)
     
     
     'from settings_local import *' used; unable to detect undefined names
    
  5. 
      
Kanghee Park
Kanghee Park
Review Bot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/admin/read_only.py
        reviewboard/reviews/models/base_comment.py
        reviewboard/webapi/base.py
        reviewboard/reviews/default_actions.py
        reviewboard/reviews/models/review_request.py
        reviewboard/datagrids/grids.py
        reviewboard/admin/context_processors.py
        reviewboard/settings.py
        reviewboard/reviews/templatetags/reviewtags.py
    
    Ignored Files:
        reviewboard/static/rb/js/pages/views/dashboardView.js
        reviewboard/static/rb/js/views/starManagerView.js
        reviewboard/templates/reviews/review_reply_section.html
        reviewboard/templates/base/navbar.html
        reviewboard/static/rb/js/utils/apiUtils.es6.js
        reviewboard/templates/base.html
        reviewboard/templates/base/_nav_support_menu.html
        reviewboard/static/rb/js/models/userSessionModel.js
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
        reviewboard/static/rb/js/models/reviewRequestEditorModel.js
        reviewboard/static/rb/js/views/commentDialogView.js
        reviewboard/static/rb/js/models/commentEditorModel.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/admin/read_only.py
        reviewboard/reviews/models/base_comment.py
        reviewboard/webapi/base.py
        reviewboard/reviews/default_actions.py
        reviewboard/reviews/models/review_request.py
        reviewboard/datagrids/grids.py
        reviewboard/admin/context_processors.py
        reviewboard/settings.py
        reviewboard/reviews/templatetags/reviewtags.py
    
    Ignored Files:
        reviewboard/static/rb/js/pages/views/dashboardView.js
        reviewboard/static/rb/js/views/starManagerView.js
        reviewboard/templates/reviews/review_reply_section.html
        reviewboard/templates/base/navbar.html
        reviewboard/static/rb/js/utils/apiUtils.es6.js
        reviewboard/templates/base.html
        reviewboard/templates/base/_nav_support_menu.html
        reviewboard/static/rb/js/models/userSessionModel.js
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
        reviewboard/static/rb/js/models/reviewRequestEditorModel.js
        reviewboard/static/rb/js/views/commentDialogView.js
        reviewboard/static/rb/js/models/commentEditorModel.js
    
    
  2. reviewboard/settings.py (Diff revision 2)
     
     
     'django_reset' imported but unused
    
  3. reviewboard/settings.py (Diff revision 2)
     
     
     'from settings_local import *' used; unable to detect undefined names
    
  4. 
      
Simon Zhang
  1. Mostly just small indentation issues. :)

  2. reviewboard/reviews/default_actions.py (Diff revision 2)
     
     

    Indent this by two more spaces?

    1. See the comment below, but the not is supposed to be inline with the first open parens.

  3. reviewboard/reviews/default_actions.py (Diff revision 2)
     
     

    Indent this by one space as well.

    1. See the other comments re: indentation.

  4. Indent this by one space here?

    1. Perhaps I can move the not ... to be the first term of this expression, but these have to be aligned with the inner-most open parenthesis for the value/subexpression. So here, the not is in-line with the first parenthesis right after return.

    2. Ah! My mistake. Too many opening/closing brackets confuse me. Sorry for the bad review.

      I'd say you don't need to move the not ... just to make the indentation a little more clear to people like me. :)

  5. Move this after the if statement?

    Putting it before the if statement subtly gives the impression that the if statement uses it--which it doesn't.

    1. Hey Simon, I believe the coding style we use is to declare all variables at the top of the function.

  6. 
      
Barret Rennie
  1. 
      
  2. reviewboard/admin/context_processors.py (Diff revision 2)
     
     

    Missing args/returns.

  3. reviewboard/admin/read_only.py (Diff revision 2)
     
     

    If you use the following, it will link to the class

    "the :py:class:`~djblets.siteconfig.models.SiteConfiguration`"
    
  4. reviewboard/admin/read_only.py (Diff revision 2)
     
     

    We need to use full paths, e.g. django.contrib.auth.models.User

  5. reviewboard/datagrids/grids.py (Diff revision 2)
     
     

    Missing args.

  6. reviewboard/datagrids/grids.py (Diff revision 2)
     
     

    Missing args.

  7. We only require vars declared at the top for ES5 javascript (due to hoisting). It doesn't matter if you put this before or after the if statement.

  8. Format as:

    ```javascript
    /
    * Comment
    * text
    /

    1. Do you mean like:

      /*
       * An error can be caused by a 503 when the site is in 
       * read-only mode, in which case the the fields will be
       * empty.
       */
      

      Wasn't too sure what you meant in your comment but I looked around more and most seem to look like this.

  9. I'm unsure if this declaration should live here -- I don't think we do this anywhere else, really. Perhaps ping some other mentors about this.

  10. 
      
Christian Hammond
  1. 
      
  2. reviewboard/admin/read_only.py (Diff revision 2)
     
     
     
     

    Missing:

    from __future__ import unicode_literals
    

    There must be a blank line between that import and any other imports.

  3. reviewboard/admin/read_only.py (Diff revision 2)
     
     
     
     

    If user.is_superuser is True, there's no reason to bother with a cache. That's just as fast.

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

    Where does disable_save come from? Is there a change for Djblets that isn't up for review?

    It's better to have options be positive rather than negative. For instance, "enable_save" vs. "disable_save." I'm not sure I love either, though.

    There are lots of places where a Profile gets modified, and I don't think we should special-case every single place in existence. Instead, I'd override Profile.save and have it not save if is_site_read_only_for returns True. That way, the DataGrid can try saving all it wants, but it won't matter.

    1. The change to djblets didn't show up here, but I agree that overriding Profile would be better. I thought it was a django class so it would be less work this way, but I found it in rb models. I changed the Profile.save() and tested that the override worked by ensuring the code reaches the conditional which returns early with a print statement.

  5. reviewboard/reviews/default_actions.py (Diff revision 2)
     
     
     
     
     
     

    Let's pull out context['request'].user into a variable, so we're not doing the (actually fairly expensive, since context is not your standard kind of dict) lookup more than once.

    Same with all others that use this more than once.

    1. Done. Changed other instances of this below.

  6. reviewboard/reviews/models/review_request.py (Diff revision 2)
     
     
     
     
     

    Let's move the read-only check last, as it's going to be less frequently a factor (it's more likely someone will fail the other checks before this one).

  7. reviewboard/reviews/models/review_request.py (Diff revision 2)
     
     
     
     

    Same here.

  8. Can you say why this happens? It's not going to be clear to people in the future that this means it's in read-only mode.

    1. Done. Updated to:

      /* 
       * An error can be caused by a 503 when the site is in 
       * read-only mode, in which case the the fields will be
       * empty.
       */
      
  9. It'd be nice to only do this at one place. So instead, how about something like:

    if (rsp.fields === undefined) {
        message = xhr.errorText;
    } else {
        /* All the stuff here for computing an error message below. */
    }
    
    options.error.call(...);
    
  10. No trailing comma. Since this isn't an ES6 JavaScript file, it will end up breaking some browsers (such as IE).

  11. Isn't there always a value returned for readOnly? We probably don't need to specify a default.

  12. We probably don't need to specify a default, since readOnly should always have a value.

    It would also be nice to log something here to let anyone diagnosing things know that the request was ignored because it's in read-only mode.

    1. I've added the following:

              console.log(options.type + " request not sent to the API because " +
                  "the site is in read-only mode");
      
  13. We specify variables with values before variables without values.

    Also, I don't think we need a default here for readOnly.

  14. Instead of this, can you just set actions to empty if in this mode? So:

    this.template({
        ...
        actions: (this.showActions ? this.actions : []),
        ...
    });
    
  15. reviewboard/webapi/base.py (Diff revision 2)
     
     
     
     
     

    Maybe it'd make more sense to have the other change built on top of this one instead.

    1. That's true. I'll try and rearrange this soon.

    2. I've moved this change and read_only.py to the previous @ #8657.

  16. 
      
Kanghee Park
Review Bot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/admin/read_only.py
        reviewboard/reviews/models/base_comment.py
        reviewboard/webapi/base.py
        reviewboard/reviews/default_actions.py
        reviewboard/reviews/models/review_request.py
        reviewboard/accounts/models.py
        reviewboard/admin/context_processors.py
        reviewboard/settings.py
        reviewboard/reviews/templatetags/reviewtags.py
    
    Ignored Files:
        reviewboard/static/rb/js/pages/views/dashboardView.js
        reviewboard/static/rb/js/views/starManagerView.js
        reviewboard/templates/reviews/review_reply_section.html
        reviewboard/templates/base/navbar.html
        reviewboard/static/rb/js/utils/apiUtils.es6.js
        reviewboard/templates/base.html
        reviewboard/templates/base/_nav_support_menu.html
        reviewboard/static/rb/js/models/userSessionModel.js
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
        reviewboard/static/rb/js/models/reviewRequestEditorModel.js
        reviewboard/static/rb/js/views/commentDialogView.js
        reviewboard/static/rb/js/models/commentEditorModel.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/admin/read_only.py
        reviewboard/reviews/models/base_comment.py
        reviewboard/webapi/base.py
        reviewboard/reviews/default_actions.py
        reviewboard/reviews/models/review_request.py
        reviewboard/accounts/models.py
        reviewboard/admin/context_processors.py
        reviewboard/settings.py
        reviewboard/reviews/templatetags/reviewtags.py
    
    Ignored Files:
        reviewboard/static/rb/js/pages/views/dashboardView.js
        reviewboard/static/rb/js/views/starManagerView.js
        reviewboard/templates/reviews/review_reply_section.html
        reviewboard/templates/base/navbar.html
        reviewboard/static/rb/js/utils/apiUtils.es6.js
        reviewboard/templates/base.html
        reviewboard/templates/base/_nav_support_menu.html
        reviewboard/static/rb/js/models/userSessionModel.js
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
        reviewboard/static/rb/js/models/reviewRequestEditorModel.js
        reviewboard/static/rb/js/views/commentDialogView.js
        reviewboard/static/rb/js/models/commentEditorModel.js
    
    
  2. reviewboard/settings.py (Diff revision 3)
     
     
     'django_reset' imported but unused
    
  3. reviewboard/settings.py (Diff revision 3)
     
     
     'from settings_local import *' used; unable to detect undefined names
    
  4. 
      
Kanghee Park
Review Bot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/models/base_comment.py
        reviewboard/reviews/default_actions.py
        reviewboard/reviews/templatetags/reviewtags.py
        reviewboard/reviews/models/review_request.py
        reviewboard/accounts/models.py
        reviewboard/admin/context_processors.py
        reviewboard/settings.py
    
    Ignored Files:
        reviewboard/static/rb/js/pages/views/dashboardView.js
        reviewboard/static/rb/js/views/starManagerView.js
        reviewboard/templates/reviews/review_reply_section.html
        reviewboard/templates/base/navbar.html
        reviewboard/static/rb/js/utils/apiUtils.es6.js
        reviewboard/templates/base.html
        reviewboard/templates/base/_nav_support_menu.html
        reviewboard/static/rb/js/models/userSessionModel.js
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
        reviewboard/static/rb/js/models/reviewRequestEditorModel.js
        reviewboard/static/rb/js/views/commentDialogView.js
        reviewboard/static/rb/js/models/commentEditorModel.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/models/base_comment.py
        reviewboard/reviews/default_actions.py
        reviewboard/reviews/templatetags/reviewtags.py
        reviewboard/reviews/models/review_request.py
        reviewboard/accounts/models.py
        reviewboard/admin/context_processors.py
        reviewboard/settings.py
    
    Ignored Files:
        reviewboard/static/rb/js/pages/views/dashboardView.js
        reviewboard/static/rb/js/views/starManagerView.js
        reviewboard/templates/reviews/review_reply_section.html
        reviewboard/templates/base/navbar.html
        reviewboard/static/rb/js/utils/apiUtils.es6.js
        reviewboard/templates/base.html
        reviewboard/templates/base/_nav_support_menu.html
        reviewboard/static/rb/js/models/userSessionModel.js
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
        reviewboard/static/rb/js/models/reviewRequestEditorModel.js
        reviewboard/static/rb/js/views/commentDialogView.js
        reviewboard/static/rb/js/models/commentEditorModel.js
    
    
  2. reviewboard/settings.py (Diff revision 4)
     
     
     'django_reset' imported but unused
    
  3. reviewboard/settings.py (Diff revision 4)
     
     
     'from settings_local import *' used; unable to detect undefined names
    
  4. 
      
Barret Rennie
  1. 
      
  2. Mind wrapping your description & testing done at 72 characters? Also, can you change your testing done to be bullet points (- or *).

  3. Does is_site_read_only_for correctly handle user=None ?

    1. Can user ever not be in context? Especially since this is for rendering a reply, it seems like a place where a user would have to have been determined for the context. Would it be safe to remove the default value of None on context.get('user', None)?

      I can also handle None in is_site_read_only_for just in case it can happen, but I figured user will always be something by the time it gets used by is_site_read_only_for

    2. Since context is a dict, context.get(key) has a default return of None, so you can get rid of that. However, you should just do user = context['user'].

  4. Do we want this to be user-visible? Or is this only for debugging?

    1. I intended this to be a user-visible message as Christian in a previous review requested for some logging here to help with diagnosing stuff.

  5. 
      
Kanghee Park
Kanghee Park
Christian Hammond
  1. 
      
  2. reviewboard/accounts/models.py (Diff revision 5)
     
     

    This needs Args.

    It should also start off by saying the main thing that it does: "Saves the profile to the database."

    Then, in the description, it should say that if the site is in read-only mode, the profile will not be saved.

  3. Parameters should align.

    This should also use format strings:

    console.log('%s request not sent ...',
                options.type);
    

    I'd also maybe use console.error instead.

  4. 
      
Kanghee Park
Barret Rennie
  1. 
      
  2. reviewboard/accounts/models.py (Diff revision 6)
     
     

    "The profile will only be saved if the user is not affected by read-only mode."

  3. Mind adding Args/Returns while you're here?

  4. Mind adding Args/Returns while you're here?

  5. Could you put the summary on the first line of the docstring?

  6. Mind adding Args/Returns while you're here?

  7. reviewboard/templates/base.html (Diff revision 6)
     
     
     
     

    Instead of this, why not just

    readOnly: {% is_read_only|yesno:'true,false' %},
    

    That way the information is always available.

  8. 
      
Kanghee Park
Review request changed
Loading...