Fixes #20264: Allow to customize in which menu plugins are set #3993
Conversation
|
Commit modified |
VinceMacBuche
force-pushed
the
ust_20264/allow_to_customize_in_which_menu_plugins_are_set
branch
from
55252d2
to
9de4c53
Compare
| @@ -356,28 +356,28 @@ class Boot extends Loggable { | |||
|
|
|||
| // All the following is related to the sitemap | |||
| val nodeManagerMenu = | |||
| (Menu("NodeManagerHome", <i class="fa fa-sitemap"></i> ++ <span>Node management</span>: NodeSeq) / | |||
| (Menu("10", <i class="fa fa-sitemap"></i> ++ <span>Node management</span>: NodeSeq) / | |||
There was a problem hiding this comment.
I think we should keep the number only as a prefix, like what you did for plugin below, because:
- it will make debugging in some cases easier (IIRC, we have a map of menu id -> menu at some places, and having only "10" won't help much)
- having globally unique name is less risky if at some point we make a map of all menu items (including sub menu).
There was a problem hiding this comment.
these name will become part of the api (since they can be used by plugin), we need to normalize them, I propose:
- two digits followed by
- - lower case, no space,
-between words
There was a problem hiding this comment.
OK, using a name is perhaps not super futur proof, because it will be hard to rename that id. I'm not sur it is of importance (we can still rename between major version, since plugin are not compatible between them).
| "secure" / (name+"Manager") / "ruleManagement" | ||
| >> LocGroup(name+"Group") | ||
| >> TestAccess( () => userIsAllowed("/secure/index",AuthorizationType.Rule.Read ) ) | ||
|
|
||
| , Menu("Rule Details", <span>Rule</span>) / | ||
| , Menu("10", <span>Rule</span>) / |
There was a problem hiding this comment.
I don't think we should authorize same menu idea, I'm pretty sure it will create unspecified behavior
| "secure" / "utilities" / "healthcheck" | ||
| >> LocGroup("utilitiesGroup") | ||
| >> TestAccess ( () => userIsAllowed("/secure/index",AuthorizationType.Administration.Read) ) | ||
| ) | ||
| } | ||
|
|
||
| val rootMenu = List( | ||
| Menu("Dashboard", <i class="fa fa-dashboard"></i> ++ <span>Dashboard</span>: NodeSeq) / "secure" / "index" | ||
| Menu("0", <i class="fa fa-dashboard"></i> ++ <span>Dashboard</span>: NodeSeq) / "secure" / "index" |
| "secure" / "administration" / "apiManagement" | ||
| >> LocGroup("administrationGroup") | ||
| >> TestAccess ( () => userIsAllowed("/secure/administration/policyServerManagement",AuthorizationType.Administration.Write) ) | ||
| ) | ||
|
|
||
| def pluginsMenu = { | ||
| (Menu("PluginsHome", <i class="fa fa-puzzle-piece"></i> ++ <span>Plugins</span> ++ <span data-lift="PluginExpirationInfo.renderIcon"></span>: NodeSeq) / | ||
| (Menu("50 - plugins", <i class="fa fa-puzzle-piece"></i> ++ <span>Plugins</span> ++ <span data-lift="PluginExpirationInfo.renderIcon"></span>: NodeSeq) / |
There was a problem hiding this comment.
you should not use that with the toIntOption. But I'm not sure we should convert to int at all.
| "secure" / "plugins" / "index" | ||
| >> LocGroup("pluginsGroup") | ||
| >> TestAccess ( () => userIsAllowed("/secure/index", AuthorizationType.Administration.Read) | ||
| )) submenus ( | ||
| Menu("pluginInformation", <span>Plugin information</span>) / | ||
| Menu("0", <span>Plugin information</span>) / |
There was a problem hiding this comment.
I think I would use 01 here to let the possibility to have things before in the future, in the middle of a release cycle.
| val menu = if (menus.exists (_.loc.name == name )) { | ||
| menus | ||
| } else { | ||
| (parentMenu :: menus).sortBy(_.loc.name.toIntOption.getOrElse(Int.MaxValue)) |
There was a problem hiding this comment.
I think you can use the same logic without the toInt
VinceMacBuche
force-pushed
the
ust_20264/allow_to_customize_in_which_menu_plugins_are_set
branch
from
9de4c53
to
fc06b00
Compare
|
Commit modified |
|
Commit modified |
VinceMacBuche
force-pushed
the
ust_20264/allow_to_customize_in_which_menu_plugins_are_set
branch
from
fc06b00
to
6ed61e4
Compare
|
This PR is not mergeable to upper versions. |
|
OK, merging this PR |
https://issues.rudder.io/issues/20264