Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fixes #23840: Adding property on nodes is not possible with role node_all #5236

Merged
merged 1 commit into from
Dec 8, 2023
Merged

Fixes #23840: Adding property on nodes is not possible with role node_all #5236

merged 1 commit into from
Dec 8, 2023

Conversation

ElaadF
Copy link
Member

@ElaadF ElaadF commented Dec 6, 2023 •
edited
Loading

@ElaadF ElaadF added the WIP Use that label for a Work In Progress PR that must not be merged yet label Dec 6, 2023
@ElaadF
Copy link
Member Author

ElaadF commented Dec 6, 2023

Commit modified

@ElaadF ElaadF force-pushed the bug_23840/adding_property_on_nodes_is_not_possible_with_role_node_all branch from 03b52ff to 55bebea Compare December 6, 2023 13:59
@ElaadF
Copy link
Member Author

ElaadF commented Dec 6, 2023

Commit modified

@ElaadF ElaadF force-pushed the bug_23840/adding_property_on_nodes_is_not_possible_with_role_node_all branch from 55bebea to 6aea3d5 Compare December 6, 2023 14:03
@ElaadF
Copy link
Member Author

ElaadF commented Dec 6, 2023

Commit modified

@ElaadF ElaadF force-pushed the bug_23840/adding_property_on_nodes_is_not_possible_with_role_node_all branch from 6aea3d5 to 1f7c0cb Compare December 6, 2023 14:13
@ElaadF ElaadF removed the WIP Use that label for a Work In Progress PR that must not be merged yet label Dec 6, 2023
ElaadF
ElaadF commented Dec 6, 2023
View reviewed changes
...s/rudder/rudder-web/src/main/webapp/templates-hidden/components/ComponentNodeProperties.html Outdated
</script>
</lift:authz>
<lift:authz role="node_read">
<script>
hasReadRights = true;
hasReadRightsBackend = true;
Copy link
Member Author

Choose a reason for hiding this comment

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

I have no idea why for read rights it was working, but not for write...

ElaadF
ElaadF commented Dec 6, 2023
View reviewed changes
...s/rudder/rudder-web/src/main/webapp/templates-hidden/components/ComponentNodeProperties.html Outdated
@@ -4,17 +4,17 @@ <h3 class="page-title foldable" onclick="$('#nodeProp').toggle(); $(this).toggle
<div id="nodeproperties-app" class="portlet-content"></div>
<script data-lift="with-cached-resource" src="/javascript/rudder/elm/rudder-nodeproperties.js"></script>
<script>
var hasWriteRights = false;
var hasReadRights = false;
var hasWriteRightsBackend = false;
Copy link
Member Author

@ElaadF ElaadF Dec 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

It seemed that the write was override by something else, but what.. ? Maybe because this is shared between node group properties?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe the variable should be "CanWriteNode" and "CanReadNode"

VinceMacBuche
VinceMacBuche reviewed Dec 6, 2023
View reviewed changes
...sources/rudder/rudder-web/src/main/scala/com/normation/rudder/web/services/DisplayNode.scala Outdated
var main = document.getElementById("nodeproperties-app")
|var initValues = {
| contextPath : "${S.contextPath}"
| , hasWriteRights : hasWriteRights
| , hasReadRights : hasReadRights
| , hasWriteRights : hasWriteRightsBackend
Copy link
Member

Choose a reason for hiding this comment

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

Can you modify it to "hasNodeWrite" ?

@ElaadF
Copy link
Member Author

ElaadF commented Dec 6, 2023

PR updated with a new commit

@Normation-Quality-Assistant
Copy link
Contributor

This PR is not mergeable to upper versions.
Since it is "Ready for merge" you must merge it by yourself using the following command:
rudder-dev merge https://github.com/Normation/rudder/pull/5236
-- Your faithful QA
Kant merge: "Morality is not the doctrine of how we may make ourselves happy, but how we may make ourselves worthy of happiness."
(https://ci.normation.com/jenkins/job/merge-accepted-pr/77193/console)

@ElaadF
Copy link
Member Author

ElaadF commented Dec 8, 2023

OK, squash merging this PR

@ElaadF ElaadF force-pushed the bug_23840/adding_property_on_nodes_is_not_possible_with_role_node_all branch from 93b83fd to 9482f91 Compare December 8, 2023 08:48
@ElaadF ElaadF merged commit 9482f91 into Normation:branches/rudder/8.0 Dec 8, 2023
0 of 15 checks passed
@ElaadF ElaadF mentioned this pull request Dec 12, 2023
Merged
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@VinceMacBuche VinceMacBuche VinceMacBuche approved these changes

Assignees
No one assigned
Labels
qa: Can't merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ElaadF @Normation-Quality-Assistant @VinceMacBuche