• 
      

    BackBone.JS Models for File Provider framework

    Review Request #7140 — Created March 31, 2015 and updated

    Information

    Review Board
    master
    8b34ae7...

    Reviewers

    In order to make use of responses from the File Provider framework (via api calls), BackBone.JS models can be defined to abstract these responses. These models are extended from RB.BaseResource, which include base implementation such as fetch, and parse to interact with the api calls properly. As they are BackBone.JS models, functionality can be defined on top of the object as needed, such as initialization and serialization.

    Test cases for each model was implemented (static/rb/js/resources/models/tests/*.js), each one passed.

    Description From Last Updated

    id is a reserved attribute for Django models. Assuming this id maps to the one in the database, you don't …

    chipx86chipx86

    Trailing commas in dictionaries or lists will break certain browsers.

    chipx86chipx86

    id doesn't need to be included in this list or the next one.

    chipx86chipx86

    Most things will actually have a type of "object". I would instead have this be: if (!options || !options.credentials) { …

    chipx86chipx86

    In JavaScript, you should almost never use new Object(). Instead, just pass {}.

    chipx86chipx86

    Hmm, looks like we're using id to mean something else here. We should pick a name other than id. Also, …

    chipx86chipx86

    Same comment about id.

    chipx86chipx86

    Same concerns about id here and below.

    chipx86chipx86

    Many things will have a type of "object". You should use _.isObject() instead.

    chipx86chipx86

    Should this default to false, rather than null?

    daviddavid
    reviewbot
    1. Tool: Pyflakes
      Processed Files:
          reviewboard/staticbundles.py
      
      Ignored Files:
          reviewboard/static/rb/js/resources/models/fileProviderAccountModel.js
          reviewboard/static/rb/js/resources/models/tests/fileProviderAccountModelTests.js
          reviewboard/static/rb/js/resources/models/fileProviderNodeModel.js
          reviewboard/static/rb/js/resources/models/tests/fileProviderModelTests.js
          reviewboard/static/rb/js/resources/models/tests/fileProviderNodeModelTests.js
          reviewboard/static/rb/js/resources/models/fileProviderModel.js
      
      
      
      Tool: PEP8 Style Checker
      Processed Files:
          reviewboard/staticbundles.py
      
      Ignored Files:
          reviewboard/static/rb/js/resources/models/fileProviderAccountModel.js
          reviewboard/static/rb/js/resources/models/tests/fileProviderAccountModelTests.js
          reviewboard/static/rb/js/resources/models/fileProviderNodeModel.js
          reviewboard/static/rb/js/resources/models/tests/fileProviderModelTests.js
          reviewboard/static/rb/js/resources/models/tests/fileProviderNodeModelTests.js
          reviewboard/static/rb/js/resources/models/fileProviderModel.js
      
      
    2. 
        
    VT
    reviewbot
    1. Tool: PEP8 Style Checker
      Processed Files:
          reviewboard/staticbundles.py
      
      Ignored Files:
          reviewboard/static/rb/js/resources/models/fileProviderAccountModel.js
          reviewboard/static/rb/js/resources/models/tests/fileProviderAccountModelTests.js
          reviewboard/static/rb/js/resources/utils/serializers.js
          reviewboard/static/rb/js/resources/models/fileProviderNodeModel.js
          reviewboard/static/rb/js/resources/models/tests/fileProviderModelTests.js
          reviewboard/static/rb/js/resources/models/tests/fileProviderNodeModelTests.js
          reviewboard/static/rb/js/resources/models/fileProviderModel.js
      
      
      
      Tool: Pyflakes
      Processed Files:
          reviewboard/staticbundles.py
      
      Ignored Files:
          reviewboard/static/rb/js/resources/models/fileProviderAccountModel.js
          reviewboard/static/rb/js/resources/models/tests/fileProviderAccountModelTests.js
          reviewboard/static/rb/js/resources/utils/serializers.js
          reviewboard/static/rb/js/resources/models/fileProviderNodeModel.js
          reviewboard/static/rb/js/resources/models/tests/fileProviderModelTests.js
          reviewboard/static/rb/js/resources/models/tests/fileProviderNodeModelTests.js
          reviewboard/static/rb/js/resources/models/fileProviderModel.js
      
      
    2. 
        
    chipx86
    1. 
        
    2. Show all issues

      id is a reserved attribute for Django models. Assuming this id maps to the one in the database, you don't need it here.

    3. Show all issues

      Trailing commas in dictionaries or lists will break certain browsers.

    4. Show all issues

      id doesn't need to be included in this list or the next one.

    5. Show all issues

      Most things will actually have a type of "object". I would instead have this be:

      if (!options || !options.credentials) {
          ...
      }
      
    6. Show all issues

      In JavaScript, you should almost never use new Object(). Instead, just pass {}.

    7. Show all issues

      Hmm, looks like we're using id to mean something else here. We should pick a name other than id.

      Also, it doesn't look like this is matching the deserializedAttrs below?

    8. Show all issues

      Same comment about id.

    9. Show all issues

      Same concerns about id here and below.

    10. Show all issues

      Many things will have a type of "object". You should use _.isObject() instead.

    11. 
        
    VT
    Review request changed
    Change Summary:

    Updated how ids are stored for RB.FileProviderNode and RB.FileProvider using idAttribute.

    Remove id property and parsing from RB.FileProviderAccount.

    Removed all unused path_sep logic in tests and model.

    Updated the serializer flattenJSON to use a better Object verifier.

    Commit:
    794af31a51813684dc846f1123dd8ee327c6bd54
    8b34ae7d4b3629e8f6c286258d2e984abe68c086
    reviewbot
    1. Tool: PEP8 Style Checker
      Processed Files:
          reviewboard/staticbundles.py
      
      Ignored Files:
          reviewboard/static/rb/js/resources/models/fileProviderAccountModel.js
          reviewboard/static/rb/js/resources/models/tests/fileProviderAccountModelTests.js
          reviewboard/static/rb/js/resources/utils/serializers.js
          reviewboard/static/rb/js/resources/models/fileProviderNodeModel.js
          reviewboard/static/rb/js/resources/models/tests/fileProviderModelTests.js
          reviewboard/static/rb/js/resources/models/tests/fileProviderNodeModelTests.js
          reviewboard/static/rb/js/resources/models/fileProviderModel.js
      
      
      
      Tool: Pyflakes
      Processed Files:
          reviewboard/staticbundles.py
      
      Ignored Files:
          reviewboard/static/rb/js/resources/models/fileProviderAccountModel.js
          reviewboard/static/rb/js/resources/models/tests/fileProviderAccountModelTests.js
          reviewboard/static/rb/js/resources/utils/serializers.js
          reviewboard/static/rb/js/resources/models/fileProviderNodeModel.js
          reviewboard/static/rb/js/resources/models/tests/fileProviderModelTests.js
          reviewboard/static/rb/js/resources/models/tests/fileProviderNodeModelTests.js
          reviewboard/static/rb/js/resources/models/fileProviderModel.js
      
      
    2. 
        
    david
    1. 
        
    2. Show all issues

      Should this default to false, rather than null?

    3.