Skip to content

Conversation

smacker
Copy link
Contributor

@smacker smacker commented Feb 8, 2018

Fixes #44

Signed-off-by: Maxim Sukharev <maxim@sourced.tech>
@smacker smacker requested a review from bzz February 8, 2018 19:44
@bzz bzz requested a review from carlosms February 9, 2018 09:09
@bzz
Copy link
Contributor

bzz commented Feb 9, 2018

\cc @carlosms for review

@carlosms
Copy link
Contributor

carlosms commented Feb 9, 2018

Maybe we should restrict this route only for users with the Requester role. Do you agree?

@smacker
Copy link
Contributor Author

smacker commented Feb 9, 2018

we don't have ACL yet. But we should later, yes.

@carlosms
Copy link
Contributor

carlosms commented Feb 9, 2018

The roles are already in models.go, and #60 should be merged soon. I'd make this change to save us a new PR in a few hours. Up to you.

Otherwise, looks good.

@smacker
Copy link
Contributor Author

smacker commented Feb 9, 2018

You won't be able to test it after merge then. I prefer going step by step. In my opinion, the right place to implement it is connection FE to BE. Because the same restriction will be needed on FE side.

@carlosms
Copy link
Contributor

carlosms commented Feb 9, 2018

I'm ok with any option as long as we don't forget about it. Can you please make a new issue, or add a comment to the relevant issue to make sure we don't forget? Thank you.

@smacker
Copy link
Contributor Author

smacker commented Feb 9, 2018

@carlosms added in #47

Copy link
Contributor

@bzz bzz left a comment

Choose a reason for hiding this comment

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

LGTM

@smacker smacker merged commit 32df796 into src-d:master Feb 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants