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

Users admin tab #2672

Merged
merged 2 commits into from
Nov 4, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions backend/src/database/repositories/userRepository.ts
Original file line number Diff line number Diff line change
Expand Up @@ -384,6 +384,7 @@ export default class UserRepository {
as: 'tenants',
where: {
tenantId: currentTenant.id,
status: 'active',
},
})
}
Expand All @@ -410,6 +411,15 @@ export default class UserRepository {
whereAnd.push(SequelizeFilterUtils.ilikeIncludes('user', 'email', filter.email))
}

if (filter.query) {
whereAnd.push({
[Op.or]: [
SequelizeFilterUtils.ilikeIncludes('user', 'fullName', filter.query),
SequelizeFilterUtils.ilikeIncludes('user', 'email', filter.query),
],
})
}

if (filter.role) {
const innerWhereAnd: Array<any> = []

Expand Down
6 changes: 6 additions & 0 deletions frontend/src/modules/admin/models/User.model.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
export interface UserModel {
id: string;
email: string;
fullName: string;
roles: string[];
}
152 changes: 152 additions & 0 deletions frontend/src/modules/admin/pages/users.page.vue
Original file line number Diff line number Diff line change
@@ -0,0 +1,152 @@
<template>
<div>
<div class="pt-6 pb-6">
<lf-search v-model="search" class="h-9" :lazy="true" placeholder="Search users..." @update:model-value="searchUsers()" />
</div>
Comment on lines +3 to +5
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add debounce to search input to prevent excessive API calls.

The search input's lazy prop might not be sufficient to prevent rapid API calls as users type.

-      <lf-search v-model="search" class="h-9" :lazy="true" placeholder="Search users..." @update:model-value="searchUsers()" />
+      <lf-search
+        v-model="search"
+        class="h-9"
+        :lazy="true"
+        :debounce="300"
+        placeholder="Search users..."
+        @update:model-value="searchUsers()"
+      />
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
<div class="pt-6 pb-6">
<lf-search v-model="search" class="h-9" :lazy="true" placeholder="Search users..." @update:model-value="searchUsers()" />
</div>
<div class="pt-6 pb-6">
<lf-search
v-model="search"
class="h-9"
:lazy="true"
:debounce="300"
placeholder="Search users..."
@update:model-value="searchUsers()"
/>
</div>

<div v-if="users.length > 0">
<lf-table>
<thead>
<tr>
<th>User</th>
<th>Email</th>
<th>Role</th>
</tr>
</thead>
<tbody>
<tr v-for="user of users" :key="user.id">
<td>
<div class="flex items-center gap-3">
<lf-avatar :name="nameDisplay(user)" :size="32" />
<p class="text-medium font-semibold">
{{ nameDisplay(user) }}
</p>
</div>
</td>
<td>
<p class="text-medium">
{{ user.email }}
</p>
</td>
<td>
<p class="text-medium">
{{ roleDisplay(user.roles) }}
</p>
</td>
</tr>
</tbody>
</lf-table>
<div v-if="users.length < total" class="pt-4">
<lf-button
type="primary-ghost"
loading-text="Loading users..."
:loading="loading"
@click="loadMore()"
>
Load more
</lf-button>
</div>
</div>

<div v-else-if="!loading">
<app-empty-state-cta
icon="user-group"
title="No users found"
/>
</div>
<div v-if="loading" class="pt-8 flex justify-center">
<lf-spinner />
</div>
</div>
</template>

<script setup lang="ts">
import LfSearch from '@/ui-kit/search/Search.vue';
import { onMounted, ref } from 'vue';
import { UsersService } from '@/modules/admin/services/users.service';
import { UserModel } from '@/modules/admin/models/User.model';
import LfTable from '@/ui-kit/table/Table.vue';
import LfAvatar from '@/ui-kit/avatar/Avatar.vue';
import LfSpinner from '@/ui-kit/spinner/Spinner.vue';
import LfButton from '@/ui-kit/button/Button.vue';

const search = ref('');
const loading = ref<boolean>(false);
const offset = ref(0);
const limit = ref(20);
const total = ref(0);

const users = ref<UserModel[]>([]);

const fetchUsers = () => {
if (loading.value) {
return;
}
loading.value = true;
UsersService.list({
filter: {
query: search.value,
},
offset: offset.value,
limit: limit.value,
})
.then((res) => {
if (offset.value > 0) {
users.value = [...users.value, ...res.rows];
} else {
users.value = res.rows;
}

if (res.rows.length < limit.value) {
total.value = users.value.length;
} else {
total.value = res.count;
}
})
.finally(() => {
loading.value = false;
});
};

const searchUsers = () => {
offset.value = 0;
fetchUsers();
};

const loadMore = () => {
offset.value = users.value.length;
fetchUsers();
};

const roleDisplay = (roles: string[]) => {
const role = roles?.[0];

if (role === 'admin') {
return 'Admin';
}
if (role === 'projectAdmin') {
return 'Project Admin';
}

if (role === 'readonly') {
return 'Read-only';
}
return role;
};
Comment on lines +120 to +134
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Improve role mapping maintainability.

Consider using a constant map for role display values and adding type safety.

+const ROLE_DISPLAY_MAP: Record<string, string> = {
+  admin: 'Admin',
+  projectAdmin: 'Project Admin',
+  readonly: 'Read-only',
+} as const;
+
-const roleDisplay = (roles: string[]) => {
+const roleDisplay = (roles: string[] | undefined): string => {
   const role = roles?.[0];
-
-  if (role === 'admin') {
-    return 'Admin';
-  }
-  if (role === 'projectAdmin') {
-    return 'Project Admin';
-  }
-
-  if (role === 'readonly') {
-    return 'Read-only';
-  }
-  return role;
+  return role ? (ROLE_DISPLAY_MAP[role] ?? role) : 'No role';
 };
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const roleDisplay = (roles: string[]) => {
const role = roles?.[0];
if (role === 'admin') {
return 'Admin';
}
if (role === 'projectAdmin') {
return 'Project Admin';
}
if (role === 'readonly') {
return 'Read-only';
}
return role;
};
const ROLE_DISPLAY_MAP: Record<string, string> = {
admin: 'Admin',
projectAdmin: 'Project Admin',
readonly: 'Read-only',
} as const;
const roleDisplay = (roles: string[] | undefined): string => {
const role = roles?.[0];
return role ? (ROLE_DISPLAY_MAP[role] ?? role) : 'No role';
};


const nameDisplay = (user: UserModel) => {
if ((user.fullName || '').length > 0) {
return user.fullName;
}
return user.email.split('@')[0];
};

onMounted(() => {
searchUsers();
});
</script>

<script lang="ts">
export default {
name: 'LfAdminUsers',
};
</script>
17 changes: 17 additions & 0 deletions frontend/src/modules/admin/services/users.service.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
import authAxios from '@/shared/axios/auth-axios';
import { AuthService } from '@/modules/auth/services/auth.service';

export class UsersService {
static async list(query: any) {
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add type safety for query parameters.

The any type for the query parameter reduces type safety. Consider creating a proper interface for the query parameters.

const tenantId = AuthService.getTenantId();

const response = await authAxios.get(
`/tenant/${tenantId}/user`,
{
params: query,
},
);

Comment on lines +8 to +14
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add proper error handling.

The HTTP request lacks error handling. Consider implementing try-catch blocks and proper error handling for different scenarios (network errors, 404, etc.).

return response.data;
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add type definition for the response data.

The response data is returned without type information. Consider defining an interface for the response structure to improve type safety and documentation.

}
}
Comment on lines +4 to +17
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Convert class to a standalone function module.

The static analysis tool correctly identifies that this class only contains static methods. Following TypeScript best practices, we should convert this to a module with standalone functions.

Here's the suggested implementation:

-export class UsersService {
-  static async list(query: any) {
-    const tenantId = AuthService.getTenantId();
-
-    const response = await authAxios.get(
-      `/tenant/${tenantId}/user`,
-      {
-        params: query,
-      },
-    );
-
-    return response.data;
-  }
-}
+interface UserQuery {
+  search?: string;
+  page?: number;
+  limit?: number;
+  // add other query parameters as needed
+}
+
+interface UserResponse {
+  data: User[];
+  total: number;
+  // add other response fields as needed
+}
+
+export async function listUsers(query: UserQuery): Promise<UserResponse> {
+  const tenantId = AuthService.getTenantId();
+
+  try {
+    const response = await authAxios.get<UserResponse>(
+      `/tenant/${tenantId}/user`,
+      {
+        params: query,
+      },
+    );
+    return response.data;
+  } catch (error) {
+    // Handle specific error cases
+    if (error.response?.status === 404) {
+      throw new Error('Users not found');
+    }
+    throw new Error('Failed to fetch users');
+  }
+}

Committable suggestion skipped: line range outside the PR's diff.

🧰 Tools
🪛 Biome

[error] 4-17: Avoid classes that contain only static members.

Prefer using simple functions instead of classes with only static members.

(lint/complexity/noStaticOnlyClass)

Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,9 @@
v-if="activeTab === 'audit-logs'"
/>
</el-tab-pane>
<el-tab-pane v-if="isAdminUser" label="Users" name="users">
<lf-admin-users v-if="activeTab === 'users'" />
</el-tab-pane>
<el-tab-pane v-if="isDevMode" label="Dev" name="dev">
<lf-devmode v-if="isDevMode && activeTab === 'dev'" />
</el-tab-pane>
Expand All @@ -53,6 +56,7 @@ import AppLfAuditLogsPage from '@/modules/lf/segments/pages/lf-audit-logs-page.v
import LfDevmode from '@/modules/lf/segments/components/dev/devmode.vue';
import { LfRole } from '@/shared/modules/permissions/types/Roles';
import AppOrganizationCommonPage from '@/modules/organization/pages/organization-common-page.vue';
import LfAdminUsers from '@/modules/admin/pages/users.page.vue';
const route = useRoute();
const router = useRouter();
Expand Down
46 changes: 46 additions & 0 deletions frontend/src/ui-kit/search/Search.vue
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
<template>
<lf-input v-model="valueProxy">
<template #prefix>
<lf-icon name="search" />
</template>
<template v-if="valueProxy.length" #suffix>
<div @click="valueProxy = ''">
<lf-icon name="xmark" />
</div>
</template>
</lf-input>
</template>

<script setup lang="ts">
import { ref, watch } from 'vue';
import { debounce } from 'lodash';
import LfInput from '@/ui-kit/input/Input.vue';
import LfIcon from '@/ui-kit/icon/Icon.vue';
const props = defineProps<{
modelValue: string | number,
lazy?: boolean,
}>();
const emit = defineEmits<{(e: 'update:modelValue', value: string | number): any }>();
const valueProxy = ref(props.modelValue);
const emitValue = (value: string | number) => emit('update:modelValue', value);
const debouncedEmitValue = debounce(emitValue, 300);
watch(valueProxy, (newVal) => {
if (props.lazy) {
debouncedEmitValue(newVal);
} else {
emitValue(newVal);
}
});
Comment on lines +31 to +39
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add cleanup for debounced function

The debounced function should be cleaned up when the component is unmounted to prevent memory leaks and unexpected behavior.

+import { ref, watch, onUnmounted } from 'vue';

 const debouncedEmitValue = debounce(emitValue, 300);

+onUnmounted(() => {
+  debouncedEmitValue.cancel();
+});
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const debouncedEmitValue = debounce(emitValue, 300);
watch(valueProxy, (newVal) => {
if (props.lazy) {
debouncedEmitValue(newVal);
} else {
emitValue(newVal);
}
});
import { ref, watch, onUnmounted } from 'vue';
const debouncedEmitValue = debounce(emitValue, 300);
onUnmounted(() => {
debouncedEmitValue.cancel();
});
watch(valueProxy, (newVal) => {
if (props.lazy) {
debouncedEmitValue(newVal);
} else {
emitValue(newVal);
}
});

</script>

<script lang="ts">
export default {
name: 'LfSearch',
};
</script>
Loading