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

Jump to bottom

Make things support multi-owner #2454

Closed

upupzealot wants to merge 5 commits into strongloop:master from upupzealot:master

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 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."

crandmck added the #community contribution label

upupzealot mentioned this pull request

Multi owner for one model #2441

Closed

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.

0candy added feature triaging labels

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?

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 commented Jun 22, 2016

Can one of the admins verify this patch?

Author

upupzealot commented Jun 23, 2016

any progress?

ahmed-abdulmoniem commented Oct 16, 2016

++++1

slnode commented Oct 16, 2016

Can one of the admins verify this patch?

3 similar comments

slnode commented Oct 16, 2016

Can one of the admins verify this patch?

slnode commented Oct 16, 2016

Can one of the admins verify this patch?

slnode commented Oct 16, 2016

Can one of the admins verify this patch?

superkhau assigned 0candy

Contributor

0candy commented Oct 17, 2016

@bajtos Please review.

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 commented Oct 26, 2016

Hi :)

Are you still planing to integrate this feature?

Thanks.

ahmed-abdulmoniem commented Oct 26, 2016

+1

upscreen commented Oct 27, 2016

Good day

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

Regards.

0candy removed the triaging label

Contributor

0candy commented Oct 28, 2016

@upupzealot Could you please rebase your code?

@raymondfeng Could you please review?

Author

upupzealot commented Oct 29, 2016

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

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 added 4 commits

October 31, 2016 13:47


          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.


          format the code

483fa95


          fix the long line

0f0cdf0


          add some test

811ac75

0candy force-pushed the master branch from d1363a2 to ebfaa8f Compare

October 31, 2016 18:20

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!

0candy force-pushed the master branch from ebfaa8f to f84fa6b Compare

October 31, 2016 18:24


          Fix indentation

071f8de

0candy force-pushed the master branch from f84fa6b to 071f8de Compare

October 31, 2016 18:25

cgole removed the #community contribution label

bajtos mentioned this pull request

Allow multiple ownership relations #3106

Closed

pierreclr reviewed

View reviewed changes

common/models/role.js

@@ @@ -210,26 +210,42 @@ module.exports = function(Role) { @@
                       if (callback) callback(null, matches(ownerId, userId));
                       return;
                     } else {

Contributor

pierreclr Jan 17, 2017

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

Author

upupzealot Jan 18, 2017

@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.

Member

bajtos commented Jan 19, 2017

Closing in favour of #3106

bajtos closed this

pierreclr mentioned this pull request

Fix OWNER role to handle multiple relations #3140

Merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels