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

Feature/implement ocr button #2731

Merged
merged 22 commits into from
Oct 1, 2024
Merged
Show file tree
Hide file tree
Changes from 16 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
2 changes: 1 addition & 1 deletion apps/backoffice-v2/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@
"i18next-http-backend": "^2.1.1",
"leaflet": "^1.9.4",
"libphonenumber-js": "^1.10.49",
"lucide-react": "^0.239.0",
"lucide-react": "^0.445.0",
"match-sorter": "^6.3.1",
"msw": "^1.0.0",
"posthog-js": "^1.154.2",
Expand Down
4 changes: 4 additions & 0 deletions apps/backoffice-v2/public/locales/en/toast.json
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,10 @@
"pdf_certificate": {
"error": "Failed to open PDF certificate."
},
"document_ocr": {
"success": "OCR performed successfully.",
"error": "Failed to perform OCR on the document."
},
"business_report_creation": {
"success": "Merchant check created successfully.",
"error": "Error occurred while creating a merchant check.",
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
import { ctw } from '@/common/utils/ctw/ctw';
import { ComponentProps, FunctionComponent } from 'react';
import { Loader2, ScanTextIcon } from 'lucide-react';

export interface IImageOCR extends ComponentProps<'button'> {
onOcrPressed?: () => void;
isOcrDisabled: boolean;
isLoadingOCR?: boolean;
}

export const ImageOCR: FunctionComponent<IImageOCR> = ({
isOcrDisabled,
onOcrPressed,
className,
isLoadingOCR,
...props
}) => (
<button
{...props}
type="button"
className={ctw(
'disabled: btn btn-circle btn-ghost btn-sm bg-base-300/70 text-[0.688rem] focus:outline-primary disabled:bg-base-300/70',
isLoadingOCR,
className,
)}
onClick={() => onOcrPressed?.()}
disabled={isOcrDisabled || isLoadingOCR}
>
{isLoadingOCR ? (
<Loader2 className="animate-spin stroke-foreground" />
) : (
<ScanTextIcon className={'p-0.5'} />
)}
</button>
);
1 change: 1 addition & 0 deletions apps/backoffice-v2/src/domains/customer/fetchers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ const CustomerSchema = z.object({
createBusinessReportBatch: z
.object({ enabled: z.boolean().default(false), options: createBusinessReportOptions })
.optional(),
isDocumentOcrEnabled: z.boolean().default(false).optional(),
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

Inconsistent Default Value for isDocumentOcrEnabled

The property isDocumentOcrEnabled is defined with a default value of false in fetchers.ts. However, in useDocumentsLogic.tsx, it is used with a fallback to true:

shouldOCR: customer?.features?.isDocumentOcrEnabled || true, // TODO remove default true after review

This inconsistency can lead to shouldOCR being true even when it should default to false, potentially causing unintended behavior.

Recommendations:

  • Remove the fallback to true in useDocumentsLogic.tsx to align with the schema default.
  • Address the TODO comment to ensure consistency across the codebase.
🔗 Analysis chain

LGTM: New OCR feature flag added correctly

The new isDocumentOcrEnabled property has been added appropriately to the features object within the CustomerSchema. It's correctly defined as an optional boolean with a default value of false, which aligns with best practices for feature flags.

A few points to consider:

  1. The naming is clear and follows the existing naming conventions in the schema.
  2. The default value of false ensures backwards compatibility with existing data.
  3. The property is optional, which is appropriate for a feature flag.

To ensure this change doesn't introduce any unexpected behavior, let's verify its usage:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for any existing usage of isDocumentOcrEnabled
# Expected: No results if this is a new feature

rg --type typescript 'isDocumentOcrEnabled'

Length of output: 78


Script:

#!/bin/bash
# Description: Check for any existing usage of isDocumentOcrEnabled in TypeScript files
# Expected: No results if this is a new feature

rg 'isDocumentOcrEnabled' -g '*.ts' -g '*.tsx'

Length of output: 364


Script:

#!/bin/bash
# Description: Extract surrounding lines for the usage of isDocumentOcrEnabled in useDocumentsLogic.tsx
# Expected: Contextual code to assess the implementation and TODO comment

rg 'isDocumentOcrEnabled' -A 5 -B 5 apps/backoffice-v2/src/pages/Entity/components/Case/hooks/useDocuments/useDocumentsLogic.tsx

Length of output: 394

})
.nullable(),
config: z
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
import { useMutation, useQueryClient } from '@tanstack/react-query';
import { fetchWorkflowDocumentOCRResult } from '@/domains/workflows/fetchers';
import { toast } from 'sonner';
import { t } from 'i18next';
import { workflowsQueryKeys } from '@/domains/workflows/query-keys';
import { useFilterId } from '@/common/hooks/useFilterId/useFilterId';

export const useDocumentOcr = ({ workflowId }: { workflowId: string }) => {
const filterId = useFilterId();
const workflowById = workflowsQueryKeys.byId({ workflowId, filterId });
const queryClient = useQueryClient();

return useMutation({
mutationFn: ({ documentId }: { documentId: string }) => {
return fetchWorkflowDocumentOCRResult({
workflowRuntimeId: workflowId,
documentId,
});
},
onSuccess: (data, variables) => {
void queryClient.invalidateQueries(workflowsQueryKeys._def);
toast.success(t('toast:document_ocr.success'));
},
onError: (error, variables) => {
console.error('OCR error:', error, 'for document:', variables.documentId);
void queryClient.invalidateQueries(workflowsQueryKeys._def);
toast.error(t('toast:document_ocr.error'));
},
});
};
18 changes: 18 additions & 0 deletions apps/backoffice-v2/src/domains/workflows/fetchers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -298,3 +298,21 @@ export const createWorkflowRequest = async ({

return handleZodError(error, workflow);
};

export const fetchWorkflowDocumentOCRResult = async ({
workflowRuntimeId,
documentId,
}: {
workflowRuntimeId: string;
documentId: string;
}) => {
const [workflow, error] = await apiClient({
method: Method.GET,
url: `${getOriginUrl(
env.VITE_API_URL,
)}/api/v1/internal/workflows/${workflowRuntimeId}/documents/${documentId}/run-ocr`,
schema: z.any(),
});

return handleZodError(error, workflow);
};
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import { CommonWorkflowEvent } from '@ballerine/common';
import { ComponentProps, FunctionComponent, useCallback, useEffect, useState } from 'react';
import { toast } from 'sonner';
import { useApproveTaskByIdMutation } from '../../../../../../domains/entities/hooks/mutations/useApproveTaskByIdMutation/useApproveTaskByIdMutation';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ export const Details: FunctionComponent<ExtractCellProps<'details'>> = ({
workflowId,
documents = [],
onSubmit,
isSaveDisabled,
props,
}) => {
if (!value.data?.length) {
Expand All @@ -38,6 +39,7 @@ export const Details: FunctionComponent<ExtractCellProps<'details'>> = ({
documents={documents}
title={value?.title}
data={sortedData}
isSaveDisabled={isSaveDisabled}
contextUpdateMethod={contextUpdateMethod}
onSubmit={onSubmit}
/>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,7 @@ export const EditableDetails: FunctionComponent<IEditableDetails> = ({
documents,
title,
workflowId,
isSaveDisabled,
contextUpdateMethod = 'base',
onSubmit: onSubmitCallback,
}) => {
Expand Down Expand Up @@ -427,7 +428,11 @@ export const EditableDetails: FunctionComponent<IEditableDetails> = ({
</div>
<div className={`flex justify-end`}>
{data?.some(({ isEditable }) => isEditable) && (
<Button type="submit" className={`ms-auto mt-3`}>
<Button
type="submit"
className={`ms-auto mt-3 aria-disabled:pointer-events-none aria-disabled:opacity-50`}
aria-disabled={isSaveDisabled}
>
Save
</Button>
)}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ export interface IEditableDetails {
documents: IEditableDetailsDocument[];
title: string;
workflowId: string;
isSaveDisabled?: boolean;
contextUpdateMethod?: 'base' | 'director';
onSubmit?: (document: AnyObject) => void;
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,13 @@ export const MultiDocuments: FunctionComponent<IMultiDocumentsProps> = ({ value

return (
<div className={`m-2 rounded p-1`}>
<Case.Documents documents={documents} isLoading={value?.isLoading} />
<Case.Documents
documents={documents}
isDocumentEditable={value?.isDocumentEditable}
isLoading={value?.isLoading}
onOcrPressed={value?.onOcrPressed}
isLoadingOCR={value?.isLoadingOCR}
/>
</div>
);
};
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
export interface IMultiDocumentsProps {
value: {
isLoading: boolean;
onOcrPressed: () => void;
isLoadingOCR: boolean;
isDocumentEditable: boolean;
data: Array<{
imageUrl: string;
title: string;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,7 @@ export type TDetailsCell = {
hideSeparator?: boolean;
documents?: IEditableDetailsDocument[];
contextUpdateMethod?: 'base' | 'director';
isSaveDisabled?: boolean;
value: {
id: string;
title: string;
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { MotionButton } from '@/common/components/molecules/MotionButton/MotionButton';
import { checkIsBusiness } from '@/common/utils/check-is-business/check-is-business';
import { ctw } from '@/common/utils/ctw/ctw';
import { CommonWorkflowStates, StateTag, valueOrNA } from '@ballerine/common';
import { CommonWorkflowStates, isObject, StateTag, valueOrNA } from '@ballerine/common';
import { useApproveTaskByIdMutation } from '@/domains/entities/hooks/mutations/useApproveTaskByIdMutation/useApproveTaskByIdMutation';
import { useRejectTaskByIdMutation } from '@/domains/entities/hooks/mutations/useRejectTaskByIdMutation/useRejectTaskByIdMutation';
import { useRemoveDecisionTaskByIdMutation } from '@/domains/entities/hooks/mutations/useRemoveDecisionTaskByIdMutation/useRemoveDecisionTaskByIdMutation';
Expand Down Expand Up @@ -29,6 +29,7 @@ import { X } from 'lucide-react';
import * as React from 'react';
import { FunctionComponent, useCallback, useMemo } from 'react';
import { toTitleCase } from 'string-ts';
import { useDocumentOcr } from '@/domains/entities/hooks/mutations/useDocumentOcr/useDocumentOcr';

export const useDocumentBlocks = ({
workflow,
Expand Down Expand Up @@ -79,6 +80,14 @@ export const useDocumentBlocks = ({

const { mutate: mutateApproveTaskById, isLoading: isLoadingApproveTaskById } =
useApproveTaskByIdMutation(workflow?.id);
const {
mutate: mutateOCRDocument,
isLoading: isLoadingOCRDocument,
data: ocrResult,
} = useDocumentOcr({
workflowId: workflow?.id,
});

Comment on lines +83 to +90
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Inconsistent Variable Naming: Use CamelCase for mutateOCRDocument and isLoadingOCRDocument

The variables mutateOCRDocument and isLoadingOCRDocument use an uppercase OCR in the middle of their names, which is inconsistent with camelCase conventions used elsewhere in the codebase (e.g., useDocumentOcr). For consistency and readability, consider renaming these variables to mutateOcrDocument and isLoadingOcrDocument.

Apply this diff to rename the variables:

 const {
-  mutate: mutateOCRDocument,
-  isLoading: isLoadingOCRDocument,
+  mutate: mutateOcrDocument,
+  isLoading: isLoadingOcrDocument,
   data: ocrResult,
 } = useDocumentOcr({
   workflowId: workflow?.id,
 });
📝 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 {
mutate: mutateOCRDocument,
isLoading: isLoadingOCRDocument,
data: ocrResult,
} = useDocumentOcr({
workflowId: workflow?.id,
});
const {
mutate: mutateOcrDocument,
isLoading: isLoadingOcrDocument,
data: ocrResult,
} = useDocumentOcr({
workflowId: workflow?.id,
});

const { isLoading: isLoadingRejectTaskById } = useRejectTaskByIdMutation(workflow?.id);

const { comment, onClearComment, onCommentChange } = useCommentInputLogic();
Expand Down Expand Up @@ -358,6 +367,25 @@ export const useDocumentBlocks = ({
})
.cellAt(0, 0);

const documentEntries = Object.entries(
{
...additionalProperties,
...propertiesSchema?.properties,
} ?? {},
).map(([title, formattedValue]) => {
if (isObject(formattedValue)) {
return [
title,
{
...formattedValue,
value: formattedValue.value || ocrResult?.parsedData?.[title],
},
];
}

return [title, formattedValue];
});

const detailsCell = createBlocksTyped()
.addBlock()
.addCell({
Expand All @@ -370,12 +398,7 @@ export const useDocumentBlocks = ({
value: {
id,
title: `${category} - ${docType}`,
data: Object.entries(
{
...additionalProperties,
...propertiesSchema?.properties,
} ?? {},
)?.map(
data: documentEntries?.map(
([
title,
{
Expand Down Expand Up @@ -441,6 +464,7 @@ export const useDocumentBlocks = ({
},
},
workflowId: workflow?.id,
isSaveDisabled: isLoadingOCRDocument,
documents: workflow?.context?.documents,
})
.addCell(decisionCell)
Expand All @@ -455,6 +479,9 @@ export const useDocumentBlocks = ({
type: 'multiDocuments',
value: {
isLoading: storageFilesQueryResult?.some(({ isLoading }) => isLoading),
onOcrPressed: () => mutateOCRDocument({ documentId: id }),
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Inconsistent Function Name: Rename mutateOCRDocument to mutateOcrDocument

The function mutateOCRDocument is being called here. To maintain consistency with the corrected variable names and camelCase conventions, consider renaming it to mutateOcrDocument.

Apply this diff to update the function name:

 onOcrPressed: () => mutateOCRDocument({ documentId: id }),
+onOcrPressed: () => mutateOcrDocument({ documentId: id }),

Committable suggestion was skipped due to low confidence.

isDocumentEditable: caseState.writeEnabled,
isLoadingOCR: isLoadingOCRDocument,
Comment on lines +482 to +484
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Inconsistent Variable Naming: Align mutateOCRDocument with CamelCase Convention

The function mutateOCRDocument is called here, but its naming is inconsistent with camelCase conventions and other variables like onOcrPressed. Similarly, isLoadingOCRDocument should be isLoadingOcrDocument. Renaming these variables enhances readability and maintains consistency across the codebase.

Apply this diff to update the variable names:

 onOcrPressed: () => mutateOCRDocument({ documentId: id }),
+onOcrPressed: () => mutateOcrDocument({ documentId: id }),
 isDocumentEditable: caseState.writeEnabled,
-isLoadingOCR: isLoadingOCRDocument,
+isLoadingOCR: isLoadingOcrDocument,
📝 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
onOcrPressed: () => mutateOCRDocument({ documentId: id }),
isDocumentEditable: caseState.writeEnabled,
isLoadingOCR: isLoadingOCRDocument,
onOcrPressed: () => mutateOcrDocument({ documentId: id }),
isDocumentEditable: caseState.writeEnabled,
isLoadingOCR: isLoadingOcrDocument,

⚠️ Potential issue

Add Error Handling for OCR Mutation

Currently, onOcrPressed triggers the OCR mutation without handling potential errors. To improve user experience and robustness, consider adding error handling to provide feedback in case the mutation fails.

Example implementation:

 onOcrPressed: () => {
-  mutateOCRDocument({ documentId: id }),
+  mutateOcrDocument(
+    { documentId: id },
+    {
+      onError: (error) => {
+        // Handle error, e.g., display notification
+        console.error('OCR mutation failed:', error);
+      },
+    },
+  );
 },

This addition ensures that errors during the mutation are caught and can be appropriately addressed.

Committable suggestion was skipped due to low confidence.

data:
documents?.[docIndex]?.pages?.map(
({ type, fileName, metadata, ballerineFileId }, pageIndex) => ({
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { ImageViewer } from '@/common/components/organisms/ImageViewer/ImageView
import { ctw } from '@/common/utils/ctw/ctw';
import { isPdf } from '@/common/utils/is-pdf/is-pdf';
import { useDocumentsToolbarLogic } from '@/pages/Entity/components/Case/hooks/useDocumentsToolbarLogic/useDocumentsToolbarLogic';
import { ImageOCR } from '@/common/components/molecules/ImageOCR/ImageOCR';

export const DocumentsToolbar: FunctionComponent<{
image: { id: string; imageUrl: string; fileType: string; fileName: string };
Expand All @@ -13,14 +14,20 @@ export const DocumentsToolbar: FunctionComponent<{
onRotateDocument: () => void;
onOpenDocumentInNewTab: (id: string) => void;
shouldDownload: boolean;
onOcrPressed?: () => void;
isOCREnabled: boolean;
isLoadingOCR: boolean;
Comment on lines +17 to +19
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Consider making onOcrPressed a required prop

The onOcrPressed prop is currently optional (?:), but it's being passed directly to the ImageOCR component without a default value or null check. If onOcrPressed is essential for the ImageOCR functionality, consider making it a required prop to prevent potential undefined errors.

fileToDownloadBase64: string;
}> = ({
image,
isLoading,
hideOpenExternalButton,
onRotateDocument,
onOpenDocumentInNewTab,
onOcrPressed,
shouldDownload,
isLoadingOCR,
isOCREnabled,
fileToDownloadBase64,
}) => {
const { onOpenInNewTabClick } = useDocumentsToolbarLogic({
Expand All @@ -31,6 +38,11 @@ export const DocumentsToolbar: FunctionComponent<{

return (
<div className={`absolute z-50 flex space-x-2 bottom-right-6`}>
<ImageOCR
isOcrDisabled={!isOCREnabled}
onOcrPressed={onOcrPressed}
isLoadingOCR={isLoadingOCR}
/>
Comment on lines +41 to +45
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Rename isOcrDisabled to isOCREnabled for clarity

The prop isOcrDisabled receives the negation of isOCREnabled. Renaming isOcrDisabled to isOCREnabled in the ImageOCR component can improve readability by avoiding double negatives and aligning naming conventions.

Apply this diff to update the prop name in the ImageOCR component:

-const ImageOCR = ({ isOcrDisabled, onOcrPressed, isLoadingOCR }) => { ... }
+const ImageOCR = ({ isOCREnabled, onOcrPressed, isLoadingOCR }) => { ... }

...

-<button disabled={isOcrDisabled} ... >
+<button disabled={!isOCREnabled} ... >

Then update the usage in Case.Documents.Toolbar.tsx:

-<ImageOCR
-  isOcrDisabled={!isOCREnabled}
+<ImageOCR
+  isOCREnabled={isOCREnabled}
   onOcrPressed={onOcrPressed}
   isLoadingOCR={isLoadingOCR}
/>

Committable suggestion was skipped due to low confidence.

{!hideOpenExternalButton && !isLoading && image?.id && (
<button
type={`button`}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import { ImageViewer } from '@/common/components/organisms/ImageViewer/ImageView
import { ctw } from '@/common/utils/ctw/ctw';
import { keyFactory } from '@/common/utils/key-factory/key-factory';
import { DocumentsToolbar } from '@/pages/Entity/components/Case/Case.Documents.Toolbar';
import { useDocuments } from './hooks/useDocuments/useDocuments';
import { useDocumentsLogic } from './hooks/useDocuments/useDocumentsLogic';
import { IDocumentsProps } from './interfaces';

/**
Expand All @@ -24,19 +24,20 @@ import { IDocumentsProps } from './interfaces';
*/
export const Documents: FunctionComponent<IDocumentsProps> = ({
documents,
onOcrPressed,
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Ensure 'onOcrPressed' is defined in 'IDocumentsProps'

You've added onOcrPressed to the component props. Please ensure that onOcrPressed is defined in the IDocumentsProps interface to maintain type safety and prevent potential TypeScript errors.

isLoading,
isDocumentEditable,
isLoadingOCR,
hideOpenExternalButton,
}) => {
const {
crop,
onCrop,
onCancelCrop,
isCropping,
onOcr,
selectedImageRef,
initialImage,
skeletons,
isLoadingOCR,
selectedImage,
onSelectImage,
documentRotation,
Expand All @@ -45,8 +46,9 @@ export const Documents: FunctionComponent<IDocumentsProps> = ({
onTransformed,
isRotatedOrTransformed,
shouldDownload,
isOCREnabled,
fileToDownloadBase64,
} = useDocuments(documents);
} = useDocumentsLogic(documents);

return (
<ImageViewer selectedImage={selectedImage} onSelectImage={onSelectImage}>
Expand Down Expand Up @@ -88,8 +90,10 @@ export const Documents: FunctionComponent<IDocumentsProps> = ({
onOpenDocumentInNewTab={onOpenDocumentInNewTab}
// isRotatedOrTransformed={isRotatedOrTransformed}
shouldDownload={shouldDownload}
isOCREnabled={!!isDocumentEditable && isOCREnabled}
onOcrPressed={onOcrPressed}
isLoadingOCR={!!isLoadingOCR}
// isCropping={isCropping}
// isLoadingOCR={isLoadingOCR}
// onCancelCrop={onCancelCrop}
fileToDownloadBase64={fileToDownloadBase64}
/>
Expand Down
Loading
Loading