-
Notifications
You must be signed in to change notification settings - Fork 199
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 6 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 useDocumentOrc = ({ 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, | ||
}); | ||
}, | ||
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 consistency in parameter naming The mutation function uses Consider one of the following options:
-export const useDocumentOcr = ({ workflowId }: { workflowId: string }) => {
+export const useDocumentOcr = ({ workflowDefinitionId }: { workflowDefinitionId: string }) => {
// ... (update all occurrences of workflowId to workflowDefinitionId)
return fetchWorkflowDocumentOCRResult({
- workflowDefinitionId: workflowId,
+ workflowId,
documentId,
}); Choose the option that best aligns with your project's terminology and the actual purpose of this ID.
|
||
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 { useDocumentOrc } from '@/domains/entities/hooks/mutations/useDocumentOcr/useDocumentOcr'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
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. Typo in Hook Name: Correct The import statement imports Apply this diff to correct the hook name: -import { useDocumentOrc } from '@/domains/entities/hooks/mutations/useDocumentOcr/useDocumentOcr';
+import { useDocumentOcr } from '@/domains/entities/hooks/mutations/useDocumentOcr/useDocumentOcr'; 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
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, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} = useDocumentOrc({ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
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. Typo |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
workflowId: workflow?.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 Hook Usage: Rename The hook is referred to as Apply this diff to correct the hook usage: const {
- mutate: mutateOCRDocument,
- isLoading: isLoadingOCRDocument,
- data: ocrResult,
-} = useDocumentOrc({
+ mutate: mutateOcrDocument,
+ isLoading: isLoadingOcrDocument,
+ data: ocrResult,
+} = useDocumentOcr({
workflowId: workflow?.id,
});
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
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 |
---|---|---|
@@ -0,0 +1,29 @@ | ||
import { useCaseState } from '@/pages/Entity/components/Case/hooks/useCaseState/useCaseState'; | ||
import { TWorkflowById } from '@/domains/workflows/fetchers'; | ||
import { CommonWorkflowEvent, CommonWorkflowStates, DefaultContextSchema } from '@ballerine/common'; | ||
import { checkCanMakeDecision } from '@/lib/blocks/hooks/useDocumentBlocks/utils/check-can-make-decision/check-can-make-decision'; | ||
|
||
export const checkCanOcr = ({ | ||
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. find better naming 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. @alonp99, I agree that the function name ✏️ Learnings added
|
||
caseState, | ||
noAction, | ||
workflow, | ||
decision, | ||
isLoadingApprove, | ||
}: { | ||
caseState: ReturnType<typeof useCaseState>; | ||
noAction: boolean; | ||
workflow: TWorkflowById; | ||
decision: DefaultContextSchema['documents'][number]['decision']; | ||
isLoadingApprove: boolean; | ||
}) => { | ||
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. can you explain why this function is needed? 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. removed |
||
const isStateManualReview = workflow.state === CommonWorkflowStates.MANUAL_REVIEW; | ||
|
||
const hasApproveEvent = workflow?.nextEvents?.includes(CommonWorkflowEvent.APPROVE); | ||
const canMakeDecision = checkCanMakeDecision({ | ||
caseState, | ||
noAction, | ||
decision, | ||
}); | ||
|
||
return !isLoadingApprove && canMakeDecision && (isStateManualReview || hasApproveEvent); | ||
}; |
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.
Fix typo in hook name:
useDocumentOrc
should beuseDocumentOcr
There's a typo in the hook name. OCR stands for Optical Character Recognition, so the correct spelling should be "Ocr" instead of "Orc".
Please apply the following change:
This will ensure consistency with the file name and prevent potential confusion.
📝 Committable suggestion