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.