Summary

Don't update syntax highlighting prefs when it is disabled globally

Review Request #9796 — Created March 16, 2018 and submitted March 19, 2018, 8:31 p.m.

Information

Owner

brennie

Repository

Review Board

Branch

release-3.0.x

Bugs

Depends On

Commit

8446a89...

Reviewers

Groups

reviewboard

People

Description

When syntax highlighting was disabled globally, we were deleting the
form field from the AccountSettingsForm. However, later on we were
trying to read from this field, causing an exception and the my account
page to not load.

Now when syntax highlighting is disabled, we mark the field as disabled
and refuse updates to it. It will also always render as if the user had
it disabled so it does not appear in a disabled but always on state for
the user.

Testing Done

Ran unit tests.

The my account page no longer raises an exception.

Issues

Description	From	Last Updated
This no longer has the right saving logic.	chipx86	March 19, 2018, 8:11 p.m.
F821 undefined name 'new_profile'	reviewbot	March 16, 2018, 3:53 p.m.
Missing Returns:	david	March 19, 2018, 12:59 p.m.
Alphabetical order.	chipx86	March 19, 2018, 5:13 p.m.
Docstring?	david	March 19, 2018, 1 p.m.
Docstring?	david	March 19, 2018, 1:02 p.m.
Maybe a bit cleaner if we pass user into self._request and have it assign it.	david	March 19, 2018, 1:02 p.m.
We already pull this in above. We shouldn't need to re-fetch.	chipx86	March 19, 2018, 5:13 p.m.
"Disabled" globally?	chipx86	March 19, 2018, 5:13 p.m.
So I was going to say we should combine the if statements, but really, we can just do: if not …	chipx86	March 19, 2018, 5:14 p.m.
Can you call this something like _build_request? Also, private methods go last.	chipx86	March 19, 2018, 5:15 p.m.
Typos: "through", "middleware"	chipx86	March 19, 2018, 5:15 p.m.
Just realized, there's no reason to do this in clean() anymore. Instead, the setting should only be conditionally saved in …	chipx86	March 19, 2018, 7:36 p.m.

flake8 failed.

JSHint passed.

flake8

reviewboard/accounts/tests.py (Diff revision 1)
The issue has been resolved. Show all issues
```
F821 undefined name 'new_profile'
```

Commit:

d9c5d9d83f266cc1d0fa01ad75a583159a718290

9a94b4dd43f871a0c75aa21f713e6069adbf54df

Diff:

Revision 2 (+84 -3)

Show changes

	reviewboard/accounts/tests.py
	reviewboard/accounts/forms/pages.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/accounts/forms/pages.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Missing Returns:
```
reviewboard/accounts/tests.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Docstring?
```
reviewboard/accounts/tests.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Docstring?
```

reviewboard/accounts/tests.py (Diff revision 2)

The issue has been resolved. Show all issues

Maybe a bit cleaner if we pass user into self._request and have it assign it.

Change Summary:

Addressed David's issues.

Commit:

9a94b4dd43f871a0c75aa21f713e6069adbf54df

dfff73219256799995a3ee9d0208716e301097f5

Diff:

Revision 3 (+106 -3)

Show changes

	reviewboard/accounts/tests.py
	reviewboard/accounts/forms/pages.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/accounts/tests.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Alphabetical order.
```
reviewboard/accounts/forms/pages.py (Diff revision 3)
The issue has been resolved. Show all issues
```
We already pull this in above. We shouldn't need to re-fetch.
```
reviewboard/accounts/forms/pages.py (Diff revision 3)
The issue has been resolved. Show all issues
```
"Disabled" globally?
```

reviewboard/accounts/forms/pages.py (Diff revision 3)

The issue has been resolved. Show all issues

So I was going to say we should combine the if statements, but really, we can just do:

if not siteconfig.get(...):
    self.cleaned_data.pop('syntax_highlighting', None)

reviewboard/accounts/tests.py (Diff revision 3)

The issue has been resolved. Show all issues

Can you call this something like _build_request?
Also, private methods go last.

reviewboard/accounts/tests.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Typos: "through", "middleware"
```

Change Summary:

address issues, rebase

Commit:

dfff73219256799995a3ee9d0208716e301097f5

2f94333af6ffa23804eee81cb9f923337d3b638f

Diff:

Revision 4 (+105 -3)

Show changes

	reviewboard/accounts/tests.py
	reviewboard/accounts/forms/pages.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

```
Ship It!
```

reviewboard/accounts/forms/pages.py (Diff revision 4)

The issue has been resolved. Show all issues

Just realized, there's no reason to do this in clean() anymore. Instead, the setting should only be conditionally saved in save() if diffviewer_syntax_highlighting is set.