BackBone.JS Models for File Provider framework
Review Request #7140 — Created March 31, 2015 and updated
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 … |
 chipx86
|
|
|
Trailing commas in dictionaries or lists will break certain browsers. |
chipx86
|
|
|
id doesn't need to be included in this list or the next one. |
chipx86
|
|
|
Most things will actually have a type of "object". I would instead have this be: if (!options || !options.credentials) { … |
chipx86
|
|
|
In JavaScript, you should almost never use new Object(). Instead, just pass {}. |
chipx86
|
|
|
Hmm, looks like we're using id to mean something else here. We should pick a name other than id. Also, … |
chipx86
|
|
|
Same comment about id. |
chipx86
|
|
|
Same concerns about id here and below. |
chipx86
|
|
|
Many things will have a type of "object". You should use _.isObject() instead. |
chipx86
|
|
|
Should this default to false, rather than null? |
 david
|
VT
Change Summary:
Expanded the fileProviderAccount model to support the following methods:
- serializers: to serialize credentials only for creating accounts, and serialize IDs on actual created accounts.
- initialize: when no credentials exist for the item, initialize an empty object for it.
Test cases for the new initialization added.
A new serializer was added to flatten Objects to a JSON-string.
Description: |
|
||||||
|---|---|---|---|---|---|---|---|
Testing Done: |
|
||||||
Commit: |
|
||||||
Diff: |
Revision 2 (+293) |
-
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
-
-
reviewboard/static/rb/js/resources/models/fileProviderAccountModel.js (Diff revision 2) idis a reserved attribute for Django models. Assuming thisidmaps to the one in the database, you don't need it here. -
reviewboard/static/rb/js/resources/models/fileProviderAccountModel.js (Diff revision 2) Trailing commas in dictionaries or lists will break certain browsers.
-
reviewboard/static/rb/js/resources/models/fileProviderAccountModel.js (Diff revision 2) iddoesn't need to be included in this list or the next one. -
reviewboard/static/rb/js/resources/models/fileProviderAccountModel.js (Diff revision 2) Most things will actually have a type of "object". I would instead have this be:
if (!options || !options.credentials) { ... } -
reviewboard/static/rb/js/resources/models/fileProviderAccountModel.js (Diff revision 2) In JavaScript, you should almost never use
new Object(). Instead, just pass{}. -
reviewboard/static/rb/js/resources/models/fileProviderModel.js (Diff revision 2) Hmm, looks like we're using
idto mean something else here. We should pick a name other thanid.Also, it doesn't look like this is matching the
deserializedAttrsbelow? -
reviewboard/static/rb/js/resources/models/fileProviderModel.js (Diff revision 2) Same comment about
id. -
reviewboard/static/rb/js/resources/models/fileProviderNodeModel.js (Diff revision 2) Same concerns about
idhere and below. -
reviewboard/static/rb/js/resources/utils/serializers.js (Diff revision 2) Many things will have a type of "object". You should use
_.isObject()instead.
VT
Change Summary:
Updated how ids are stored for
RB.FileProviderNodeandRB.FileProviderusing idAttribute.Remove id property and parsing from
RB.FileProviderAccount.Removed all unused
path_seplogic in tests and model.Updated the serializer
flattenJSONto use a better Object verifier.
-
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
-
-
reviewboard/static/rb/js/resources/models/fileProviderModel.js (Diff revision 3) Should this default to false, rather than null?
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