List OAuth applications under their local sites

Review Request #9094 - Created July 20, 2017 and submitted

Barret Rennie
Review Board
release-3.0.x
9096
f36bf09...
reviewboard

Previously, all OAuth2 applications were listed in a single list, giving
no distinction to those limited to Local Sites. Now they are grouped
by Local Site so they can be more easily be distinguished.

This change does not add support for creating applications under Local
Sites via the UI.

Testing done:

  • Created and deleted applications on the global site.
  • Deleted applications on a Local Site.
Loading file attachments...

  • 0
  • 0
  • 18
  • 2
  • 20
Description From Last Updated
Checks run (1 failed, 1 succeeded)
flake8 passed.
JSHint failed.

JSHint

Barret Rennie
David Trowbridge
  1. 
      
  2. I'm not a fan of the shorthand syntax, especially here where you could just create the object all in one go. How about:

    return _.defaults(rsp, {
       apiURL: (localSiteName
                ? `/s/${localSiteName}${baseURL}${rsp.id}`
                : `${baseURL}${rsp.id}/`),
       editURL: `${baseEditURL}/${rsp.id}/`,
    });
    
  3. reviewboard/static/rb/js/accountPrefsPage/views/oauthApplicationsView.es6.js (Diff revision 2)
     
     
     
     
     
     
     
     
     
     
     

    This is ugly. How about:

    const templateOptions = {
        emptyText: (localSiteName
                    ? interpolate(localSiteEmptyText,
                                  [localSiteName])
                    : emptyText),
    };
    

    Note also that your second case had localSiteEmptyText where I think it should have been emptyText.

  4. Can we have the positive case first?

  5. 
      
Barret Rennie
David Trowbridge
Barret Rennie
Christian Hammond
  1. 
      
  2. Can you add Model Attributes: docs for these?

  3. id is implicitly defined for models. No need to put it in here.

  4. Can we use options here? Otherwise the docs aren't going to generate right when we get around to adding that.

  5. for .. of creates a huge amount of JavaScript (with multiple layers of exception handling, nested loops, and several layers of temporary function calls) when going through Babel, totalling about 64 lines of code just for the loop itself. As nice as for .. of would be if native, it's not worth it when transpiling, and I'd really like to avoid using it in all code until we can use it natively.

    Can this be reworked to loop over this with a standard for loop? Looks like we're just looping over an array?

  6. reviewboard/static/rb/js/accountPrefsPage/views/oauthApplicationsView.es6.js (Diff revision 4)
     
     
     
     
     
     
     
     
     
     
     

    Can this be combined?

  7. 
      
Barret Rennie
Christian Hammond
  1. 
      
  2. Maybe put the [ on the previous line, so these two lines can just be pure list items and on the natural indentation level. That'll also help fix the attributes, which are currently indented 1 space too far.

  3. We're still doing 2 loops when I'm sure we can do just 1. What's stopping us from using a standard dictionary object mapping site names to arrays and doing a standard iteration through it?

  4. 
      
Barret Rennie
Christian Hammond
  1. 
      
  2. (A lot of this comment is about the under-the-hood operations of iterators and how Babel transpiles stuff, so take it as an analysis of things, with my recommendation at the very end.)

    This is still more wasteful than needed. We're building an array and discarding it, and requiring a function invocation per entry.

    The cheapest way, performance and memory-wise, would be to use a standard dictionary object with a standard loop:

    this.collections = {};
    
    ...
    
    for (const localSiteName in this.collections) {
        if (this.collections.hasOwnProperty(localSiteName)) {
            const collection = this.collections[localSiteName];
    
            ...
        }
    }
    

    We save memory by not building a throw-away array. We also technically save a little memory by using an object instead of a Map (which has additional bookkeeping), but shrug, not a big deal. It's just the array that I want to get rid of, since it's completely unnecessary and will just be populated and then immediately scheduled to be garbage-collected.

    There are two other options we should look at:

    1. Use Map.forEach, which is going to be nicer than building a throw-away array or using Babel's for .. of.
    2. Use a standard for loop, working with the iterator. A bit messier, probably not really worth it, but just to show how this would look (and how iterators work under the hood):

    for (let entries = m.entries(), entry = entries.next(); !entry.done; entry = entries.next()) {
        const localSiteName = entry.value[0];
        const collection = entry.value[1];
    
        ...
    }
    

    Not as nice as a for .. of (I can't wait until we can fully depend on that in browsers, but until then...)

    Side note on under-the-hood technicals: My code snippet pulled out the items from the entry.value array manually instead of using:

    const [localSiteName, collection] = entry.value;
    

    Every time you do that, Babel injects the following function into your file:

    var _slicedToArray = function() {
        function sliceIterator(arr, i) {
            var _arr = [];
            var _n = true;
            var _d = false;
            var _e = undefined;
            try {
                for (var _i = arr[Symbol.iterator](), _s; !(_n = (_s = _i.next()).done); _n = true) {
                    _arr.push(_s.value);
                    if (i && _arr.length === i) break;
                }
            } catch (err) {
                _d = true;
                _e = err;
            } finally {
                try {
                    if (!_n && _i["return"]) _i["return"]();
                } finally {
                    if (_d) throw _e;
                }
            }
            return _arr;
        }
        return function(arr, i) {
            if (Array.isArray(arr)) {
                return arr;
            } else if (Symbol.iterator in Object(arr)) {
                return sliceIterator(arr, i);
            } else {
                throw new TypeError("Invalid attempt to destructure non-iterable instance");
            }
        };
    }();
    

    For the case above, since it's dealing with an array, this expands to:

    var array = (function() {
        if (isArray(entry.value)) {
            return entry.value;
        }
    
        // Not reached
    )();
    
    var array = entry.value;
    var localSiteName = array[0];
    var collection = array[1];
    

    Not as bad as it could be, certainly. Still wasteful, but not horribly so. Just means the function gets injected into each file, an additional two function calls per loop, and an additional state variable within the loop. I get the tradeoff of the new syntax being more convenient, but it's tradeoff. That said, I don't particularly mind this as much, but it very much depends on the code being used. Expensive code/long loops probably shouldn't use it.

    I don't know how this looks when bundling -- I suspect we still will get a thousand copies of this function, instead of Babel graciously providing a single version. If we only got one, that would be super nice -- our JS files are getting big as-is).

    Anyway, in summary, I think the best bet for the code here is to use Map.forEach, since each option will result in a per-loop function call somewhere.

  3. 
      
Barret Rennie
David Trowbridge
  1. Just a couple formatting nits:

  2. Can we wrap this as:

    Object.entries(options.apps)
        .map(...)
    

    Right now it reads as if we're doing some operation on Object instead of on the contents of options.apps

  3. These would fit on one line, which would let you dedent the block by 4 spaces.

  4. 
      
Barret Rennie
David Trowbridge
  1. Ship It!
  2. 
      
Barret Rennie
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-3.0.x (ee31cda)
Loading...