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

Data quality assistant #2657

Merged
merged 39 commits into from
Oct 30, 2024
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
39 commits
Select commit Hold shift + click to select a range
cc291c5
Setup frontend structure
gaspergrom Oct 21, 2024
ff46371
Add member data quality checks
gaspergrom Oct 24, 2024
2d06e22
Add 3 basic checks for data issues
gaspergrom Oct 24, 2024
a167faa
Fix queries
gaspergrom Oct 24, 2024
c726926
Merge branch 'main' into improvement/data-quality-assistant
gaspergrom Oct 24, 2024
6059dba
Fix pagination loading
gaspergrom Oct 24, 2024
e35354e
Fix changes and fix lint
gaspergrom Oct 24, 2024
9f37f66
Fix tsc issues
gaspergrom Oct 24, 2024
4f1717c
Add too many emails and incomplete work experience
gaspergrom Oct 24, 2024
f140e72
Fix query for emails
gaspergrom Oct 24, 2024
c22a76d
Add project group selection
gaspergrom Oct 24, 2024
b1a3de0
Preserving project selection
gaspergrom Oct 24, 2024
800d3aa
Change queries to work cross segment
gaspergrom Oct 25, 2024
a58258f
Query optimizations
gaspergrom Oct 25, 2024
47f33c5
Merge branch 'main' into improvement/data-quality-assistant
gaspergrom Oct 25, 2024
dfaf4b1
Bug fixes and query testing
gaspergrom Oct 25, 2024
ff932dd
Fixed queries
gaspergrom Oct 25, 2024
dbfa7b1
Merge branch 'main' into improvement/data-quality-assistant
gaspergrom Oct 25, 2024
73827ef
Conflicting work history
gaspergrom Oct 28, 2024
14b3e83
Merge branch 'main' into improvement/data-quality-assistant
gaspergrom Oct 28, 2024
b355721
Debugging
gaspergrom Oct 28, 2024
d7ff1f4
Fix project segments for merge suggestions
gaspergrom Oct 29, 2024
1756682
Add activity count to merge suggestions
gaspergrom Oct 29, 2024
d0ca9bc
Merge branch 'main' of github.com:CrowdDotDev/crowd.dev
gaspergrom Oct 29, 2024
a9ee309
Merge branch 'main' into improvement/data-quality-assistant
gaspergrom Oct 29, 2024
be81cb1
Revert activity count fetching
gaspergrom Oct 29, 2024
02e5e9a
Fix org merge suggestion layout
gaspergrom Oct 29, 2024
8c54d2b
fix org merge suggestion display
gaspergrom Oct 29, 2024
3719ed2
Merge branch 'main' into improvement/data-quality-assistant
gaspergrom Oct 29, 2024
5a1ffeb
Fix display issues
gaspergrom Oct 29, 2024
06b2655
Fix queries
gaspergrom Oct 29, 2024
b8c7d4a
Merge branch 'main' into improvement/data-quality-assistant
gaspergrom Oct 29, 2024
ba00a7e
Fix queries
gaspergrom Oct 29, 2024
0037dde
Fix queries
gaspergrom Oct 29, 2024
e874c92
Fix queries
gaspergrom Oct 29, 2024
6ad0f9e
Fix queries
gaspergrom Oct 30, 2024
3699606
Fix queries
gaspergrom Oct 30, 2024
c8f71ba
Change limits and fix overlap query
gaspergrom Oct 30, 2024
8171976
Minor fixes
gaspergrom Oct 30, 2024
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
44 changes: 44 additions & 0 deletions backend/src/api/dataQuality/dataQualityMember.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
import { FeatureFlag } from '@crowd/types'

import DataQualityService from '@/services/dataQualityService'

import isFeatureEnabled from '../../feature-flags/isFeatureEnabled'
import Permissions from '../../security/permissions'
import PermissionChecker from '../../services/user/permissionChecker'

/**
* GET /tenant/{tenantId}/data-quality/member
* @summary Find a member data issues
* @tag Data Quality
* @security Bearer
* @description Find a data quality issues for members
* @pathParam {string} tenantId - Your workspace/tenant ID
* @response 200 - Ok
* @responseContent {DataQualityResponse} 200.application/json
* @response 401 - Unauthorized
* @response 404 - Not found
* @response 429 - Too many requests
*/
export default async (req, res) => {
new PermissionChecker(req).validateHas(Permissions.values.memberRead)

const segmentId = req.query.segments?.length > 0 ? req.query.segments[0] : null
if (!segmentId) {
const segmentsEnabled = await isFeatureEnabled(FeatureFlag.SEGMENTS, req)
if (segmentsEnabled) {
await req.responseHandler.error(req, res, {
code: 400,
message: 'Segment ID is required',
})
return
}
}
Comment on lines +25 to +35
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Improve segment validation logic.

The segment validation could be more robust:

  1. The array access req.query.segments[0] could throw if segments is undefined
  2. The error message could be more descriptive

Consider this improved implementation:

-  const segmentId = req.query.segments?.length > 0 ? req.query.segments[0] : null
+  const segments = Array.isArray(req.query.segments) ? req.query.segments : [];
+  const segmentId = segments.length > 0 ? segments[0] : null;
   if (!segmentId) {
     const segmentsEnabled = await isFeatureEnabled(FeatureFlag.SEGMENTS, req)
     if (segmentsEnabled) {
       await req.responseHandler.error(req, res, {
         code: 400,
-        message: 'Segment ID is required',
+        message: 'Segment ID is required when segments feature is enabled',
+        details: 'Please provide a valid segment ID in the query parameters',
       })
       return
     }
   }
📝 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 segmentId = req.query.segments?.length > 0 ? req.query.segments[0] : null
if (!segmentId) {
const segmentsEnabled = await isFeatureEnabled(FeatureFlag.SEGMENTS, req)
if (segmentsEnabled) {
await req.responseHandler.error(req, res, {
code: 400,
message: 'Segment ID is required',
})
return
}
}
const segments = Array.isArray(req.query.segments) ? req.query.segments : [];
const segmentId = segments.length > 0 ? segments[0] : null;
if (!segmentId) {
const segmentsEnabled = await isFeatureEnabled(FeatureFlag.SEGMENTS, req)
if (segmentsEnabled) {
await req.responseHandler.error(req, res, {
code: 400,
message: 'Segment ID is required when segments feature is enabled',
details: 'Please provide a valid segment ID in the query parameters',
})
return
}
}


const payload = await new DataQualityService(req).findMemberIssues(
req.params.tenantId,
req.query,
segmentId,
)

await req.responseHandler.success(req, res, payload)
Comment on lines +37 to +43
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 input validation for tenantId.

The endpoint should validate the tenantId parameter before passing it to the service.

Add validation before the service call:

+  if (!req.params.tenantId || typeof req.params.tenantId !== 'string') {
+    await req.responseHandler.error(req, res, {
+      code: 400,
+      message: 'Invalid tenant ID',
+    });
+    return;
+  }
+
   const payload = await new DataQualityService(req).findMemberIssues(
     req.params.tenantId,
     req.query,
     segmentId,
   )
📝 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 payload = await new DataQualityService(req).findMemberIssues(
req.params.tenantId,
req.query,
segmentId,
)
await req.responseHandler.success(req, res, payload)
if (!req.params.tenantId || typeof req.params.tenantId !== 'string') {
await req.responseHandler.error(req, res, {
code: 400,
message: 'Invalid tenant ID',
});
return;
}
const payload = await new DataQualityService(req).findMemberIssues(
req.params.tenantId,
req.query,
segmentId,
)
await req.responseHandler.success(req, res, payload)

⚠️ Potential issue

Add error handling for service calls.

The service call should be wrapped in a try-catch block to handle potential errors gracefully.

+  try {
     const payload = await new DataQualityService(req).findMemberIssues(
       req.params.tenantId,
       req.query,
       segmentId,
     )
     await req.responseHandler.success(req, res, payload)
+  } catch (error) {
+    await req.responseHandler.error(req, res, {
+      code: 500,
+      message: 'Failed to fetch member data quality issues',
+      error: error instanceof Error ? error.message : 'Unknown error',
+    });
+  }
📝 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 payload = await new DataQualityService(req).findMemberIssues(
req.params.tenantId,
req.query,
segmentId,
)
await req.responseHandler.success(req, res, payload)
try {
const payload = await new DataQualityService(req).findMemberIssues(
req.params.tenantId,
req.query,
segmentId,
)
await req.responseHandler.success(req, res, payload)
} catch (error) {
await req.responseHandler.error(req, res, {
code: 500,
message: 'Failed to fetch member data quality issues',
error: error instanceof Error ? error.message : 'Unknown error',
});
}

}
40 changes: 40 additions & 0 deletions backend/src/api/dataQuality/dataQualityOrganization.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
import { FeatureFlag } from '@crowd/types'

import DataQualityService from '@/services/dataQualityService'

import isFeatureEnabled from '../../feature-flags/isFeatureEnabled'
import Permissions from '../../security/permissions'
import PermissionChecker from '../../services/user/permissionChecker'

/**
* GET /tenant/{tenantId}/data-quality/organization
* @summary Find a organization data issues
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Correct grammatical errors in summary and description comments

The comments on lines 9 and 12 contain unnecessary articles. Removing 'a' improves readability.

Apply this diff to correct the comments:

- * @summary Find a organization data issues
+ * @summary Find organization data issues

...

- * @description Find a data quality issues for organizations
+ * @description Find data quality issues for organizations

Also applies to: 12-12

* @tag Data Quality
* @security Bearer
* @description Find a data quality issues for organizations
* @pathParam {string} tenantId - Your workspace/tenant ID
* @response 200 - Ok
* @responseContent {DataQualityResponse} 200.application/json
* @response 401 - Unauthorized
* @response 404 - Not found
* @response 429 - Too many requests
*/
export default async (req, res) => {
new PermissionChecker(req).validateHas(Permissions.values.organizationRead)

const segmentId = req.query.segments?.length > 0 ? req.query.segments[0] : null
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Ensure proper handling of 'req.query.segments' when it may be a string or an array

In Express.js, req.query.segments can be either a string (when a single segment is provided) or an array (when multiple segments are provided). Accessing length and indexing may not work as expected if it's a string, leading to potential errors.

Apply this diff to handle both cases correctly:

-const segmentId = req.query.segments?.length > 0 ? req.query.segments[0] : null
+const segments = Array.isArray(req.query.segments) ? req.query.segments : [req.query.segments].filter(Boolean)
+const segmentId = segments.length > 0 ? segments[0] : null
📝 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 segmentId = req.query.segments?.length > 0 ? req.query.segments[0] : null
const segments = Array.isArray(req.query.segments) ? req.query.segments : [req.query.segments].filter(Boolean)
const segmentId = segments.length > 0 ? segments[0] : null

if (!segmentId) {
const segmentsEnabled = await isFeatureEnabled(FeatureFlag.SEGMENTS, req)
if (segmentsEnabled) {
await req.responseHandler.error(req, res, {
code: 400,
message: 'Segment ID is required',
})
return
}
}

const payload = await new DataQualityService(req).findOrganizationIssues()

await req.responseHandler.success(req, res, payload)
}
9 changes: 9 additions & 0 deletions backend/src/api/dataQuality/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
import { safeWrap } from '../../middlewares/errorMiddleware'

Comment on lines +1 to +2
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider importing route handlers directly.

Instead of using require in the route definitions, consider importing the handlers at the top of the file for better code organization and to leverage TypeScript's static type checking.

import { safeWrap } from '../../middlewares/errorMiddleware'
+import memberHandler from './dataQualityMember'
+import organizationHandler from './dataQualityOrganization'

Committable suggestion was skipped due to low confidence.

export default (app) => {
app.get(`/tenant/:tenantId/data-quality/member`, safeWrap(require('./dataQualityMember').default))
app.get(
`/tenant/:tenantId/data-quality/organization`,
safeWrap(require('./dataQualityOrganization').default),
)
}
1 change: 1 addition & 0 deletions backend/src/api/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -243,6 +243,7 @@ setImmediate(async () => {
require('./customViews').default(routes)
require('./dashboard').default(routes)
require('./mergeAction').default(routes)
require('./dataQuality').default(routes)
// Loads the Tenant if the :tenantId param is passed
routes.param('tenantId', tenantMiddleware)
routes.param('tenantId', segmentMiddleware)
Expand Down
122 changes: 122 additions & 0 deletions backend/src/database/repositories/dataQualityRepository.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,122 @@
import {
fetchMembersWithIncompleteWorkExperience,
fetchMembersWithTooManyEmails,
fetchMembersWithTooManyIdentities,
fetchMembersWithTooManyIdentitiesPerPlatform,
fetchMembersWithoutWorkExperience,
} from '@crowd/data-access-layer/src/data-quality'

import SequelizeRepository from '@/database/repositories/sequelizeRepository'

import { IRepositoryOptions } from './IRepositoryOptions'
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider adding explicit return types for imported functions

The imported data quality functions from @crowd/data-access-layer would benefit from explicit TypeScript return type declarations to improve type safety and developer experience.

Consider creating an interface for the return types:

interface MemberDataQualityIssue {
  id: string;
  // add other relevant fields
}

// Then update the imports to include return types
import {
  fetchMembersWithoutWorkExperience: (
    qx: any,
    tenantId: string,
    limit: number,
    offset: number,
    segmentId: string
  ) => Promise<MemberDataQualityIssue[]>,
  // similar type annotations for other functions
} from '@crowd/data-access-layer/src/data-quality'


class DataQualityRepository {
/**
* Finds members with no work experience.
*
* @param {IRepositoryOptions} options - The repository options for executing the query.
* @param {string} tenantId - The ID of the tenant.
* @param {number} limit - The maximum number of results to return.
* @param {number} offset - The offset from which to begin returning results.
* @param {string} segmentId - The ID of the segment.
* @return {Promise<Members[]>} - A promise that resolves with an array of members having no work experience.
*/
static async findMembersWithNoWorkExperience(
options: IRepositoryOptions,
tenantId: string,
limit: number,
offset: number,
segmentId: string,
) {
const qx = SequelizeRepository.getQueryExecutor(options)
return fetchMembersWithoutWorkExperience(qx, tenantId, limit, offset, segmentId)
}
Comment on lines +26 to +35
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 input validation for method parameters

While the implementation is correct, it would benefit from parameter validation to ensure data integrity and provide clear error messages.

Consider adding validation:

static async findMembersWithNoWorkExperience(
  options: IRepositoryOptions,
  tenantId: string,
  limit: number,
  offset: number,
  segmentId: string,
) {
  if (!tenantId) throw new Error('tenantId is required')
  if (limit < 0) throw new Error('limit must be non-negative')
  if (offset < 0) throw new Error('offset must be non-negative')
  
  const qx = SequelizeRepository.getQueryExecutor(options)
  return fetchMembersWithoutWorkExperience(qx, tenantId, limit, offset, segmentId)
}


/**
* Finds and returns members with too many identities.
* Executes a query to fetch members who exceed a certain number of identities.
*
* @param {IRepositoryOptions} options - Repository options for querying the database.
* @param {string} tenantId - Identifier of the tenant whose members are being queried.
* @param {number} limit - The maximum number of members to retrieve.
* @param {number} offset - The number of members to skip before starting to collect the result set.
* @param {string} segmentId - Identifier of the segment to filter members.
* @return {Promise<Array>} A promise that resolves to an array of members with too many identities.
*/
static async findMembersWithTooManyIdentities(
options: IRepositoryOptions,
tenantId: string,
limit: number,
offset: number,
segmentId: string,
) {
const qx = SequelizeRepository.getQueryExecutor(options)
return fetchMembersWithTooManyIdentities(qx, 15, tenantId, limit, offset, segmentId)
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Extract magic numbers into named constants

The hardcoded threshold values should be extracted into named constants at the module level for better maintainability and documentation.

Add these constants at the top of the file:

const THRESHOLDS = {
  MAX_IDENTITIES: 15,
  MAX_IDENTITIES_PER_PLATFORM: 1,
  MAX_EMAILS: 3
} as const

// Then update the method calls:
// Line 56: fetchMembersWithTooManyIdentities(qx, THRESHOLDS.MAX_IDENTITIES, ...)
// Line 78: fetchMembersWithTooManyIdentitiesPerPlatform(qx, THRESHOLDS.MAX_IDENTITIES_PER_PLATFORM, ...)
// Line 99: fetchMembersWithTooManyEmails(qx, THRESHOLDS.MAX_EMAILS, ...)

Also applies to: 78-78, 99-99

}
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider externalizing magic numbers into constants

The hardcoded threshold value 15 in fetchMembersWithTooManyIdentities can be replaced with a named constant to improve readability and maintainability.

Apply this diff to declare a constant for the threshold:

+const MAX_IDENTITIES_THRESHOLD = 15;

...

 static async findMembersWithTooManyIdentities(
     options: IRepositoryOptions,
     tenantId: string,
     limit: number,
     offset: number,
     segmentId: string,
   ) {
     const qx = SequelizeRepository.getQueryExecutor(options)
-    return fetchMembersWithTooManyIdentities(qx, 15, tenantId, limit, offset, segmentId)
+    return fetchMembersWithTooManyIdentities(qx, MAX_IDENTITIES_THRESHOLD, tenantId, limit, offset, segmentId)
 }

Committable suggestion was skipped due to low confidence.


/**
* Finds members with too many identities per platform.
*
* @param {IRepositoryOptions} options - The repository options for database connection and other configurations.
* @param {string} tenantId - The ID of the tenant to filter members by.
* @param {number} limit - The maximum number of members to return.
* @param {number} offset - The number of members to skip before starting to collect the result set.
* @param {string} segmentId - The ID of the segment to filter members by.
*
* @return {Promise<Array>} A promise that resolves to an array of members with too many identities per platform.
*/
static async findMembersWithTooManyIdentitiesPerPlatform(
options: IRepositoryOptions,
tenantId: string,
limit: number,
offset: number,
segmentId: string,
) {
const qx = SequelizeRepository.getQueryExecutor(options)
return fetchMembersWithTooManyIdentitiesPerPlatform(qx, 1, tenantId, limit, offset, segmentId)
}
Comment on lines +70 to +79
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 error handling for database operations

The method should handle potential database errors gracefully and provide meaningful error messages.

Consider adding try-catch block:

static async findMembersWithTooManyIdentitiesPerPlatform(
  options: IRepositoryOptions,
  tenantId: string,
  limit: number,
  offset: number,
  segmentId: string,
) {
  try {
    const qx = SequelizeRepository.getQueryExecutor(options)
    return await fetchMembersWithTooManyIdentitiesPerPlatform(qx, 1, tenantId, limit, offset, segmentId)
  } catch (error) {
    throw new Error(`Failed to fetch members with too many identities per platform: ${error.message}`)
  }
}


/**
* Finds members who have too many emails within a given tenant and segment.
*
* @param {IRepositoryOptions} options - The repository options containing configuration and context for the query.
* @param {string} tenantId - The identifier for the tenant where members are queried.
* @param {number} limit - The maximum number of members to return.
* @param {number} offset - The starting point for pagination.
* @param {string} segmentId - The identifier for the segment to filter members.
* @return {Promise<Array>} - A promise that resolves to an array of members who have too many emails.
*/
static async findMembersWithTooManyEmails(
options: IRepositoryOptions,
tenantId: string,
limit: number,
offset: number,
segmentId: string,
) {
const qx = SequelizeRepository.getQueryExecutor(options)
return fetchMembersWithTooManyEmails(qx, 3, tenantId, limit, offset, segmentId)
}
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 pagination interface for better type safety

Consider creating a dedicated interface for pagination parameters to ensure consistency across repository methods.

Create and use a pagination interface:

interface PaginationParams {
  limit: number;
  offset: number;
}

static async findMembersWithTooManyEmails(
  options: IRepositoryOptions,
  tenantId: string,
  pagination: PaginationParams,
  segmentId: string,
) {
  const { limit, offset } = pagination
  const qx = SequelizeRepository.getQueryExecutor(options)
  return fetchMembersWithTooManyEmails(qx, 3, tenantId, limit, offset, segmentId)
}


/**
* Find members with incomplete work experience.
*
* @param {IRepositoryOptions} options - The repository options.
* @param {string} tenantId - The tenant ID to filter members.
* @param {number} limit - The maximum number of members to return.
* @param {number} offset - The number of members to skip before starting to collect the result set.
* @param {string} segmentId - The segment ID to filter members.
* @return {Promise<Array>} A promise that resolves to an array of members with incomplete work experience.
*/
static async findMembersWithIncompleteWorkExperience(
options: IRepositoryOptions,
tenantId: string,
limit: number,
offset: number,
segmentId: string,
) {
const qx = SequelizeRepository.getQueryExecutor(options)
return fetchMembersWithIncompleteWorkExperience(qx, tenantId, limit, offset, segmentId)
}
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 executor

The query executor type should be explicitly defined to ensure type safety when passing it to the DAL function.

Add type definition:

import { QueryTypes } from 'sequelize'

interface QueryExecutor {
  query<T = any>(
    sql: string,
    options?: { type: QueryTypes; replacements?: any }
  ): Promise<T>;
}

static async findMembersWithIncompleteWorkExperience(
  options: IRepositoryOptions,
  tenantId: string,
  limit: number,
  offset: number,
  segmentId: string,
) {
  const qx = SequelizeRepository.getQueryExecutor(options) as QueryExecutor
  return fetchMembersWithIncompleteWorkExperience(qx, tenantId, limit, offset, segmentId)
}

}

export default DataQualityRepository
50 changes: 50 additions & 0 deletions backend/src/services/dataQualityService.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
/* eslint-disable no-continue */
import { LoggerBase } from '@crowd/logging'

import DataQualityRepository from '@/database/repositories/dataQualityRepository'
import { IDataQualityParams, IDataQualityType } from '@/types/data-quality/data-quality-filters'

import { IServiceOptions } from './IServiceOptions'

export default class DataQualityService extends LoggerBase {
options: IServiceOptions

constructor(options: IServiceOptions) {
super(options.log)
this.options = options
}

/**
* Finds issues related to member data quality based on the specified type.
*
* @param {string} tenantId - The ID of the tenant for whom to find member issues.
* @param {IDataQualityParams} params - The parameters for finding member issues, including the type of issue, limit, and offset.
* @param {string} segmentId - The ID of the segment where the members belong.
* @return {Promise<Array>} A promise that resolves to an array of members with the specified data quality issues.
*/
async findMemberIssues(tenantId: string, params: IDataQualityParams, segmentId: string) {
const methodMap = {
[IDataQualityType.NO_WORK_EXPERIENCE]: DataQualityRepository.findMembersWithNoWorkExperience,
[IDataQualityType.TOO_MANY_IDENTITIES]:
DataQualityRepository.findMembersWithTooManyIdentities,
[IDataQualityType.TOO_MANY_IDENTITIES_PER_PLATFORM]:
DataQualityRepository.findMembersWithTooManyIdentitiesPerPlatform,
[IDataQualityType.TOO_MANY_EMAILS]: DataQualityRepository.findMembersWithTooManyEmails,
[IDataQualityType.INCOMPLETE_WORK_EXPERIENCE]:
DataQualityRepository.findMembersWithIncompleteWorkExperience,
}

const method = methodMap[params.type]

if (method) {
return method(this.options, tenantId, params.limit || 10, params.offset || 0, segmentId)
}
return []
}

// TODO: Implement this method when there are checks available
// eslint-disable-next-line class-methods-use-this
async findOrganizationIssues() {
return Promise.resolve([])
}
}
14 changes: 14 additions & 0 deletions backend/src/types/data-quality/data-quality-filters.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
export enum IDataQualityType {
TOO_MANY_IDENTITIES = 'too-many-identities',
TOO_MANY_IDENTITIES_PER_PLATFORM = 'too-many-identities-per-platform',
TOO_MANY_EMAILS = 'too-many-emails',
NO_WORK_EXPERIENCE = 'no-work-experience',
INCOMPLETE_WORK_EXPERIENCE = 'incomplete-work-experience',
CONFLICTING_WORK_EXPERIENCE = 'conflicting-work-experience',
}

export interface IDataQualityParams {
type: IDataQualityType
limit?: number
offset?: number
}
Comment on lines +11 to +15
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Enhance type safety and reusability of the interface.

Consider improving the interface with:

  1. Type narrowing for better compile-time safety
  2. Extracting pagination params into a reusable interface

Consider this alternative structure:

/** Common pagination parameters */
export interface PaginationParams {
  readonly limit?: number;
  readonly offset?: number;
}

/** Parameters for data quality queries */
export interface IDataQualityParams extends PaginationParams {
  readonly type: IDataQualityType;
}

/** Type guard for positive numbers */
export const isPositiveNumber = (n: number): boolean => 
  Number.isInteger(n) && n > 0;

10 changes: 10 additions & 0 deletions frontend/config/styles/components/badge.scss
Original file line number Diff line number Diff line change
Expand Up @@ -34,4 +34,14 @@
--lf-badge-tertiary-background: var(--lf-color-gray-100);
--lf-badge-tertiary-border: var(--lf-color-gray-200);
--lf-badge-tertiary-text: var(--lf-color-gray-900);

/* Danger */
--lf-badge-danger-background: var(--lf-color-red-100);
--lf-badge-danger-border: var(--lf-color-red-200);
--lf-badge-danger-text: var(--lf-color-red-800);

/* Warning */
--lf-badge-warning-background: var(--lf-color-yellow-100);
--lf-badge-warning-border: var(--lf-color-yellow-200);
--lf-badge-warning-text: var(--lf-color-yellow-800);
}
2 changes: 2 additions & 0 deletions frontend/src/config/permissions/admin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,8 @@ const admin: Record<LfPermission, boolean> = {
[LfPermission.mergeOrganizations]: true,
[LfPermission.customViewsCreate]: true,
[LfPermission.customViewsTenantManage]: true,
[LfPermission.dataQualityRead]: true,
[LfPermission.dataQualityEdit]: true,
};

export default admin;
2 changes: 2 additions & 0 deletions frontend/src/config/permissions/projectAdmin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,8 @@ const projectAdmin: Record<LfPermission, boolean> = {
[LfPermission.mergeOrganizations]: true,
[LfPermission.customViewsCreate]: true,
[LfPermission.customViewsTenantManage]: true,
[LfPermission.dataQualityRead]: true,
[LfPermission.dataQualityEdit]: true,
};

export default projectAdmin;
2 changes: 2 additions & 0 deletions frontend/src/config/permissions/readonly.ts
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,8 @@ const readonly: Record<LfPermission, boolean> = {
[LfPermission.mergeOrganizations]: false,
[LfPermission.customViewsCreate]: true,
[LfPermission.customViewsTenantManage]: false,
[LfPermission.dataQualityRead]: true,
[LfPermission.dataQualityEdit]: false,
};

export default readonly;
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
<template>
<div>
<div class="flex gap-3 mb-6">
<lf-data-quality-type-dropdown
v-model="tab"
:types="[
DataIssueType.TOO_MANY_IDENTITIES,
DataIssueType.TOO_MANY_IDENTITIES_PER_PLATFORM,
DataIssueType.TOO_MANY_EMAILS,
DataIssueType.NO_WORK_EXPERIENCE,
DataIssueType.INCOMPLETE_WORK_EXPERIENCE,
]"
/>
<lf-data-quality-project-dropdown v-model="projectGroup" />
</div>
<div class="border-b border-gray-200 w-full mb-1" />
<div>
<lf-data-quality-member-merge-suggestions
v-if="tab === 'merge-suggestions'"
:project-group="projectGroup"
/>
<lf-data-quality-member-issues
v-else
:type="tab"
:project-group="projectGroup"
/>
</div>
Comment on lines +17 to +27
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 loading and error states for better UX.

The component should handle loading and error states when switching between views.

-    <div>
+    <div v-if="isLoading">
+      <lf-loading />
+    </div>
+    <div v-else-if="error">
+      <lf-error-state :error="error" @retry="loadData" />
+    </div>
+    <div v-else>
       <lf-data-quality-member-merge-suggestions
         v-if="tab === 'merge-suggestions'"
         :project-group="projectGroup"
       />
       <lf-data-quality-member-issues
         v-else
         :type="tab"
         :project-group="projectGroup"
       />
     </div>

Add to script setup:

const isLoading = ref(false);
const error = ref<Error | null>(null);

async function loadData() {
  try {
    isLoading.value = true;
    // Add data fetching logic here
  } catch (e) {
    error.value = e instanceof Error ? e : new Error('Unknown error');
  } finally {
    isLoading.value = false;
  }
}

</div>
</template>

<script lang="ts" setup>
import { ref } from 'vue';
import LfDataQualityMemberMergeSuggestions
from '@/modules/data-quality/components/member/data-quality-member-merge-suggestions.vue';
import LfDataQualityMemberIssues from '@/modules/data-quality/components/member/data-quality-member-issues.vue';
import LfDataQualityTypeDropdown from '@/modules/data-quality/components/shared/data-quality-type-dropdown.vue';
import LfDataQualityProjectDropdown from '@/modules/data-quality/components/shared/data-quality-project-dropdown.vue';
import { useRoute } from 'vue-router';
import { DataIssueType } from '@/modules/data-quality/types/DataIssueType';

const route = useRoute();

const tab = ref('merge-suggestions');
const projectGroup = ref(route.query.projectGroup as string);
Comment on lines +43 to +44
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Improve type safety and state management.

The current implementation lacks proper typing and reactive state management.

-const tab = ref('merge-suggestions');
-const projectGroup = ref(route.query.projectGroup as string);
+type TabValue = 'merge-suggestions' | DataIssueType;
+
+const tab = ref<TabValue>('merge-suggestions');
+const projectGroup = ref<string | undefined>(
+  typeof route.query.projectGroup === 'string' ? route.query.projectGroup : undefined
+);
+
+watch(tab, () => {
+  loadData();
+});
+
+watch(projectGroup, () => {
+  if (projectGroup.value) {
+    loadData();
+  }
+});
+
+onMounted(() => {
+  loadData();
+});

Committable suggestion was skipped due to low confidence.

</script>

<script lang="ts">
export default {
name: 'LfDataQualityMember',
};
</script>
Comment on lines +47 to +51
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Remove redundant script block.

When using <script setup>, the component name can be defined using a <script> block with defineOptions.

Replace the separate script block with:

-<script lang="ts">
-export default {
-  name: 'LfDataQualityMember',
-};
-</script>
+<script setup lang="ts">
+defineOptions({
+  name: 'LfDataQualityMember',
+});
// ... rest of your setup code
📝 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
<script lang="ts">
export default {
name: 'LfDataQualityMember',
};
</script>
<script setup lang="ts">
defineOptions({
name: 'LfDataQualityMember',
});
// ... rest of your setup code

Loading
Loading