Merge pull request #4522 from furinvader/feature/hal-resource-array-resource-handling

Redo: "Implement proper array resource handling for HalResource"

frontend/app/components/api/api-v3/hal-link/hal-link.service.ts

@@ -26,10 +26,19 @@
 // See doc/COPYRIGHT.rdoc for more details.
 //++
 
+import {opApiModule} from "../../../../angular-modules";
+
 var $q:ng.IQService;
 var apiV3:restangular.IService;
 
-export class HalLink {
+export interface HalLinkInterface {
+  href:string;
+  method:string;
+  title?:string;
+  templated?:boolean;
+}
+
+export class HalLink implements HalLinkInterface {
   public static fromObject(link):HalLink {
     return new HalLink(link.href, link.title, link.method, link.templated);
   }
@@ -70,13 +79,13 @@ export class HalLink {
   }
 }
 
-function halLink(_$q_:ng.IQService, _apiV3_:restangular.IService) {
+function halLinkService(_$q_:ng.IQService, _apiV3_:restangular.IService) {
   $q = _$q_;
   apiV3 = _apiV3_;
 
   return HalLink;
 }
 
-angular
-  .module('openproject.api')
-  .factory('HalLink', ['$q', 'apiV3', halLink]);
+halLinkService.$inject = ['$q', 'apiV3'];
+
+opApiModule.factory('HalLink', halLinkService);

frontend/app/components/api/api-v3/hal-resources/hal-resource.service.test.ts

@@ -32,13 +32,13 @@ const expect = chai.expect;
 describe('HalResource service', () => {
   var HalResource;
   var $httpBackend:ng.IHttpBackendService;
-  var NotificationsService;
+  var resource;
+  var source;
 
   beforeEach(angular.mock.module(opApiModule.name, opServicesModule.name));
-  beforeEach(angular.mock.inject((_HalResource_, _$httpBackend_, _NotificationsService_, apiV3) => {
-    NotificationsService = _NotificationsService_;
-    HalResource = _HalResource_;
+  beforeEach(angular.mock.inject((_$httpBackend_, _HalResource_, apiV3) => {
     $httpBackend = _$httpBackend_;
+    HalResource = _HalResource_;
 
     apiV3.setDefaultHttpFields({cache: false});
   }));
@@ -51,27 +51,15 @@ describe('HalResource service', () => {
     expect(new HalResource().href).to.equal(null);
   });
 
-  it('should return null for the href if it has no self link', () => {
-    expect(new HalResource({}).href).to.equal(null);
-  });
-
   it('should set its source to _plain if _plain is a property of the source', () => {
-    let source = {
-      _plain: {
-        _links: {},
-        prop: true
-      }
-    };
-    let resource = new HalResource(source);
+    source = {_plain: {prop: true}};
+    resource = new HalResource(source);
 
     expect(resource.prop).to.exist;
   });
 
   describe('when creating the resource using fromLink', () => {
-    var resource;
-    var link = {
-      href: 'foo'
-    };
+    var link = {href: 'foo'};
 
     beforeEach(() => {
       resource = HalResource.fromLink(link);
@@ -83,12 +71,10 @@ describe('HalResource service', () => {
 
     it('should have the same self href as the link', () => {
       expect(resource.href).to.eq(link.href);
-      expect(resource.$links.self.$link.href).to.eq(link.href);
     });
   });
 
   describe('when after generating the lazy object', () => {
-    var resource;
     var linkFn = sinon.spy();
     var embeddedFn = sinon.spy();
 
@@ -101,7 +87,7 @@ describe('HalResource service', () => {
           }
         },
         _embedded: {
-          get res() {
+          get resource() {
             embeddedFn();
             return {};
           }
@@ -118,29 +104,29 @@ describe('HalResource service', () => {
     });
 
     it('should use the source link only once when called', () => {
-      resource.$links.link;
-      resource.$links.link;
+      resource.link;
+      resource.link;
       expect(linkFn.calledOnce).to.be.true;
     });
 
     it('should use the source embedded only once when called', () => {
-      resource.$embedded.res;
-      resource.$embedded.res;
+      resource.resource;
+      resource.resource;
       expect(embeddedFn.calledOnce).to.be.true;
     });
   });
 
-  describe('when the source has properties', () => {
-    var resource;
+  describe('when the source has properties, the resource', () => {
     beforeEach(() => {
-      resource = new HalResource({
+      source = {
         _links: {},
         _embedded: {},
         property: 'foo',
         obj: {
           foo: 'bar'
         }
-      });
+      };
+      resource = new HalResource(source);
     });
 
     it('should have the same properties', () => {
@@ -148,6 +134,11 @@ describe('HalResource service', () => {
       expect(resource.obj).to.exist;
     });
 
+    it('should have properties with equal values', () => {
+      expect(resource.property).to.eq(source.property);
+      expect(resource.obj).to.eql(source.obj);
+    });
+
     it('should not have the _links property', () => {
       expect(resource._links).to.not.exist;
     });
@@ -171,12 +162,71 @@ describe('HalResource service', () => {
     });
   });
 
-  describe('when transforming an object with _links', () => {
+  describe('when creating a resource from a source with a self link', () => {
+    beforeEach(() => {
+      source = {
+        _links: {
+          self: {
+            href: '/api/hello',
+            title: 'some title'
+          }
+        }
+      };
+      resource = new HalResource(source);
+    });
+
+    it('should have a name attribute that is equal to the title of the self link', () => {
+      expect(resource.name).to.eq('some title');
+    });
+
+    it('should have a writable name attribute', () => {
+      resource.name = 'some name';
+      expect(resource.name).to.eq('some name');
+    });
+
+    it('should have a href property that is the same as the self href', () => {
+      expect(resource.href).to.eq(resource.$links.self.$link.href);
+    });
+
+    it('should have a href property that is equal to the source href', () => {
+      expect(resource.href).to.eq(source._links.self.href);
+    });
+
+    it('should not have a self property', () => {
+      expect(resource.self).not.to.exist;
+    });
+  });
+
+  describe('when using $plain', () => {
     var plain;
-    var resource;
+    source = {hello: 'world'};
+
+    beforeEach(() => {
+      plain = new HalResource(source).$plain();
+    });
+
+    it('should return an object that is equal to the source', () => {
+      expect(plain).to.eql(source);
+    });
+  });
 
+  describe('when creating a resource with a source that has no links', () => {
     beforeEach(() => {
-      plain = {
+      resource = new HalResource({});
+    });
+
+    it('should return null for the href if it has no self link', () => {
+      expect(resource.href).to.equal(null);
+    });
+
+    it('should have a $link object with null href', () => {
+      expect(resource.$link.href).to.equal(null);
+    });
+  });
+
+  describe('when transforming an object with _links', () => {
+    beforeEach(() => {
+      source = {
         _type: 'Hello',
         _links: {
           post: {
@@ -205,55 +255,13 @@ describe('HalResource service', () => {
         }
       };
 
-      resource = new HalResource(plain);
-    });
-
-    it('should be transformed', () => {
-      expect(resource.$isHal).to.be.true;
-    });
-
-    it('should have a href property that is the same as the self href', () => {
-      expect(resource.href).to.eq(resource.$links.self.$link.href);
+      resource = new HalResource(source);
     });
 
     it('should return an empty $embedded object', () => {
       expect(resource.$embedded).to.eql({});
     });
 
-    describe('when the self link has a title attribute', () => {
-      beforeEach(() => {
-        resource = new HalResource({
-          _links: {
-            self: {
-              href: '/api/hello',
-              title: 'some title'
-            }
-          }
-        });
-      });
-
-      it('should have a name attribute that is equal to the title of the self link', () => {
-        expect(resource.name).to.eq('some title');
-      });
-
-      it('should have a writable name attribute', () => {
-        resource.name = 'some name';
-        expect(resource.name).to.eq('some name');
-      });
-    });
-
-    //TODO: Fix
-    describe.skip('when returning back the plain object', () => {
-      var element;
-      beforeEach(() => {
-        element = resource.$plain();
-      });
-
-      it('should be the same as the source element', () => {
-        expect(element).to.eql(plain);
-      });
-    });
-
     describe('when after the $links property is generated', () => {
       it('should exist', () => {
         expect(resource.$links).to.exist;
@@ -275,7 +283,7 @@ describe('HalResource service', () => {
 
       it('should have a links property with the same keys as the original _links', () => {
         const transformedLinks = Object.keys(resource.$links);
-        const plainLinks = Object.keys(plain._links);
+        const plainLinks = Object.keys(source._links);
 
         expect(transformedLinks).to.have.members(plainLinks);
       });
@@ -283,11 +291,8 @@ describe('HalResource service', () => {
   });
 
   describe('when transforming an object with _embedded', () => {
-    var plain;
-    var resource;
-
     beforeEach(() => {
-      plain = {
+      source = {
         _type: 'Hello',
         _embedded: {
           resource: {
@@ -312,11 +317,7 @@ describe('HalResource service', () => {
         }
       };
 
-      resource = new HalResource(plain);
-    });
-
-    it('should return an empty $links object', () => {
-      expect(resource.$links).to.eql({});
+      resource = new HalResource(source);
     });
 
     it('should not be restangularized', () => {
@@ -368,62 +369,93 @@ describe('HalResource service', () => {
     });
   });
 
-  describe('when transforming an object with a links property that is an array', () => {
-    var resource;
-    var plain = {
-      _links: {
-        values: [
-          {href: '/api/value/1', title: 'some title'},
-          {href: '/api/value/2', title: 'some other title'}
-        ]
-      }
+  describe('when creating a resource from a source with a linked array property', () => {
+    var expectLengthsToBe = (length, update = 'update') => {
+      it(`should ${update} the values of the resource`, () => {
+        expect(resource.values).to.have.lengthOf(length);
+      });
+
+      it(`should ${update} the source`, () => {
+        expect(source._links.values).to.have.lengthOf(length);
+      });
+
+      it(`should ${update} the $source property`, () => {
+        expect(resource.$source._links.values).to.have.lengthOf(length);
+      });
     };
 
     beforeEach(() => {
-      resource = new HalResource(plain);
+      source = {
+        _links: {
+          values: [
+            {
+              href: '/api/value/1',
+              title: 'val1'
+            },
+            {
+              href: '/api/value/2',
+              title: 'val2'
+            }
+          ]
+        }
+      };
+      resource = new HalResource(source);
     });
 
-    it('should be an array of links in $links', () => {
-      expect(Array.isArray(resource.$links.values)).to.be.true;
+    it('should be an array that is a property of the resource', () => {
+      expect(resource).to.have.property('values').that.is.an('array');
     });
 
-    it('should should be the same amount of items as the original', () => {
-      expect(resource.$links.values.length).to.eq(2);
+    expectLengthsToBe(2);
+
+    describe('when adding resources to the array', () => {
+      beforeEach(() => {
+        resource.values.push(resource);
+      });
+      expectLengthsToBe(3);
     });
 
-    it('should have made each link callable', () => {
-      expect(resource.$links.values[0]).to.not.throw(Error);
+    describe('when adding arbitrary values to the array', () => {
+      beforeEach(() => {
+        resource.values.push('something');
+      });
+      expectLengthsToBe(2, 'not update');
     });
 
-    it('should be an array that is a property of the resource', () => {
-      expect(Array.isArray(resource.values)).to.be.true;
+    describe('when removing resources from the array', () => {
+      beforeEach(() => {
+        resource.values.pop();
+      });
+      expectLengthsToBe(1);
     });
 
-    describe('when the array of resources is a property of the resource, each value', () => {
-      it('should be a HalResource', () => {
-        expect(resource.values[0].$isHal).to.be.true;
+    describe('when each value is transformed', () => {
+      beforeEach(() => {
+        resource = resource.values[0];
+        source = source._links.values[0];
       });
 
-      it('should have the same href as the self link of the linked resource', () => {
-        expect(resource.values[0].href).to.eq(plain._links.values[0].href);
+      it('should have made each link a resource', () => {
+        expect(resource.$isHal).to.be.true;
       });
 
-      it('should have an equal name as the title of the linked resource', () => {
-        expect(resource.values[0].name).to.eq(plain._links.values[0].title);
+      it('should be resources generated from the links', () => {
+        expect(resource.href).to.eq(source.href);
+      });
+
+      it('should have a name attribute equal to the title of its link', () => {
+        expect(resource.name).to.eq(source.title);
       });
 
       it('should not be loaded', () => {
-        expect(resource.values[0].$loaded).to.be.false;
+        expect(resource.$loaded).to.be.false;
       });
     });
   });
 
   describe('when transforming an object with an _embedded list with the list element having _links', () => {
-    var plain;
-    var resource;
-
     beforeEach(() => {
-      plain = {
+      source = {
         _type: 'Hello',
         _embedded: {
           elements: [
@@ -439,7 +471,7 @@ describe('HalResource service', () => {
         }
       };
 
-      resource = new HalResource(plain);
+      resource = new HalResource(source);
     });
 
     it('should not be restangularized', () => {
@@ -465,10 +497,8 @@ describe('HalResource service', () => {
   });
 
   describe('when transforming an object with _links and _embedded', () => {
-    var resource;
-
     beforeEach(() => {
-      const plain = {
+      source = {
         _links: {
           property: {
             href: '/api/property',
@@ -504,7 +534,7 @@ describe('HalResource service', () => {
         }
       };
 
-      resource = new HalResource(plain);
+      resource = new HalResource(source);
     });
 
     it('should be loaded', () => {
@@ -551,14 +581,9 @@ describe('HalResource service', () => {
       var embeddedResource;
       beforeEach(() => {
         embeddedResource = {
-          $isHal: true,
-          $links: {
-            self: {
-              $link: {
-                method: 'get',
-                href: 'newHref'
-              }
-            }
+          $link: {
+            method: 'get',
+            href: 'newHref'
           }
         };
 

frontend/app/components/api/api-v3/hal-resources/hal-resource.service.ts

@@ -27,7 +27,8 @@
 //++
 
 import {opApiModule} from "../../../../angular-modules";
-import {HalLink} from "../hal-link/hal-link.service";
+import {HalLink, HalLinkInterface} from "../hal-link/hal-link.service";
+import ObservableArray = require('observable-array');
 
 var $q:ng.IQService;
 var lazy;
@@ -49,26 +50,18 @@ export class HalResource {
     return {_links: {self: {href: null}}};
   }
 
-  public $isHal:boolean = true;
   public $self:ng.IPromise<HalResource>;
 
   private _name:string;
   private _$links:any;
   private _$embedded:any;
 
-  public get name():string {
-    return this._name || this.$links.self.$link.title || '';
-  }
-
-  public set name(name:string) {
-    this._name = name;
+  public get $isHal():boolean {
+    return true;
   }
 
-  public get href():string|void {
-    if (this.$links.self) {
-      return this.$links.self.$link.href;
-    }
-    return null;
+  public get $link():HalLinkInterface {
+    return this.$links.self.$link;
   }
 
   public get $links() {
@@ -92,9 +85,29 @@ export class HalResource {
     });
   }
 
+  public get name():string {
+    return this._name || this.$link.title || '';
+  }
+
+  public set name(name:string) {
+    this._name = name;
+  }
+
+  public get href():string {
+    return this.$link.href;
+  }
+
   constructor(public $source:any = HalResource.getEmptyResource(), public $loaded:boolean = true) {
     this.$source = $source._plain || $source;
 
+    if (!this.$source._links) {
+      this.$source._links = {};
+    }
+
+    if (!this.$source._links.self) {
+      this.$source._links.self = new HalLink();
+    }
+
     this.proxyProperties();
     this.setLinksAsProperties();
     this.setEmbeddedAsProperties();
@@ -144,10 +157,21 @@ export class HalResource {
     _.without(Object.keys(this.$links), 'self').forEach(linkName => {
       lazy(this, linkName,
         () => {
-          const link = this.$links[linkName].$link || this.$links[linkName];
+          const link:any = this.$links[linkName].$link || this.$links[linkName];
 
           if (Array.isArray(link)) {
-            return link.map(item => HalResource.fromLink(item.$link));
+            var items = link.map(item => HalResource.fromLink(item.$link));
+            var property:Array = new ObservableArray(...items).on('change', () => {
+              property.forEach(item => {
+                if (!item.$link) {
+                  property.splice(property.indexOf(item), 1);
+                }
+              });
+
+              this.$source._links[linkName] = property.map(item => item.$link);
+            });
+
+            return property;
           }
 
           if (link.href) {
@@ -187,11 +211,11 @@ export class HalResource {
   }
 
   private setter(val, linkName) {
-    if (val && val.$isHal && val.$links && val.$links.self) {
-      const link = val.$links.self.$link;
+    if (val && val.$link) {
+      const link = val.$link;
 
-      if (link && link.href && link.method === 'get') {
-        this.$source._links[linkName] = val.$links.self.$link;
+      if (link.href && link.method === 'get') {
+        this.$source._links[linkName] = link;
       }
 
       return val;
@@ -199,7 +223,7 @@ export class HalResource {
   }
 }
 
-function halResource(_$q_, _lazy_, _halTransform_, _HalLink_) {
+function halResourceService(_$q_, _lazy_, _halTransform_, _HalLink_) {
   $q = _$q_;
   lazy = _lazy_;
   halTransform = _halTransform_;
@@ -208,4 +232,6 @@ function halResource(_$q_, _lazy_, _halTransform_, _HalLink_) {
   return HalResource;
 }
 
-opApiModule.factory('HalResource', ['$q', 'lazy', 'halTransform', 'HalLink', halResource]);
+halResourceService.$inject = ['$q', 'lazy', 'halTransform', 'HalLink'];
+
+opApiModule.factory('HalResource', halResourceService);

frontend/npm-shrinkwrap.json

@@ -1218,6 +1218,9 @@
     "json-loader": {
       "version": "0.5.1"
     },
+    "json5": {
+      "version": "0.4.0"
+    },
     "lodash": {
       "version": "2.4.2"
     },
@@ -1457,6 +1460,61 @@
         }
       }
     },
+    "observable-array": {
+      "version": "0.0.4",
+      "dependencies": {
+        "d": {
+          "version": "0.1.1"
+        },
+        "es5-ext": {
+          "version": "0.10.11",
+          "dependencies": {
+            "es6-iterator": {
+              "version": "2.0.0"
+            },
+            "es6-symbol": {
+              "version": "3.0.2"
+            }
+          }
+        },
+        "event-emitter": {
+          "version": "0.3.4"
+        },
+        "memoizee": {
+          "version": "0.3.10",
+          "dependencies": {
+            "es6-weak-map": {
+              "version": "0.1.4",
+              "dependencies": {
+                "es6-iterator": {
+                  "version": "0.1.3"
+                },
+                "es6-symbol": {
+                  "version": "2.0.1"
+                }
+              }
+            },
+            "lru-queue": {
+              "version": "0.1.0"
+            },
+            "next-tick": {
+              "version": "0.2.2"
+            },
+            "timers-ext": {
+              "version": "0.1.0"
+            }
+          }
+        },
+        "observable-value": {
+          "version": "0.0.5",
+          "dependencies": {
+            "es6-symbol": {
+              "version": "3.1.0"
+            }
+          }
+        }
+      }
+    },
     "polyfill-function-prototype-bind": {
       "version": "0.0.1"
     },

frontend/package.json

@@ -44,6 +44,7 @@
     "lodash": "^2.4.2",
     "ng-annotate-loader": "0.0.10",
     "ngtemplate-loader": "^0.1.2",
+    "observable-array": "0.0.4",
     "polyfill-function-prototype-bind": "0.0.1",
     "shelljs": "^0.3.0",
     "style-loader": "^0.8.2",