Skip to content

Make things support multi-owner#2454

Closed
upupzealot wants to merge 5 commits intostrongloop:masterfrom
upupzealot:master
Closed

Make things support multi-owner#2454
upupzealot wants to merge 5 commits intostrongloop:masterfrom
upupzealot:master

Conversation

@upupzealot
Copy link

@upupzealot upupzealot commented Jun 20, 2016

When a model has two or more "belongsTo" relations, $owner will check
them all, instead of only checking the first one.

@slnode
Copy link

slnode commented Jun 20, 2016

Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test."

@upupzealot upupzealot mentioned this pull request Jun 20, 2016
Closed
@0candy
Copy link
Contributor

0candy commented Jun 20, 2016 •
edited
Loading

Thank you for submitting a PR. Please fix the long line:

/strongloop/loopback/common/models/role.js
  221:1  error  Line 221 exceeds the maximum line length of 100  max-len

Also, please add a unit test to verify your changes.

@upupzealot
Copy link
Author

upupzealot commented Jun 21, 2016 •
edited
Loading

@0candy this is strange, because I don't change this line, it's original line 222 in role.js
It seems that code indentation is the reason, I would figure that out.

About the test, I will do that, but not sure how.
Should I add a test file under the /test folder or should I modify the role.test.js which already exists?

@0candy
Copy link
Contributor

0candy commented Jun 21, 2016 •
edited
Loading

@upupzealot You're right, there is an extra indent for that line as it was moved into the if block.
As for the unit test, you can just add a test case to role.test.js. Thanks!

@slnode
Copy link

slnode commented Jun 22, 2016

Can one of the admins verify this patch?

@upupzealot
Copy link
Author

upupzealot commented Jun 23, 2016

any progress?

@ahmed-abdulmoniem
Copy link

ahmed-abdulmoniem commented Oct 16, 2016

++++1

@slnode
Copy link

slnode commented Oct 16, 2016

Can one of the admins verify this patch?

3 similar comments
@slnode
Copy link

slnode commented Oct 16, 2016

Can one of the admins verify this patch?

@slnode
Copy link

slnode commented Oct 16, 2016

Can one of the admins verify this patch?

@slnode
Copy link

slnode commented Oct 16, 2016

Can one of the admins verify this patch?

@0candy
Copy link
Contributor

0candy commented Oct 17, 2016

@bajtos Please review.

@bajtos
Copy link
Member

bajtos commented Oct 18, 2016

I am not familiar with this part of our codebase, I believe @raymondfeng and/or @ritch are more suitable to review this patch.

BTW the code needs rebasing on top of the current master, test/role.test.js was changed quite a bit recently to fix intermittent test failure.

@VictorAlbertos
Copy link

VictorAlbertos commented Oct 26, 2016

Hi :)

Are you still planing to integrate this feature?

Thanks.

@ahmed-abdulmoniem
Copy link

ahmed-abdulmoniem commented Oct 26, 2016

+1

@upscreen
Copy link

upscreen commented Oct 27, 2016

Good day

When can we expect this feature to be available in the master?

Regards.

@0candy 0candy removed the triaging label Oct 28, 2016
@0candy
Copy link
Contributor

0candy commented Oct 28, 2016

@upupzealot Could you please rebase your code?

@raymondfeng Could you please review?

@upupzealot
Copy link
Author

upupzealot commented Oct 29, 2016

@0candy conflict in the test. How to solve this?

@upupzealot
Copy link
Author

upupzealot commented Oct 29, 2016 •
edited
Loading

Did "Allow edits from maintainers", hope it would help somehow.
But how to rebase a pull request.
Actually my work with loopback ends a month ago, maybe I should make a new pull request?

@upupzealot @0candy
Make things support multi-owner
8a497eb 
When a model has two or more "belongsTo"  relations, $owner will check
them all, instead of only checking the first one.
@upupzealot @0candy
format the code
483fa95
@upupzealot @0candy
fix the long line
0f0cdf0
@upupzealot @0candy
add some test
811ac75
@0candy
Copy link
Contributor

0candy commented Oct 31, 2016

Hi @upupzealot , I rebased onto your branch, however some of the unit tests are failing. Could you please take a look? Thanks!

@bajtos bajtos mentioned this pull request Jan 16, 2017
Closed
pierreclr
pierreclr reviewed Jan 17, 2017
View reviewed changes
common/models/role.js
@@ -210,26 +210,42 @@ module.exports = function(Role) {
if (callback) callback(null, matches(ownerId, userId));
return;
} else {
Copy link
Contributor

@pierreclr pierreclr Jan 17, 2017

Choose a reason for hiding this comment

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

That way you allow multiple owner only when it's not base on the userId / owner field ... see https://github.com/strongloop/loopback/pull/3106/files for my implementation

Copy link
Author

@upupzealot upupzealot Jan 18, 2017

Choose a reason for hiding this comment

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

@pierreclr Hi, for some reason I'm no longer maintaining this pr, it's glad to see somebody else fulfilled the request for mult-owner. Hope to see this be merged into the matser soon.

@bajtos
Copy link
Member

bajtos commented Jan 19, 2017

Closing in favour of #3106

@bajtos bajtos closed this Jan 19, 2017
@pierreclr pierreclr mentioned this pull request Jan 26, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@pierreclr pierreclr pierreclr left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@0candy 0candy

Labels

feature

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

10 participants

@upupzealot @slnode @0candy @ahmed-abdulmoniem @bajtos @VictorAlbertos @upscreen @pierreclr @cgole @crandmck