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

Review Request #9796 — Created March 16, 2018 and submitted

brennie
Review Board
release-3.0.x
8446a89...
reviewboard

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.

Ran unit tests.

The my account page no longer raises an exception.

Description From Last Updated

This no longer has the right saving logic.

chipx86chipx86

F821 undefined name 'new_profile'

reviewbotreviewbot

Missing Returns:

daviddavid

Alphabetical order.

chipx86chipx86

Docstring?

daviddavid

Docstring?

daviddavid

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

daviddavid

We already pull this in above. We shouldn't need to re-fetch.

chipx86chipx86

"Disabled" globally?

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

Typos: "through", "middleware"

chipx86chipx86

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

chipx86chipx86
Checks run (1 failed, 1 succeeded)
flake8 failed.
JSHint passed.

flake8

brennie
david
  1. 
      
  2. reviewboard/accounts/forms/pages.py (Diff revision 2)
     
     

    Missing Returns:

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

    Docstring?

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

    Docstring?

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

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

  6. 
      
brennie
chipx86
  1. 
      
  2. reviewboard/accounts/tests.py (Diff revision 2)
     
     
     

    Alphabetical order.

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

    We already pull this in above. We shouldn't need to re-fetch.

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

    "Disabled" globally?

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

    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)
    
  6. reviewboard/accounts/tests.py (Diff revision 3)
     
     

    Can you call this something like _build_request?

    Also, private methods go last.

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

    Typos: "through", "middleware"

  8. 
      
brennie
chipx86
  1. Ship It!

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

    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.

  3. 
      
brennie
chipx86
  1. 
      
  2. This no longer has the right saving logic.

  3. 
      
brennie
brennie
chipx86
  1. Ship It!
  2. 
      
brennie
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-3.0.x (37a8986)
Loading...