-
Notifications
You must be signed in to change notification settings - Fork 76
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
various permission fixes for master on prem #305
Conversation
cee97cd
to
fe8981e
Compare
|
||
# end_users account_plans service_plans finance require_cc_on_signup | ||
# multiple_services multiple_applications multiple_users skip_email_engagement_footer | ||
# groups branding web_hooks iam_tools |
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.
This is veeeeery easy to get outdated. If anybody changes this in Settings::SWITCHES
, nobody will remember that it was also here 🤔
Maybe we should have a test/unit/abilities/switches_test.rb
that tests this file and as it will be referencing Settings::SWITCHES
dynamically and in a test, it won't be outdated, it will be easy to find and good for documentation purposes as well
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.
It's just there to find it when you search in text editor, it's copied from https://github.com/3scale/porta/blob/master/app/models/settings.rb#L15 and basically a nudge that these are not 'normal' abilities. Agreed that this is not a definitive solution. It's a temp patch…
@@ -18,7 +18,6 @@ Feature: API menu | |||
| ActiveDocs | | |||
| Integration | | |||
|
|||
@javascript |
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.
If it keeps working, better without JS, yup 😄
@@ -10,15 +10,14 @@ | |||
title: 'Trash', | |||
path: provider_admin_messages_trash_index_path | |||
|
|||
- if can?(:manage, :settings) | |||
- if can?(:manage, :settings) && !master_on_premises? |
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.
Right, better 👍
icon: 'cubes', | ||
vertical_nav_item: vertical_nav_item, | ||
submenu: :applications } | ||
- if can?(:manage, :applications) |
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.
Right 👍
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.
Good 😄 Only that this has massive chances to get outdated: https://github.com/3scale/porta/pull/305/files#r231933652
But it is not the end of the world I guess 😄
closes:
https://issues.jboss.org/browse/THREESCALE-1461
https://issues.jboss.org/browse/THREESCALE-1476