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.

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

Regression test for Impersonating Troubleshooter #1697

Merged
merged 6 commits into from
Nov 1, 2023

Conversation

labkey-tchad
Copy link
Member

@labkey-tchad labkey-tchad commented Oct 27, 2023

Rationale

The new ImpersonatingTroubleshooterRole needs testing.
PermissionsEditor automatically saves after several operations, which makes it difficult to test error conditions. It isn't widely used however (it duplicates a lot of functionality from the more populare UIPermissionsHelper).

Story

Related Pull Requests

Changes

  • Regression test for Impersonating Troubleshooter
  • Additional regression tests for App Admin
  • Update PermissionsEditor test page
  • Make PermissionsHelper.getRoles static

@labkey-tchad labkey-tchad marked this pull request as ready for review October 30, 2023 18:38
Copy link
Contributor

@labkey-adam labkey-adam left a comment

Choose a reason for hiding this comment

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

Looks good to me. Just a couple optional suggestions.

@@ -40,11 +42,18 @@ public class AppAdminRoleTest extends BaseWebDriverTest
{
private static final String APP_ADMIN = "[email protected]";
private static final String USER = "[email protected]";
private static final String ADMIN_GROUP = "Custom Admin Group";
Copy link
Contributor

Choose a reason for hiding this comment

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

SITE_ADMIN_GROUP and "Custom Site Admin Group" for clarity?

public static final String DEVELOPER_ROLE = "Platform Developer";
public static final String IMP_TROUBLESHOOTER_ROLE = "Impersonating Troubleshooter";

public static String toRole(final String name)
Copy link
Contributor

@labkey-adam labkey-adam Oct 31, 2023

Choose a reason for hiding this comment

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

Should callers use the above constants to specify common roles instead of hard-coded strings?

@labkey-adam
Copy link
Contributor

Just FYI: I have a local change that disables the privileged roles on the permissions page for App Admins, which I think is a better experience. This test will need to be updated when (or before) that's merged.

@labkey-tchad labkey-tchad changed the base branch from develop to release23.11-SNAPSHOT November 1, 2023 21:45
@labkey-tchad labkey-tchad merged commit e295f75 into release23.11-SNAPSHOT Nov 1, 2023
@labkey-tchad labkey-tchad deleted the fb_permissionsEditorUpdate branch November 1, 2023 21:47
@labkey-danield labkey-danield restored the fb_permissionsEditorUpdate branch November 10, 2023 21:56
@labkey-danield labkey-danield deleted the fb_permissionsEditorUpdate branch November 10, 2023 21:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants