Check for enabled features on local sites when on a global site

Review Request #9705 — Created Feb. 26, 2018 and submitted

brennie
Review Board
release-3.0.x
87ac911...
reviewboard

When browsing a non-LocalSite specific page, if any LocalSite the user is
a member of has a feature enabled then it should be enabled for that
user while browsing the non-LocalSite page. Previously, this was not
taken into account and features that a user should otherwise have access
to would be disabled if they were enabled only for their LocalSites.
This has been corrected.

Ran unit tests.

  • 0
  • 0
  • 8
  • 0
  • 8
Description From Last Updated
chipx86
  1. 
      
  2. reviewboard/features/checkers.py (Diff revision 1)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    We can simplify this, reducing the code paths for checking a value and returning and code paths with:

    if local_site:
        local_sites = [local_site]
    elif request is not None and request.user.is_authenticated():
        local_sites = request.user.local_site.all()
    else:
        local_sites = []
    
    for local_site in local_sites:
        if self._is_enabled_for_local_site(...):
            return True
    
    return False
    
  3. reviewboard/features/checkers.py (Diff revision 1)
     
     
     
     
     
     
     

    I don't like this returning None as an optional, and don't like not having an explicit return. This way leads to confusing problems down the road. This can easily return a boolean explicitly using basically what we had before:

    if local_site.extra_data:
        try:
            return local_site.extra_data[self.EXTRA_DATA_KEY][feature_id]
        except KeyError:
            pass
    
    return False
    
    1. Or:

      return (local_site.extra_data is not None and
              local_site.extra_data.get(self.EXTRA_DATA_KEY, {}).get(feature_id, False)
      
  4. reviewboard/features/tests.py (Diff revision 1)
     
     

    No trailing comma.

  5. 
      
brennie
Review request changed

Change Summary:

Addressed Christian's comments.

Commit:

-8d52386cace9a4b8fbe18b6b598b0a0f59bfa5d6
+665cbeb63e91ebd99879681f69000a58061b6990

Diff:

Revision 2 (+84 -26)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

brennie
chipx86
  1. Ship It!

  2. 
      
brennie
chipx86
  1. 
      
  2. Typo in the description: "browing" -> "browsing"

  3. reviewboard/features/checkers.py (Diff revision 4)
     
     

    This would be better as:

    if getattr(request, 'local_site', None):
        ...
    
  4. reviewboard/features/checkers.py (Diff revision 4)
     
     
     
     
     

    These two if statements should be combined:

    if (local_site.extra_data and
        local_site.extra_data.get(...)):
    
  5. reviewboard/webapi/tests/test_base.py (Diff revision 4)
     
     

    Extra space before "but".

  6. 
      
brennie
david
  1. Ship It!
  2. 
      
brennie
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-3.0.x (74f5768)
Loading...