-
-
Notifications
You must be signed in to change notification settings - Fork 719
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
Allow only enterprise managers to connect apps #12507
Allow only enterprise managers to connect apps #12507
Conversation
Otherwise it doesn't work because non-managers, like super admins, are not authorised to access enterprise data via the DFC API.
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.
Great, nice simple update. I have a suggestion to make the message more verbose though.
@@ -1357,6 +1357,7 @@ en: | |||
enable: "Allow data sharing" | |||
disable: "Stop sharing" | |||
loading: "Loading" | |||
need_to_be_manager: "Only managers can connect apps." |
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.
Would this be a more helpful explanation? Helps work out how to solve the problem.
need_to_be_manager: "Only managers can connect apps." | |
need_to_be_manager: "You cannot connect an app because you are not a manager of this enterprise." |
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.
That would probably be more helpful. I kept it short because it fits better in that space. It's only seen by super-admins and therefore I wouldn't put too much effort into the message.
@@ -14,6 +14,10 @@ def select_only_item(producers) | |||
producers.size == 1 ? producers.first.id : nil | |||
end | |||
|
|||
def can_connect_apps?(enterprise) |
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 do you think about naming this for the condition? It's what the message talks about. Forgive me if I'm overthinking it now..
def managed_by_user?(enterprise)
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.
Yes, that sounds good. It is clearer for now because you know what the method is doing. If the condition changes in the future then we may need a different name again. Anyway, I think either is good but I applied your recommendation.
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.
Looks good !
Hey @mkllnk , I've verified that after staging and logging in as superadmin the connect button is disabled 🟢 It remains enabled for both enterprise owners 🟢 And managers 🟢 (the orange line is meant to indicate, that for this enterprise, button remains with the endless spinner, but this is not introduced by this PR. The button is correctly displayed for other enterprises, and works as expected). I've noticed though, that the user is sent to the login screen, when clicking the Disconnect app button. There seems to be some inconsistent behavior, on this regard. I'll need to investigate master, to be sure this is not introduced by this PR.
|
What? Why?
Connecting apps only works for enterprise managers because non-managers, like super admins, are not authorised to access enterprise data via the DFC API.
What should we test?
Release notes
Changelog Category (reviewers may add a label for the release notes):
The title of the pull request will be included in the release notes.
Dependencies
Documentation updates