Skip to content

Role model: resolve related models by name#3107

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
bajtos merged 1 commit into from
Jan 25, 2017
Merged

Role model: resolve related models by name #3107

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions common/models/role.js
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -24,9 +24,9 @@ module.exports = function(Role) {
Role.resolveRelatedModels = function() {
if (!this.userModel) {
var reg = this.registry;
this.roleMappingModel = reg.getModelByType(loopback.RoleMapping);
this.userModel = reg.getModelByType(loopback.User);
this.applicationModel = reg.getModelByType(loopback.Application);
this.roleMappingModel = reg.getModelByType('RoleMapping');
this.userModel = reg.getModelByType('User');
this.applicationModel = reg.getModelByType('Application');
}
};

Expand Down
3 changes: 0 additions & 3 deletions test/role.test.js
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -47,9 +47,6 @@ describe('role model', function() {
ACL.roleMappingModel = RoleMapping;
ACL.userModel = User;
ACL.applicationModel = Application;
Role.roleMappingModel = RoleMapping;
Role.userModel = User;
Role.applicationModel = Application;
});

it('should define role/role relations', function(done) {

@benkroegerbenkroegerJan 23, 2017

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bajtos with these lines, the original tests implemented a work-around that made my original issue not happen in this module's unit-tests. By setting Role's related models manually, this skipped the process of looking-up loopback's built-in models by prototype and hence prevented the error from happening. If you just remove these lines from the test without implementing my fix, all tests should fail (and hence automatically verify my change)

@bajtosbajtosJan 25, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

I am wondering about the four lines above that set ACL.* models - are they indicating a similar problem in the ACL model as you are fixing in this patch? Is it something to eventually fix too?

@benkroegerbenkroegerJan 25, 2017

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the ACL model was already fixed in one of my previous PRs #2627

@bajtosbajtosJan 25, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we remove those four lines from the test then?

@benkroegerbenkroegerJan 25, 2017

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep. tried it out earlier. But didn't want to change scope of this PR.

Expand Down