[BUGFIX] normalize model name for belongs to relationships #5477
Conversation
|
@agrobbin if you have time to add tests, I can help you locate the right spot and draft them :) |
|
@runspired absolutely, just point me in the right direction and I'll add them! |
|
@runspired were you able to locate where tests should go for this change? |
|
@agrobbin haven't looked yet, sorry. I should get back to you this afternoon though :) |
|
Unless @runspired has a better suggestion I think it would be ok to add a test for this change to tests/unit/model/relationships-test.js. |
agrobbin
force-pushed
the
belongs-to-model-name-normalization
branch
from
1cb1b5a to
7e32805
Compare
|
@bmac @runspired I just pushed up test coverage for this. Let me know if everything looks good! |
| const StreamItem = DS.Model.extend(); | ||
|
|
||
| let User = DS.Model.extend({ | ||
| streamItems: DS.hasMany(), |
There was a problem hiding this comment.
this should actually assert, as there is no inverse, surprised it does not
There was a problem hiding this comment.
Not sure why it didn't assert, but I added the inverse relationship to StreamItem above.
| const UserProfile = DS.Model.extend(); | ||
|
|
||
| let User = DS.Model.extend({ | ||
| userProfile: DS.belongsTo(), |
There was a problem hiding this comment.
same comment as below, this should assert and I'm surprised it does not, UserProfile needs the inverse defined.
There was a problem hiding this comment.
Ditto here, I added the inverse relationship definition to UserProfile above.
agrobbin
force-pushed
the
belongs-to-model-name-normalization
branch
from
7e32805 to
64461e5
Compare
| const relationships = get(User, 'relationships'); | ||
| const relationship = relationships.get('stream-item')[0]; | ||
|
|
||
| assert.equal(relationship.meta.name, 'streamItems'); |
There was a problem hiding this comment.
give asserts a meaningful description as the last arg (same for the other test).
| User = store.modelFor('user'); | ||
|
|
||
| const relationships = get(User, 'relationships'); | ||
| const relationship = relationships.get('stream-item')[0]; |
There was a problem hiding this comment.
we likely want an assertion that we did in fact normalize to stream-item from streamItems here, instead of just relying on it implicitly being tested due to how we access. Same with the test above.
|
@agrobbin saw you resolved a few comments, left a few more. Resolve those and we should be good :) |
Given these models:
```ruby
const User = DS.Model.extend({
userProfile: DS.belongsTo(),
streamItems: DS.hasMany()
});
const UserProfile = DS.Model.extend({
user: DS.belongsTo()
});
const StreamItem = DS.Model.extend({
user: DS.belongsTo()
});
```
The inferred model type of `User.streamItems` will be `stream-item` while the type of `User.userProfile` will be `userProfile`.
This goes against the concept of all model types being dasherized.
With this change, `User.userProfile` will be converted into `user-profile`.
agrobbin
force-pushed
the
belongs-to-model-name-normalization
branch
from
64461e5 to
ff47a3e
Compare
|
@runspired just pushed some more updates! |
|
Thanks for all of the help @runspired and @bmac! Looking forward to seeing this included in the next Ember Data release. |
) Given these models: ```ruby const User = DS.Model.extend({ userProfile: DS.belongsTo(), streamItems: DS.hasMany() }); const UserProfile = DS.Model.extend({ user: DS.belongsTo() }); const StreamItem = DS.Model.extend({ user: DS.belongsTo() }); ``` The inferred model type of `User.streamItems` will be `stream-item` while the type of `User.userProfile` will be `userProfile`. This goes against the concept of all model types being dasherized. With this change, `User.userProfile` will be converted into `user-profile`.
Given these models:
The inferred model type of
User.streamItemswill bestream-itemwhile the type ofUser.userProfilewill beuserProfile.This goes against the concept of all model types being dasherized.
With this change,
User.userProfilewill be converted intouser-profile.I'm not exactly sure where this kind of thing should be tested. It seems like it belongs in same place that the
hasManymodel type normalization tests reside, but I'm having trouble finding that place!