-
Notifications
You must be signed in to change notification settings - Fork 142
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.
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
GraphQL API: Site Permissions #880
base: the-future
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
couple small questions, but love it!
833a1b9
to
e92dc68
Compare
9b471f3
to
365247d
Compare
365247d
to
32233c2
Compare
Code Climate has analyzed commit 62ad2f3 and detected 0 issues on this pull request. View more on Code Climate. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some questions, but looks good for the most part!
res = Accounts::SetSitePermissions.call( | ||
user: input.account, | ||
permissions: permissions | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What type of errors can happen here and should we account for them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UnknownPermissions
could happen due to a programming error but otherwise it won't happen, and the only other ones are RecordNotFound
or RecordInvalid
which I believe are accounted for automatically
def load_account(value) | ||
::User.find(value) | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want this here, or inside the mutation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here, because we use it in authorized?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ooooh, I see what you've done. If you want to go that route shouldn't we rename the value
argument to id
to be more specific?
Also unless I am thinking of this wrong from the load_account
it takes what that returns and passes that along to the authorized?
method (which would be a user) not an input?
I'll test this locally tomorrow because either this won't work at all or you are about to teach me something new (and I'm hoping it is the latter because this really opens up the possibilities).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think I'm doing anything weird, I'm pretty sure I've just copied it from your stuff… my thinking is that this should run on the input type to load the record and then when authorized?
runs it's available as input.account
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the load_*
isn't actually related to an input
. Its the graphql-ruby gem magic and it must be in the mutation file itself.
What
Why
Whenever we add and remove moderators, I have to pop into a rails console to manually do it. This is the first step to on-site permission management.
Checklist