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

feat: userroles and permissions #123

Merged
merged 67 commits into from
Nov 23, 2023
Merged

feat: userroles and permissions #123

merged 67 commits into from
Nov 23, 2023

Conversation

Chakravarthy7102
Copy link
Contributor

@Chakravarthy7102 Chakravarthy7102 commented Oct 18, 2023

Summary of change

Add user roles and permissions feature to the dashboard.

Problem Statement

Figma

TODOS

  • handle bad requests
  • should reset server index.ts file.

(Overview of how the problem is solved by this PR)

Related issues

  • Link to issue1 here
  • Link to issue1 here

Test Plan

Tested on all primary browsers for:

  • Chrome
    • Desktop
    • Mobile
    • Tablet
  • Safari
    • Desktop
    • Mobile
    • Tablet
  • Firefox
    • Desktop
    • Mobile
    • Tablet
  • (Optional) Tested on Safari 12 (Physical or emulator)
    • iPad
    • iPhone
  • (Optional) Tested on physical mobile and tablet device for:
    • Android
    • iOS (including iPadOS)

Feature tests:

  • Search
    • Search with anything that results in an empty state in the UI (Should show an empty state explaining that there were no results)
    • Search with an empty string (Dashboard should not allow this)
    • Email search
    • Serach for "e" with email tag and then delete the tag (Should show one user initially then show all with pagination enabled)
    • Search for "test" with the email tag (Expect 14 results)
    • Search with "g" for email tag (Expect 3 results) !(More than expected results are shown)
    • Search with "g" and "p" for email tag (Expect 3 results)
    • Search with "g" and "t" for email tag, then delete "t" (Expect 17 results initially, then 3)
    • Search with "@" for email tag (Expect 0 results (This is because at the time of adding this case we would only check for the start of the email or the domain and not any character inside the full email))
    • Search for "[email protected]" with email tag (Expect 1 result) !(No results)
    • Search for "[email protected]" with email tag (Expect 3 results) !(No results)
    • Search for "gmail" with email tag (Expect 3 results)
    • Search for "ABC" with email tag (Expect 0 results)
    • Search for "a" with email tag (Expect 0 results (This is because at the time of adding this we only check if the email starts with the query and not contains))
    • Search for "team" with email tag !(Expect 0 results) (showing more than 1 result)
    • Search for "782" with email tag !(Expect 1 result) (No results are shown)
    • Phone search
    • Search for "1" with phone tag (Expect 3 results)
    • Search for "+1" with phone tag (Expect 3 results)
    • Search for "91" with phone tag (Expect 1 result)
    • Search for "291" with phone tag (Expect 0 results)
    • Search for "+12" with phone tag (Expect 2 results) !(it only gives 1 results)
    • Search for "5" with phone tag (Expect 0 results) !(shows 1 result)
    • Search for "1(" with phone tag (Expect 0 results (This is because we render phone numbers with brackets so users may end up searching with that))
    • Provider search
    • Search for "g" with provider tag (Expect 5 results) !(More than 5 are shown)
    • Search for "gi" with provider tag (Expect 2 results) !(More than 2 are shown)
    • Search for "t" with provider tag (Expect 0 results) !(1 result)
    • Search for "google" with provider tag (Expect 3 results) !(more than 3 are shown)
    • Combination testing
    • Search with "g" for email tag and "g" for provider tag (Expect 1 result) !(More than 1 result)
    • Search for "github" with provider tag and "782" with email tag (Expect 1 result) !(No results)
    • Search for "github" and "google" with provider tag (Expect 5 results) !(Gives more than 5 results)
    • Search for "j" and "g" with email tag (Expect 6 results)
    • Search for "1" and "91" with phone tag (Expect 4 results)
    • Search with "google" for provider tag and "1" for phone tag (Expect 0 results)
    • Search for "g" with email tag and "1" with phone tag (Expect 0 results)
    • Search for "k" with provider and "a", "g", "b" (in that order) for email (Expect 0 resutls)
  • General UI testing
    • Test that emty state renders fine (no overflow, no UI glitches, responsiveness etc)
    • Test that the list renders fine (no overflow, no UI glitches, responsiveness etc)
    • Test that pagination is visiable and usable (There should be at least 2 pages worth of users)
    • Test that the list only shows 10 users at a time
    • Test that for users with no accounts linked the auth method i nthe list is correct
    • Test that for users with multiple login methods, the auth method shows correctly
    • Test that search is visible if the feature is enabled
  • Multi tenant testing
    • Create one tenant (tenant1), and add 3 users to them. In the dashboard, when you switch to that tenant, it should list those users.

Documentation changes

(If relevant, please create a PR in our docs repo, or create a checklist here highlighting the necessary changes)

Checklist for important updates

  • Changelog has been updated
  • Changes to the version if needed
    • In package.json
    • In package-lock.json
    • In src/version.ts
  • Had run npm run build
  • Had installed and ran the pre-commit hook

Remaining TODOs for this PR

  • Item1
  • Item2

@Chakravarthy7102 Chakravarthy7102 marked this pull request as draft October 18, 2023 11:10
@Chakravarthy7102 Chakravarthy7102 marked this pull request as ready for review November 2, 2023 05:36
@@ -88,6 +89,7 @@ SuperTokens.init({
}),
Session.init(),
AccountLinking.init(),
UserRoles.init(),
Copy link
Contributor

Choose a reason for hiding this comment

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

need to test when this is not done.

src/ui/components/footer/footer.scss Show resolved Hide resolved
src/ui/components/footer/footer.scss Show resolved Hide resolved
src/ui/components/pagination/index.tsx Show resolved Hide resolved
src/ui/components/userDetail/userDetail.tsx Outdated Show resolved Hide resolved
};

async function fetchPermissionsForRoles() {
setIsFetchingRoles(true);
Copy link
Contributor

Choose a reason for hiding this comment

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

need to fetch permissions per role. Can't block the whole UI

permissions: response.permissions,
});
} else {
throw new Error("This should never happen.");
Copy link
Contributor

Choose a reason for hiding this comment

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

this kind of pattern shoul dbe avoided

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

} else {
throw new Error("This should never happen.");
}
await new Promise((res) => setTimeout(res, 250));
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
await new Promise((res) => setTimeout(res, 250));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done


const [rolesRawResponse, setRolesRawResponse] = useState<string[]>([]);
// used to store roles with permissions data that are fetched on the client side.
const [roles, setRoles] = useState<Role[]>([]);
Copy link
Contributor

Choose a reason for hiding this comment

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

type of role will change too. To set permissions array as undefined.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

const { showToast } = useContext(PopupContentContext);

// list of roles fetched from the api
const [roles, setRoles] = useState<string[]>([]);
Copy link
Contributor

Choose a reason for hiding this comment

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

should be undefined.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -19,7 +19,6 @@ import { getDashboardAppBasePath } from "./utils";

Copy link
Contributor

Choose a reason for hiding this comment

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

add test in PR checklist for testing assigning / managing roles for a user who is a part of multiple tenants.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will do.

src/ui/components/userDetail/userRoles/UserRolesList.tsx Outdated Show resolved Hide resolved
selectedOption: string;
onOptionSelect: (value: string) => void;
};
export default function Select({ onOptionSelect, options, selectedOption }: SelectProps) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Use the browser selector and style that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a click action to the select component to handle mobile and tablet devices. as styling native select element is really a overhead.

const [userRoles, setUserRoles] = useState<string[] | undefined>(undefined);
const [isUserRolesFeatureEnabled, setIsUserRolesFeatureEnabled] = useState<boolean | undefined>(undefined);
const [userRolesData, setUserRolesData] = useState<UserRolesResponse | undefined>(undefined);
const [currentlySelectedTenantId, setCurrentlySelectedTenantId] = useState(getSelectedTenantId() ?? "public");
Copy link
Contributor

Choose a reason for hiding this comment

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

the default should be based on the tenant that the user belongs to - not from what comes from the user listing page.

Copy link
Contributor

Choose a reason for hiding this comment

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

If the user belongs to the public tenant, show that, else whatever is the first item in the list of their tenants.

void fetchUserRoles();
}, [currentlySelectedTenantId]);

if (userDetail === undefined || userRolesData === undefined || isLoading) {
Copy link
Contributor

Choose a reason for hiding this comment

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

remove userRolesData === undefined

Copy link
Contributor

Choose a reason for hiding this comment

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

Have the loading thing just in the user details section.

setCurrentlySelectedTenantId(tenantId);
}}
isFeatureEnabled={userRolesData.status !== "FEATURE_NOT_ENABLED_ERROR"}
roles={userRolesData.status === "OK" ? userRolesData.roles : []}
Copy link
Contributor

Choose a reason for hiding this comment

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

for isFeatureEnabled and roles, we can't just assume true and [] in case userRolesData is undefined.

import AssignRolesDialog from "../../userroles/components/dialogs/AssignRoles";
import DeleteUserRoleDialog from "../../userroles/components/dialogs/DeleteUserRole";
import { useUserDetailContext } from "../context/UserDetailContext";
import "./userRolesList.scss";

type UserRolesListProps = {
currentlySelectedTenantId: string;
userId: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

not needed

const [showDeleteRoleDialog, setShowDeleteDialogRole] = useState(false);

const tenantIdsThatUserIsPartOf = userDetail.details.tenantIds;
const [currentlySelectedTenantId, setCurrentlySelectedTenantId] = useState(tenantIdsThatUserIsPartOf[0]);
Copy link
Contributor

Choose a reason for hiding this comment

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

if they are a part of the public tenant, we show that, else we show the 0th index.

@rishabhpoddar rishabhpoddar merged commit 80ad365 into master Nov 23, 2023
3 checks passed
@rishabhpoddar rishabhpoddar deleted the feat/userroles branch November 23, 2023 06:37
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.

2 participants