feat(articles): ArticlesService extended $resource

[mleanos]

Extends the ArticlesService $resource object to include a custom method for creating, or updating, an Article instance.

Related #1260

[mleanos]

If we're happy with these changes, and going this direction with extending the Angular Resource objects, I'll refactor the rest of the client-side Articles code to follow this pattern. For instance, I'll add a method for the $resource.query() method called find() or list().

I'll also add some extra logic to the createOrUpdate method to return a new Promise, so we can catch errors inside ArticlesService & handle them accordingly.

[ilanbiala]

@mleanos take a look at angular/angular.js#10692 (comment), An Angular.js member suggests a slightly different style, maybe see if there's any reason to do it one way over the other?

[mleanos]

I originally came across this implementation, when investigating #1149. However, when I came across http://sauceio.com/index.php/2014/07/adding-custom-methods-to-data-models-with-angular-resource/, i like it much better.

Using angular.extend allows us to add Class level methods, as well as instance methods. We can separate instance methods from the Class methods like so..

var Article = $resource('api/articles/:articleId', {
      articleId: '@_id'
    }, {
      update: {
        method: 'PUT'
      }
    });

    // instance methods
    angular.extend(Article.prototype, {
      createOrUpdate: function () {
        var article = this;
        return createOrUpdate(article);
      }
    });

    // Class methods
    angular.extend(Article, {
      find: function () {
        var articlesResource = this;
        return articlesResource.query();
      }
    });

    return Article;

    function createOrUpdate(article) {
      if (article._id) {
        return article.$update();
      } else {
        return article.$save();
      }
    }
  }

Using Article.prototype.find would be ambiguous as to whether or not we're working with an instance or the Class itself. And would require a bit more logic.

[mleanos]

@codydaig @ilanbiala @lirantal Any thoughts on this implementation?

[mleanos]

@ilanbiala Thoughts?

[rhutchison]

I personally am a huge fan of angular-model-factory.

[ilanbiala]

I would just consider moving this above the return, some people might get confused and forget about hoisting.

[mleanos]

I think having the function declarations after the return statement make it easier to read, and understand the implementation.

It follows this style from John Papa's guide. I don't feel super strong about it, but I think it would help others that are following, or aware of, the style-guide.

[ilanbiala]

LGTM other than the one code order concern I mentioned above.

[mleanos]

@rhutchison I really like the idea of having client-side model's. However, angular-model-factory didn't really impress me. I felt like it added more complexity than it's worth to the project. I'd love to see how you would integrate it into this framework though. Perhaps, there's a benefit that I just didn't see.

Our framework is quite lightweight, so it might be better to give our user's a simple approach, and let them decide if they wanted to implement something like angular-model-factory.

[trendzetter]

Thanks! I think it is useful as example of how to use the service to create a separate client side data layer instead of preparing your data in the controller.

[ilanbiala]

@mleanos other than adding tests, which may or may not be necessary, LGTM.

[mleanos]

I'm pretty sure we already have tests for this functionality. I'll double check though.

[ilanbiala]

@mleanos if we don't need to add tests, LGTM.

[mleanos]

@ilanbiala I rebased this and added some additional error/callback handling from inside the service.

I verified that we already have test coverage for this functionality. We're testing the save() method of the Article client controller, which now implements this new service method.

@codydaig @ilanbiala @lirantal @simison LGTY? If so, I'll merge.

[coveralls]

Coverage remained the same at 70.615% when pulling 383a3a6 on mleanos:feature/articles-client-save-or-update into e3cd65f on meanjs:master.

[ilanbiala]

LGTM.

[mleanos]

@meanjs/contributors I'll merge this in in the next few days, if there are no objections & I can get another LGTM. I just want to make sure we're still ok with this change.

[lirantal]

I guess it's ok by me.

[codydaig]

LGTM.