lahirujayathilake commented on code in PR #528:
URL: https://github.com/apache/airavata-custos/pull/528#discussion_r3591926144


##########
web/src/app/(portal)/admin/users/management/IdentitiesCell.tsx:
##########
@@ -1,34 +1,21 @@
-// Licensed to the Apache Software Foundation (ASF) under one
-// or more contributor license agreements.  See the NOTICE file
-// distributed with this work for additional information
-// regarding copyright ownership.  The ASF licenses this file
-// to you under the Apache License, Version 2.0 (the
-// "License"); you may not use this file except in compliance
-// with the License.  You may obtain a copy of the License at
-//
-//   http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing,
-// software distributed under the License is distributed on an
-// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
-// KIND, either express or implied.  See the License for the
-// specific language governing permissions and limitations
-// under the License.
-

Review Comment:
   License header must not be deleted. Check the other files too.



##########
web/src/features/core/users/schemas.ts:
##########
@@ -0,0 +1,69 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0.
+
+import { z } from "zod";
+import { zRole, zUser, zUserIdentity, zUserPrivilege, zUserRole } from 
"@/generated/core/zod.gen";
+
+export const privilegeKeySchema = z.string().min(1);

Review Comment:
   this, and line numbers 19, 25, 53  already exist in 
features/core/identity/schemas.ts. Import them instead



##########
web/src/features/core/users/api.ts:
##########
@@ -0,0 +1,66 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0.

Review Comment:
   use the correct license



##########
web/src/features/core/users/api.ts:
##########
@@ -0,0 +1,66 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0.
+
+import { apiFetch } from "@/shared/api/client";
+import {
+  grantRoleResponseSchema,
+  roleDetailResponseSchema,
+  rolesResponseSchema,
+  userIdentitiesResponseSchema,
+  userListResponseSchema,
+  userPrivilegesResponseSchema,
+  userRolesResponseSchema,
+} from "./schemas";
+import type {
+  Role,
+  RoleDetail,
+  UserIdentity,
+  UserListResponse,
+  UserPrivilege,
+  UserRole,
+} from "./schemas";
+import type { UserListParams } from "./types";
+
+export async function listUsers(params: UserListParams = {}): 
Promise<UserListResponse> {
+  const search = new URLSearchParams();
+  if (typeof params.limit === "number") search.set("limit", 
String(params.limit));
+  if (typeof params.offset === "number") search.set("offset", 
String(params.offset));
+  const query = search.toString();
+  return userListResponseSchema.parse(await apiFetch(`/users${query ? 
`?${query}` : ""}`));
+}
+
+export async function listRolesCatalog(): Promise<Role[]> {
+  return rolesResponseSchema.parse(await apiFetch("/roles"));
+}
+
+export async function listRolesForUser(userId: string): Promise<UserRole[]> {
+  return userRolesResponseSchema.parse(await 
apiFetch(`/users/${userId}/roles`));
+}
+
+export async function listUserIdentities(userId: string): 
Promise<UserIdentity[]> {
+  return userIdentitiesResponseSchema.parse(await 
apiFetch(`/users/${userId}/user-identities`));
+}
+
+export async function listDirectPrivileges(userId: string): 
Promise<UserPrivilege[]> {
+  return userPrivilegesResponseSchema.parse(await 
apiFetch(`/users/${userId}/privileges`));
+}
+
+export async function getRoleDetail(roleId: string): Promise<RoleDetail> {
+  return roleDetailResponseSchema.parse(await apiFetch(`/roles/${roleId}`));
+}
+
+export async function assignUserRole(userId: string, roleId: string): 
Promise<UserRole> {

Review Comment:
   `POST /users/{id}/roles1 accepts a reason and the backend stores it. Nothing 
here ever sends one. Worth capturing an optional reason in the dialog so role 
grants have an audit trail.



##########
web/src/app/(portal)/admin/users/management/RoleAssignMenu.tsx:
##########
@@ -1,24 +1,7 @@
-// Licensed to the Apache Software Foundation (ASF) under one

Review Comment:
   don't remove license header



##########
web/src/app/(portal)/admin/users/management/PrivilegeList.tsx:
##########
@@ -0,0 +1,52 @@
+import { Badge } from "@/shared/ui/badge";

Review Comment:
   Missing Apache2 license headers. Valid for the rest of the files.



##########
web/src/app/(portal)/admin/users/management/UsersTable.tsx:
##########
@@ -191,26 +214,51 @@ export function UsersTable() {
         </select>
       </div>
 
+      {filtersActive ? (
+        <p className="text-sm text-muted-foreground">
+          Showing {filteredUsers.length} match{filteredUsers.length === 1 ? "" 
: "es"} on this page;{" "}
+          {total} users total.
+        </p>
+      ) : null}
+
       <DataTable
         columns={columns}
         rows={filteredUsers}
-        rowKey={(row) => row.id ?? row.email ?? ""}
+        rowKey={(row) => row.id}
         onRowClick={(row) => {
-          if (row.id) expandedRow.collapseUnless(row.id);
-          setSelectedId((prev) => (prev === row.id ? null : (row.id ?? null)));
+          expandedRow.collapseUnless(row.id);
+          setSelectedId((previous) => (previous === row.id ? null : row.id));
         }}
         rowClassName={(row) =>
-          row.id && row.id === selectedId
-            ? "bg-[color:var(--brand-tint)] hover:bg-[color:var(--brand-tint)]"
+          isCurrentUser(row)
+            ? "bg-[color:var(--custos-blue-50)]/40 
hover:bg-[color:var(--custos-blue-50)]/60"
             : undefined
         }
         empty={
           <span className="text-sm text-muted-foreground">
-            No users match the current filters.
+            {filtersActive ? "No users match on this page." : "No users 
found."}
           </span>
         }
+        pagination={{
+          page,
+          pageSize,
+          total,
+          pageSizeOptions: [25],

Review Comment:
   only one value, either add options or drop it



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to