Modify extendModel/extendCollection to modify specific routes (issues 740 and 736).

[oneironautics]

...ute to match unique routes. 736: implemented Restangular.getFullRoute() to return the complete route chain for a particular element. Added underscore.js to jshint global ignore list. Updated documentation to reflect ability to use regular expressions in extendModel/extendCollection.

[pauldijou]

Commits are free, you should do a lot of them. You are trying to do way too much in only a big one. If you mix typos, whitespaces and syntax corrections with the real feature, it makes it super hard to review because you spend half of your time ignoring green line since they are not relevant for the feature.

Also, maybe it happened automagically thanks to your IDE, but when proposing coding style corrections / modifications, you should totally do a dedicated PR rather than merging that specific stuff with a feature PR.

That said, there is one thing that I find strange but I might be missing something. Let's say I have users which have accounts, I have urls like /users/42/accounts. Currently, if I define a transformer for the route users, it will be applied only when I retrieve users, not accounts, which is pretty good.

But with your PR, when retrieving accounts, it will create a Regex('users') and test it again the full route, which is users/accounts, and it will return true, meaning my users transformer will be apply to my accounts now... Am I right?

Finally, I find it a bit strange that the full route doesn't start with a slash.

[oneironautics]

I'll split out the changes into commits if makes things easier.

You are correct that "users" will apply to any route containing "users"; this mimics the original behaviour. Specifying "^users$" will restrict the transformation to a route that begins and ends with "users". The goal of this PR is to allow flexibility when adding transformations by allowing routes to be defined as regular expressions, while preserving the original behaviour when possible. If I missed something, by all means - please let me know!

I didn't add the slash to the beginning the full route isn't representative of the final URL - Restangular.one("users", 1).one("accounts", 1).one("details") returns a full route of "users/accounts/details" without the arguments passed to each call to .one() or .all(). That said, it would look prettier, so I have no objections to adding it.

Thanks for the comments and the review; let me know what I need to do (make a new PR, add addtitional documentation, etc.) to move this along.

[mgonto]

This is missing the id in the route actually.

Also, there's already a property for this: getRestangularUrl() with which you can get the element's URL. You don't need to implement this.

[mgonto]

Hey @oneironautics thanks for the really good contribution :).

As @pauldijou was saying, if you're doing style changes, please do them in separate PRs.

Regarding the feature at hand I just have 2 comments before merging this:

• First of all, you should use getRestangularUrl to get the URL of the element instead of the newly added method. Then, the regex to match would be something like accounts/.*/users* for example. That way, you're specifying that it's accounts and users or whatever is there.
• I agree with Paul that extendModel('users' should only match user. What I'd do is to make the extendModel, extendCollection and addElementTransformer accept 2 type of first parameters. The regex you're adding here or a regular String. If you get a Regular string, you just transform it to the regex users$ so that the route has to finish with that.

Does it make sense to you? What do you think?

Cheers

[mgonto]

@pauldijou was just telling me that users$ won't do. He's right. But we should be able to create a RegEx that would match ONLY if the last section of the route has the users name.

[oneironautics]

Thanks for the input; I have a few comments and questions about the desired behaviour before submitting a new pull request.

1.) The current functionality doesn't match only terminal routes (i.e., the last route) - it matches all occurrences of a given route name. Even if only the last occurrence of a route is matched, there are certain situations where this doesn't make any sense without more context. For example: calling extendModel('details') and loading two routes ending in 'details':

XHR finished loading: GET "http://localhost:8080/api/compute/v2/d9de2b97fdbc464a9f72faaa64c6c73b/flavors/detail". angular.js:8445
extendModel("detail") called for </api/compute/v2/d9de2b97fdbc464a9f72faaa64c6c73b/flavors/detail>: [object Object] compute.js:13
XHR finished loading: GET "http://localhost:8080/api/compute/v2/d9de2b97fdbc464a9f72faaa64c6c73b/servers/detail". 
extendModel("detail") called for </api/compute/v2/d9de2b97fdbc464a9f72faaa64c6c73b/servers/detail>: [object Object] compute.js:13

The transform for flavors and servers are completely different, so this behaviour is undesireable to me - this is what my pull request is trying to address, because I need to define different transforms for /servers/detail and /flavors/detail, among other things.

2.) Expanding on the way that routes are currently matched for transforms: when you state "extendModel('detail') should only match 'detail'" - do you mean match routes terminating in detail? If so, modifying route strings under the hood will break compatibility. The two tests below show that this is not the current behaviour.

    it("should allow for a custom method to be placed at the model level when one model is requested", function() {
    var accountPromise;

    Restangular.addElementTransformer('accounts', false, function(model) {
       console.log('accounts transformed.');
       return model;
    });

    accountPromise = Restangular.one('accounts').one('test').one('accounts').one('test').one('accounts').get();

    accountPromise.then(function(account) {
      expect(1).toEqual(1);
    });

    $httpBackend.flush();
  });

  it("should allow for a custom method to be placed at the model level when one model is requested", function() {
    var accountPromise;

    Restangular.addElementTransformer('2', false, function(model) {
       console.log('2 transformed.');
       return model;
    });

    accountPromise = Restangular.one('1').one('2').one('3').one('4').one('5').get();

    accountPromise.then(function(account) {
      expect(1).toEqual(1);
    });

    $httpBackend.flush();
  });

smith:r2 smith$ grunt test
Running "karma:build" (karma) task
INFO [karma]: Karma v0.12.16 server started at http://localhost:9876/
INFO [launcher]: Starting browser PhantomJS
INFO [PhantomJS 1.9.7 (Mac OS X)]: Connected on socket tqEbj476kUHO56GchT0Z with id 12574428
LOG: 'accounts transformed.'
LOG: 'accounts transformed.'
LOG: 'accounts transformed.'
LOG: 'accounts transformed.'
LOG: '2 transformed.'
PhantomJS 1.9.7 (Mac OS X): Executed 58 of 58 SUCCESS (0.189 secs / 0.182 secs)

I personally believe it makes more sense to retain the current behaviour for compatibility (i.e., a string 'users' will match any route named 'users', regardless of position) and allow users to specify regular expressions, using the URL returned for getRestangularUrl() as the target to match for a given route. Not only does this preserve compatibility, but it affords more granular route matching for users that require it.

Your further input is appreciated.

[elliott-davis]

I would just like to chime in here and say that this patch would be helpful in my project with Restangular. If there is any testing that I can do to help get it merged I would be happy to oblige.

[mgonto]

Hey again,

I understand your points.

I understand that for your case, you want actually to match any route not just the final details. What I mean is that the default string behaviour should do what you don't want it to do, since that's the current behaviour and I'd like to make it backward compatible. However, if for the first parameter you send a regex, in that case, you can match to whatever you want and you'd be able to match anything in the route as you were actually mentioning, so in your case you would match one case of details and not the other.

The old behaviour was:

/user/1/cars ==> matches 'cars'
/user/1/cars/3 ==> matches 'cars' 

The idea is that when a String is sent, the last resource is matched. Not the last path, but the last resource. If you can accomplish that with the string param, then with the Regex, you can do what you're proposing and which I think is a really nice addition. It's just that I don't want people to expect something to happen when sending string as before and break it completely.

Do you agree?

[elliott-davis]

Hey,

So assuming I am not confusing terms, you think with this patch the new behavior would be:

/user/1/cars ==> 'cars'
/user/1/cars/3 ==> '3'

When a string is passed?

[bostrom]

This seems to be related to #1430, which separates regex transformers from simple string transformers so you can use both, side by side.