-
Notifications
You must be signed in to change notification settings - Fork 204
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
Changes from 7 commits
42bc8eb
29fa3e4
456522c
6408f95
d205e97
783bf7e
67b6ce2
9df1bfc
4c4f013
a5a8a1a
96cae03
ca3d26e
8096d8f
3d651de
2ed1587
53f7d3d
8244298
299cbe6
571b9c6
f8a4508
9e89e6a
41c6d15
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,30 @@ | ||
import { ctw } from '@/common/utils/ctw/ctw'; | ||
import { ComponentProps, FunctionComponent } from 'react'; | ||
import { ScanTextIcon } from 'lucide-react'; | ||
|
||
export interface IImageOCR extends ComponentProps<'div'> { | ||
onOcrPressed?: () => void; | ||
isOcrDisabled: boolean; | ||
documentId: string; | ||
} | ||
|
||
export const ImageOCR: FunctionComponent<IImageOCR> = ({ | ||
isOcrDisabled, | ||
onOcrPressed, | ||
documentId, | ||
className, | ||
...props | ||
}) => ( | ||
<> | ||
<button | ||
type={`button`} | ||
className={ctw( | ||
`btn btn-circle btn-ghost btn-sm bg-base-300/70 text-[0.688rem] focus:outline-primary`, | ||
)} | ||
onClick={() => onOcrPressed?.()} | ||
disabled={isOcrDisabled} | ||
> | ||
<ScanTextIcon /> | ||
</button> | ||
</> | ||
); |
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({ | ||
workflowDefinitionId: workflowId, | ||
documentId, | ||
}); | ||
}, | ||
onSuccess: (data, variables) => { | ||
void queryClient.invalidateQueries(workflowsQueryKeys._def); | ||
toast.success(t('toast:document_ocr.success')); | ||
}, | ||
onError: (_error, _variables) => { | ||
console.error(_error); | ||
void queryClient.invalidateQueries(workflowsQueryKeys._def); | ||
toast.error(t('toast:document_ocr.error')); | ||
}, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Refine error handling approach While the current error handling provides user feedback and logs the error, there are a couple of points to consider:
Consider the following improvements: - onError: (_error, _variables) => {
- console.error(_error);
- void queryClient.invalidateQueries(workflowsQueryKeys._def);
- toast.error(t('toast:document_ocr.error'));
+ onError: (error, variables) => {
+ console.error('OCR error:', error, 'for document:', variables.documentId);
+ toast.error(t('toast:document_ocr.error', { documentId: variables.documentId }));
}, This change:
Also, consider adding more specific error handling if the
|
||
}); | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -298,3 +298,21 @@ export const createWorkflowRequest = async ({ | |
|
||
return handleZodError(error, workflow); | ||
}; | ||
|
||
export const fetchWorkflowDocumentOCRResult = async ({ | ||
workflowDefinitionId, | ||
documentId, | ||
}: { | ||
workflowDefinitionId: string; | ||
documentId: string; | ||
}) => { | ||
const [workflow, error] = await apiClient({ | ||
method: Method.PATCH, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. PATCH? |
||
url: `${getOriginUrl( | ||
env.VITE_API_URL, | ||
)}/api/v1/internal/workflows/${workflowDefinitionId}/documents/${documentId}/run-ocr`, | ||
schema: z.any(), | ||
}); | ||
|
||
return handleZodError(error, workflow); | ||
}; |
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'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -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, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Inconsistent Variable Naming: Use CamelCase for The variables Apply this diff to rename the variables: const {
- mutate: mutateOCRDocument,
- isLoading: isLoadingOCRDocument,
+ mutate: mutateOcrDocument,
+ isLoading: isLoadingOcrDocument,
data: ocrResult,
} = useDocumentOcr({
workflowId: workflow?.id,
}); 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
const { isLoading: isLoadingRejectTaskById } = useRejectTaskByIdMutation(workflow?.id); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
const { comment, onClearComment, onCommentChange } = useCommentInputLogic(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -358,6 +367,19 @@ export const useDocumentBlocks = ({ | |||||||||||||||||||||||||||||||||||||||||||||||||||||||
}) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
.cellAt(0, 0); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
const documentEntries = Object.entries( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
...additionalProperties, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
...propertiesSchema?.properties, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} ?? {}, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
).map(([title, formattedValue]) => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
if (isObject(formattedValue)) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
formattedValue.value ||= ocrResult?.parsedData?.[title]; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In React its more important not to make mutations. Just pass the value to the right instead of formattedValue. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return [title, formattedValue]; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
}); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Avoid Mutating The current implementation directly mutates Apply this diff to prevent direct mutation: ).map(([title, formattedValue]) => {
if (isObject(formattedValue)) {
- formattedValue.value ||= ocrResult?.parsedData?.[title];
+ formattedValue = {
+ ...formattedValue,
+ value: formattedValue.value || ocrResult?.parsedData?.[title],
+ };
}
return [title, formattedValue];
}); This change ensures that you are not modifying the original 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
const detailsCell = createBlocksTyped() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
.addBlock() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
.addCell({ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -370,12 +392,7 @@ export const useDocumentBlocks = ({ | |||||||||||||||||||||||||||||||||||||||||||||||||||||||
value: { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
id, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
title: `${category} - ${docType}`, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
data: Object.entries( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
...additionalProperties, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
...propertiesSchema?.properties, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} ?? {}, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
)?.map( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
data: documentEntries?.map( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
([ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
title, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -450,6 +467,7 @@ export const useDocumentBlocks = ({ | |||||||||||||||||||||||||||||||||||||||||||||||||||||||
type: 'multiDocuments', | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
value: { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
isLoading: storageFilesQueryResult?.some(({ isLoading }) => isLoading), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
onOcrPressed: () => mutateOCRDocument({ documentId: id }), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Inconsistent Function Name: Rename The function Apply this diff to update the function name: onOcrPressed: () => mutateOCRDocument({ documentId: id }),
+onOcrPressed: () => mutateOcrDocument({ documentId: id }),
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
data: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
documents?.[docIndex]?.pages?.map( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
({ type, fileName, metadata, ballerineFileId }, pageIndex) => ({ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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'; | ||
|
||
/** | ||
|
@@ -24,6 +24,7 @@ import { IDocumentsProps } from './interfaces'; | |
*/ | ||
export const Documents: FunctionComponent<IDocumentsProps> = ({ | ||
documents, | ||
onOcrPressed, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ensure 'onOcrPressed' is defined in 'IDocumentsProps' You've added |
||
isLoading, | ||
hideOpenExternalButton, | ||
}) => { | ||
|
@@ -32,7 +33,6 @@ export const Documents: FunctionComponent<IDocumentsProps> = ({ | |
onCrop, | ||
onCancelCrop, | ||
isCropping, | ||
onOcr, | ||
selectedImageRef, | ||
initialImage, | ||
skeletons, | ||
|
@@ -45,8 +45,9 @@ export const Documents: FunctionComponent<IDocumentsProps> = ({ | |
onTransformed, | ||
isRotatedOrTransformed, | ||
shouldDownload, | ||
shouldOCR, | ||
fileToDownloadBase64, | ||
} = useDocuments(documents); | ||
} = useDocumentsLogic(documents); | ||
|
||
return ( | ||
<ImageViewer selectedImage={selectedImage} onSelectImage={onSelectImage}> | ||
|
@@ -88,6 +89,8 @@ export const Documents: FunctionComponent<IDocumentsProps> = ({ | |
onOpenDocumentInNewTab={onOpenDocumentInNewTab} | ||
// isRotatedOrTransformed={isRotatedOrTransformed} | ||
shouldDownload={shouldDownload} | ||
shouldOCR={shouldOCR} | ||
onOcrPressed={onOcrPressed} | ||
// isCropping={isCropping} | ||
// isLoadingOCR={isLoadingOCR} | ||
// onCancelCrop={onCancelCrop} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure consistency in parameter naming
The mutation function uses
workflowDefinitionId
, but the hook receivesworkflowId
as a parameter. This inconsistency could lead to confusion.Consider one of the following options:
workflowId
is the correct term, update the fetch function call:Choose the option that best aligns with your project's terminology and the actual purpose of this ID.