-
Notifications
You must be signed in to change notification settings - Fork 81
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
New Test ODS-2223 #1101
New Test ODS-2223 #1101
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.
Robocop found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
Robot Results
|
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.
LGTM
ods_ci/tests/Resources/Page/ODH/ODHDashboard/ODHDataScienceProject/Permissions.resource
Outdated
Show resolved
Hide resolved
...ods_dashboard/415__ods_dashboard_projects/415__ods_dashboard_projects_permissions_mgmt.robot
Show resolved
Hide resolved
...ods_dashboard/415__ods_dashboard_projects/415__ods_dashboard_projects_permissions_mgmt.robot
Outdated
Show resolved
Hide resolved
...ods_dashboard/415__ods_dashboard_projects/415__ods_dashboard_projects_permissions_mgmt.robot
Outdated
Show resolved
Hide resolved
...ods_dashboard/415__ods_dashboard_projects/415__ods_dashboard_projects_permissions_mgmt.robot
Outdated
Show resolved
Hide resolved
...ods_dashboard/415__ods_dashboard_projects/415__ods_dashboard_projects_permissions_mgmt.robot
Outdated
Show resolved
Hide resolved
Signed-off-by: manosnoam <[email protected]>
Signed-off-by: manosnoam <[email protected]>
Signed-off-by: manosnoam <[email protected]>
Signed-off-by: manosnoam <[email protected]>
Signed-off-by: manosnoam <[email protected]>
Signed-off-by: manosnoam <[email protected]>
Signed-off-by: manosnoam <[email protected]>
Signed-off-by: manosnoam <[email protected]>
Signed-off-by: manosnoam <[email protected]>
Signed-off-by: manosnoam <[email protected]>
Signed-off-by: manosnoam <[email protected]>
Signed-off-by: manosnoam <[email protected]>
Signed-off-by: manosnoam <[email protected]>
Thanks @bdattoma , @FedeAlonso I resolved the requested issues, and retested: |
|
User ${USER_B} Should Not Be Allowed To Dashboard | ||
User ${USER_C} Should Not Be Allowed To Dashboard | ||
[Teardown] Run Keywords Set Default Access Groups Settings | ||
... AND Set User Groups For Testing |
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.
wait, why running Set User Groups For Testing
in the test teardown? I think it's not needed. You don't see issues because there is the Suite Teardown which restores the default settings.
I believe you can remove the [Teardown]
here and let the suite teardown to restore default user settings
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 think it is safer to run Set User Groups For Testing
in test teardown, in case the test is not the last one in suite, so if another test is called afterwards, then the expected Users are already set properly.
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.
hmm I think you could actually avoid lines 92 and 94 because those are already executed in the Suite Setup. If you want to remove the users from the rhods-admin group as well, you can also add it to the Suite Setup (and change the Suite Teardown accordingly)
Alternatively, instead of using Set RHODS Users Group To rhods-users
you can do Set RHODS Users Group To ${USER_GROUP_1}
and Set RHODS Users Group To ${USER_GROUP_1}
. This should remove lines from 91 to 95 + 98-99
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 understand now why you did it as you did, it works of course. However it think it could be cleaner as I wrote in the above comment.
If you disagree, I will approve anyways
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.
@bdattoma the Test case steps to automate say:
assign "Admin" permissions to a user B who does not belong to any RHODS user group
assign "Edit" permissions to a user C who does not belong to any RHODS user group
The existing KW Set RHODS Users Group To
doesn't do that exactly, as it does not specifically make the user not part of a group. It modifies oc patch OdhDashboardConfig odh-dashboard-config
, which is a slightly different scenario.
This is why I use KW Remove User From Group
rhods-users and rhods-admin, which does:
oc adm groups remove-users ${group_name} ${username}
I think this is good enough for the scenario, we can later extend this scenario if needed.
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 know, but the suite setup already removes users from group. Besides, I'm suggesting to use ${USER_GROUP_1}
for both admin and basic user group because the USER_B and USER_C you're using are not part of that group.
LGTM, apart for my last comment, I didn't notice earlier sorry. @manosnoam |
I cannot count on the suite setup, if previous test could alter the users,
that's why I make sure the users are ready for the test.
I don't see any advantage in changing this logic if it works good, and if
we change the logic, it will require further tests and rebase. I think only
if you see an actual problem in this logic, we should fix it before merging.
…On Tue, Jan 9, 2024, 10:40 bdattoma ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In
ods_ci/tests/Tests/400__ods_dashboard/415__ods_dashboard_projects/415__ods_dashboard_projects_permissions_mgmt.robot
<#1101 (comment)>
:
> @@ -76,13 +82,31 @@ Verify User Can Assign Access Permissions To User Groups
Sleep 5s
${USER_C} Should Not Have Access To ${PRJ_USER_B_TITLE}
+Verify Project Sharing Does Not Override Dashboard Permissions
+ [Tags] Tier1 Sanity ODS-2223
+ [Setup] Set RHODS Users Group To rhods-users
+ Launch Data Science Project Main Page username=${OCP_ADMIN_USER.USERNAME} password=${OCP_ADMIN_USER.PASSWORD}
+ ... ocp_user_auth_type=${OCP_ADMIN_USER.AUTH_TYPE}
+ Assign Admin Permissions To User ${USER_B} in Project ${PRJ_USER_B_TITLE}
+ Assign Edit Permissions To User ${USER_C} in Project ${PRJ_USER_C_TITLE}
+ Remove User From Group username=${USER_B} group_name=rhods-users
+ Remove User From Group username=${USER_B} group_name=rhods-admins
+ Remove User From Group username=${USER_C} group_name=rhods-users
+ Remove User From Group username=${USER_C} group_name=rhods-admins
+ User ${USER_B} Should Not Be Allowed To Dashboard
+ User ${USER_C} Should Not Be Allowed To Dashboard
+ [Teardown] Run Keywords Set Default Access Groups Settings
+ ... AND Set User Groups For Testing
I know, but the suite setup already removes users from group. Besides, I'm
suggesting to use ${USER_GROUP_1} for both admin and basic user group
because the USER_B and USER_C you're using are not part of that group.
—
Reply to this email directly, view it on GitHub
<#1101 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACLUMWLJADY4E3PUGPZ3ZGLYNT66XAVCNFSM6AAAAABBQS2C6CVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTQMJQGY2DSMBXGA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Add New Test ODS-2223:
Verify permissions set as part of Project Sharing do not override Dashboard permission.
Including: