Summary

Disable front-end features for read-only mode

Review Request #8810 — Created March 11, 2017 and submitted Jan. 10, 2018, 2:25 p.m.

Information

Owner

khp

Repository

Review Board

Branch

master

Bugs

Depends On

~~8648~~, ~~8657~~

Blocks

8861, 8824, 8847, 8812, 8811

Reviewers

Groups

reviewboard, students

People

Description

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.

Testing Done


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

Issues

Description	From	Last Updated
Mind wrapping your description & testing done at 72 characters? Also, can you change your testing done to be bullet …	brennie	April 6, 2017, 2:57 a.m.
'reverse' imported but unused	reviewbot	March 11, 2017, 7:38 p.m.
'django_reset' imported but unused	reviewbot	March 11, 2017, 7:38 p.m.
'from settings_local import *' used; unable to detect undefined names	reviewbot	March 11, 2017, 7:38 p.m.
Missing args/returns.	brennie	March 14, 2017, 2:50 a.m.
Missing: from __future__ import unicode_literals There must be a blank line between that import and any other imports.	chipx86	March 14, 2017, 3:58 a.m.
If you use the following, it will link to the class "the :py:class:`~djblets.siteconfig.models.SiteConfiguration`"	brennie	March 14, 2017, 2:53 a.m.
We need to use full paths, e.g. django.contrib.auth.models.User	brennie	March 14, 2017, 2:53 a.m.
If user.is_superuser is True, there's no reason to bother with a cache. That's just as fast.	chipx86	March 14, 2017, 3:55 a.m.
Missing args.	brennie	March 14, 2017, 3 a.m.
Where does disable_save come from? Is there a change for Djblets that isn't up for review? It's better to have …	chipx86	March 14, 2017, 3:53 a.m.
Missing args.	brennie	March 14, 2017, 3 a.m.
Let's pull out context['request'].user into a variable, so we're not doing the (actually fairly expensive, since context is not your …	chipx86	March 14, 2017, 3:37 a.m.
Indent this by two more spaces?	szhang	March 13, 2017, 2:11 p.m.
Indent this by one space as well.	szhang	March 13, 2017, 2:11 p.m.
Indent this by one space here?	szhang	March 13, 2017, 2:11 p.m.
Let's move the read-only check last, as it's going to be less frequently a factor (it's more likely someone will …	chipx86	March 14, 2017, 3:35 a.m.
Same here.	chipx86	March 14, 2017, 3:29 a.m.
Move this after the if statement? Putting it before the if statement subtly gives the impression that the if statement …	szhang	March 13, 2017, 2:11 p.m.
'django_reset' imported but unused	reviewbot	March 11, 2017, 8:52 p.m.
'from settings_local import *' used; unable to detect undefined names	reviewbot	March 11, 2017, 8:52 p.m.
Can you say why this happens? It's not going to be clear to people in the future that this means …	chipx86	March 14, 2017, 3:28 a.m.
Format as: ```javascript / * Comment * text /	brennie	March 18, 2017, 1:03 a.m.
It'd be nice to only do this at one place. So instead, how about something like: if (rsp.fields === undefined) …	chipx86	March 14, 2017, 3:27 a.m.
No trailing comma. Since this isn't an ES6 JavaScript file, it will end up breaking some browsers (such as IE).	chipx86	March 14, 2017, 3:24 a.m.
Isn't there always a value returned for readOnly? We probably don't need to specify a default.	chipx86	March 14, 2017, 3:24 a.m.
We probably don't need to specify a default, since readOnly should always have a value. It would also be nice …	chipx86	March 14, 2017, 3:59 a.m.
We specify variables with values before variables without values. Also, I don't think we need a default here for readOnly.	chipx86	March 14, 2017, 3:18 a.m.
I'm unsure if this declaration should live here -- I don't think we do this anywhere else, really. Perhaps ping …	brennie	March 14, 2017, 4 a.m.
Instead of this, can you just set actions to empty if in this mode? So: this.template({ ... actions: (this.showActions ? …	chipx86	March 14, 2017, 4:08 a.m.
Maybe it'd make more sense to have the other change built on top of this one instead.	chipx86	March 18, 2017, 2:33 a.m.
'django_reset' imported but unused	reviewbot	March 14, 2017, 4:49 a.m.
'from settings_local import *' used; unable to detect undefined names	reviewbot	March 14, 2017, 4:49 a.m.
Does is_site_read_only_for correctly handle user=None ?	brennie	April 6, 2017, 8:19 p.m.
'django_reset' imported but unused	reviewbot	March 18, 2017, 2:37 a.m.
'from settings_local import *' used; unable to detect undefined names	reviewbot	March 18, 2017, 2:37 a.m.
Do we want this to be user-visible? Or is this only for debugging?	brennie	April 6, 2017, 12:53 p.m.
This needs Args. It should also start off by saying the main thing that it does: "Saves the profile to …	chipx86	April 13, 2017, 2:06 p.m.
Parameters should align. This should also use format strings: console.log('%s request not sent ...', options.type); I'd also maybe use console.error …	chipx86	April 13, 2017, 3:04 p.m.
"The profile will only be saved if the user is not affected by read-only mode."	brennie	April 16, 2017, 12:37 p.m.
Mind adding Args/Returns while you're here?	brennie	April 16, 2017, 1:08 p.m.
Mind adding Args/Returns while you're here?	brennie	April 16, 2017, 1:08 p.m.
Could you put the summary on the first line of the docstring?	brennie	April 16, 2017, 1:09 p.m.
Mind adding Args/Returns while you're here?	brennie	April 16, 2017, 1:25 p.m.
Instead of this, why not just readOnly: {% is_read_only\|yesno:'true,false' %}, That way the information is always available.	brennie	April 16, 2017, 1:40 p.m.

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

reviewboard/reviews/views.py (Diff revision 1)
The issue has been resolved. Show all issues
```
 'reverse' imported but unused
```
reviewboard/settings.py (Diff revision 1)
The issue has been dropped. Show all issues
```
 'django_reset' imported but unused
```
reviewboard/settings.py (Diff revision 1)
The issue has been dropped. Show all issues
```
 'from settings_local import *' used; unable to detect undefined names
```

Testing Done:: +
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

Diff:

Revision 2 (+140 -31)

Show changes

	reviewboard/settings.py
	reviewboard/admin/context_processors.py
	reviewboard/admin/read_only.py
	reviewboard/datagrids/grids.py
	reviewboard/reviews/default_actions.py
	reviewboard/reviews/views.py
	reviewboard/reviews/models/base_comment.py
	reviewboard/reviews/models/review_request.py
	14 more

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

reviewboard/settings.py (Diff revision 2)
The issue has been dropped. Show all issues
```
 'django_reset' imported but unused
```
reviewboard/settings.py (Diff revision 2)
The issue has been dropped. Show all issues
```
 'from settings_local import *' used; unable to detect undefined names
```

Mostly just small indentation issues. :)

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

The issue has been dropped. Show all issues

Indent this by two more spaces?

khp March 13, 2017, 2:12 p.m.

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

reviewboard/reviews/default_actions.py (Diff revision 2)
The issue has been dropped. Show all issues
```
Indent this by one space as well.
```
1. KH
  
  khp March 13, 2017, 2:12 p.m.
  See the other comments re: indentation.

reviewboard/reviews/models/base_comment.py (Diff revision 2)

The issue has been dropped. Show all issues

Indent this by one space here?

khp March 13, 2017, 2:12 p.m.

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.

szhang March 13, 2017, 4:02 p.m.

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

reviewboard/reviews/templatetags/reviewtags.py (Diff revision 2)

The issue has been dropped. Show all issues

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.

khp March 13, 2017, 2:12 p.m.

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

reviewboard/admin/context_processors.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Missing args/returns.
```

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

The issue has been resolved. Show all issues

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

"the :py:class:`~djblets.siteconfig.models.SiteConfiguration`"

reviewboard/admin/read_only.py (Diff revision 2)
The issue has been resolved. Show all issues
```
We need to use full paths, e.g. django.contrib.auth.models.User
```
reviewboard/datagrids/grids.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Missing args.
```
reviewboard/datagrids/grids.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Missing args.
```

reviewboard/reviews/templatetags/reviewtags.py (Diff revision 2)

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.

reviewboard/static/rb/js/models/reviewRequestEditorModel.js (Diff revision 2)

The issue has been resolved. Show all issues

Format as:

```javascript
/
* Comment
* text
/

khp March 14, 2017, 4:41 a.m.

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.

reviewboard/static/rb/js/views/reviewRequestEditorView.js (Diff revision 2)

The issue has been resolved. Show all issues

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.

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

The issue has been resolved. Show all issues

Missing:

from __future__ import unicode_literals


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

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

The issue has been resolved. Show all issues

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

KH

khp March 14, 2017, 4:45 a.m.
```
I see! Fixed.
```

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

The issue has been resolved. Show all issues

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.

khp March 14, 2017, 4:45 a.m.

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.

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

The issue has been resolved. Show all issues

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.

khp March 14, 2017, 4:45 a.m.

Done. Changed other instances of this below.

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

The issue has been resolved. Show all issues

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

reviewboard/reviews/models/review_request.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Same here.
```

reviewboard/static/rb/js/models/reviewRequestEditorModel.js (Diff revision 2)

The issue has been resolved. Show all issues

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.

khp March 14, 2017, 4:45 a.m.

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

reviewboard/static/rb/js/models/reviewRequestEditorModel.js (Diff revision 2)

The issue has been resolved. Show all issues

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(...);

KH

khp March 14, 2017, 4:45 a.m.
```
Done
```

reviewboard/static/rb/js/models/reviewRequestEditorModel.js (Diff revision 2)

The issue has been resolved. Show all issues

No trailing comma. Since this isn't an ES6 JavaScript file, it will end up breaking some browsers (such as IE).

reviewboard/static/rb/js/pages/views/dashboardView.js (Diff revision 2)
The issue has been resolved. Show all issues
```
Isn't there always a value returned for readOnly? We probably don't need to specify a default.
```

reviewboard/static/rb/js/utils/apiUtils.es6.js (Diff revision 2)

The issue has been resolved. Show all issues

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.

khp March 14, 2017, 4:45 a.m.

I've added the following:

        console.log(options.type + " request not sent to the API because " +
            "the site is in read-only mode");

reviewboard/static/rb/js/views/reviewRequestEditorView.js (Diff revision 2)

The issue has been resolved. Show all issues

We specify variables with values before variables without values.

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

reviewboard/static/rb/js/views/reviewRequestEditorView.js (Diff revision 2)

The issue has been resolved. Show all issues

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

this.template({
    ...
    actions: (this.showActions ? this.actions : []),
    ...
});

reviewboard/webapi/base.py (Diff revision 2)

The issue has been resolved. Show all issues

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

khp March 14, 2017, 4:45 a.m.

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

khp March 18, 2017, 2:33 a.m.

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

Change Summary:

Changes according to comments. Still need to re-arrange commits to remove redundant changes.

Diff:

Revision 3 (+196 -71)

Show changes

	reviewboard/settings.py
	reviewboard/accounts/models.py
	reviewboard/admin/context_processors.py
	reviewboard/admin/read_only.py
	reviewboard/reviews/default_actions.py
	reviewboard/reviews/views.py
	reviewboard/reviews/models/base_comment.py
	reviewboard/reviews/models/review_request.py
	14 more

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

reviewboard/settings.py (Diff revision 3)
The issue has been dropped. Show all issues
```
 'django_reset' imported but unused
```
reviewboard/settings.py (Diff revision 3)
The issue has been dropped. Show all issues
```
 'from settings_local import *' used; unable to detect undefined names
```

Change Summary:

Moved read_only.py and webapi/base.py changes to previous diff, review #8657. Since the review is based on the old 8657 still, it will not show read_only.py and the webapi.py changes on this diff, but once it is applied on 8657 it should show.

Diff:

Revision 4 (+164 -66)

Show changes

	reviewboard/settings.py
	reviewboard/accounts/models.py
	reviewboard/admin/context_processors.py
	reviewboard/reviews/default_actions.py
	reviewboard/reviews/views.py
	reviewboard/reviews/models/base_comment.py
	reviewboard/reviews/models/review_request.py
	reviewboard/reviews/templatetags/reviewtags.py
	12 more

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

reviewboard/settings.py (Diff revision 4)
The issue has been dropped. Show all issues
```
 'django_reset' imported but unused
```
reviewboard/settings.py (Diff revision 4)
The issue has been dropped. Show all issues
```
 'from settings_local import *' used; unable to detect undefined names
```

The issue has been resolved. Show all issues

Mind wrapping your description & testing done at 72 characters? Also, can you change your testing done to be bullet points (- or *).

reviewboard/reviews/templatetags/reviewtags.py (Diff revision 4)

The issue has been resolved. Show all issues

Does is_site_read_only_for correctly handle user=None ?

khp April 6, 2017, 3:49 a.m.

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

brennie April 6, 2017, 12:54 p.m.

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

reviewboard/static/rb/js/utils/apiUtils.es6.js (Diff revision 4)

The issue has been dropped. Show all issues

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

khp April 6, 2017, 3:49 a.m.

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.

Description:: ~
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.
~
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),
~
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.
~
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.
Testing Done:: ~
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
~
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

Diff:

Revision 5 (+168 -66)

Show changes

	reviewboard/settings.py
	reviewboard/accounts/models.py
	reviewboard/admin/context_processors.py
	reviewboard/admin/read_only.py
	reviewboard/reviews/default_actions.py
	reviewboard/reviews/views.py
	reviewboard/reviews/models/base_comment.py
	reviewboard/reviews/models/review_request.py
	13 more

Checks run (2 succeeded, 1 failed with error)

JSHint passed.

PEP8 Style Checker internal error.

Pyflakes passed.

reviewboard/accounts/models.py (Diff revision 5)

The issue has been resolved. Show all issues

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.

reviewboard/static/rb/js/utils/apiUtils.es6.js (Diff revision 5)

The issue has been resolved. Show all issues

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.

Diff:

Revision 6 (+178 -66)

Show changes

	reviewboard/settings.py
	reviewboard/accounts/models.py
	reviewboard/admin/context_processors.py
	reviewboard/admin/read_only.py
	reviewboard/reviews/default_actions.py
	reviewboard/reviews/views.py
	reviewboard/reviews/models/base_comment.py
	reviewboard/reviews/models/review_request.py
	13 more

Checks run (2 succeeded, 1 failed with error)

JSHint passed.

PEP8 Style Checker internal error.

Pyflakes passed.

Ship it!

reviewboard/accounts/models.py (Diff revision 6)
The issue has been resolved. Show all issues
```
"The profile will only be saved if the user is not affected by read-only mode."
```
reviewboard/reviews/models/review_request.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Mind adding Args/Returns while you're here?
```
reviewboard/reviews/models/review_request.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Mind adding Args/Returns while you're here?
```
reviewboard/reviews/templatetags/reviewtags.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Could you put the summary on the first line of the docstring?
```
reviewboard/reviews/templatetags/reviewtags.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Mind adding Args/Returns while you're here?
```

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

The issue has been resolved. Show all issues

Instead of this, why not just

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

That way the information is always available.

Diff:

Revision 7 (+226 -70)

Show changes

	reviewboard/settings.py
	reviewboard/accounts/models.py
	reviewboard/admin/context_processors.py
	reviewboard/admin/read_only.py
	reviewboard/reviews/default_actions.py
	reviewboard/reviews/views.py
	reviewboard/reviews/models/base_comment.py
	reviewboard/reviews/models/review_request.py
	13 more

Checks run (2 succeeded, 1 failed with error)

JSHint passed.

PEP8 Style Checker internal error.

Pyflakes passed.

Status:: Completed
Change Summary:: Pushed to release-4.0.x (f16f83d)

	+	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

~		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.
	~	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),
	~	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.
	~	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.