-
Notifications
You must be signed in to change notification settings - Fork 903
ATLAS-5322: Fix glossary bulk import relation parsing and error reporting. #712
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
base: master
Are you sure you want to change the base?
Changes from 1 commit
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 |
|---|---|---|
|
|
@@ -39,6 +39,11 @@ import ListItemText from "@mui/material/ListItemText"; | |
| import ArrowBackIosNewIcon from "@mui/icons-material/ArrowBackIosNew"; | ||
| import { postGlossaryImportFormData } from "@utils/glossaryImportFlow"; | ||
| import { getApiErrorToastMessage } from "@utils/apiErrorToastMessage"; | ||
| import { | ||
| buildGlossaryImportFailureSummary, | ||
| formatGlossaryImportFailure, | ||
| GlossaryImportFailure | ||
| } from "@utils/glossaryImportUtils"; | ||
|
|
||
| const BootstrapDialog = styled(Dialog)(({ theme }) => ({ | ||
| "& .MuiDialogContent-root": { | ||
|
|
@@ -113,7 +118,7 @@ export const ImportDialog: React.FC<CustomModalProps> = ({ | |
| if (importResp.data.failedImportInfoList != undefined) { | ||
| toast.dismiss(toastId.current); | ||
| toastId.current = toast.error( | ||
| importResp.data.failedImportInfoList[0].remarks | ||
| buildGlossaryImportFailureSummary(importResp.data) | ||
| ); | ||
|
|
||
| setErrorDetails(true); | ||
|
|
@@ -200,17 +205,15 @@ export const ImportDialog: React.FC<CustomModalProps> = ({ | |
| > | ||
| <List> | ||
| {importData.failedImportInfoList.map( | ||
| ( | ||
| value: { | ||
| index: number; | ||
| remarks: string; | ||
| }, | ||
| index: number | ||
| ) => ( | ||
| <ListItem key={value.index} disableGutters disablePadding> | ||
| (value: GlossaryImportFailure, index: number) => ( | ||
| <ListItem | ||
| key={`${value.childObjectName || "term"}-${index}`} | ||
| disableGutters | ||
| disablePadding | ||
| > | ||
| <ListItemText | ||
| className="dropzone-listitem" | ||
| primary={`${index + 1}. ${value.remarks}`} | ||
| primary={`${index + 1}. ${formatGlossaryImportFailure(value)}`} | ||
| /> | ||
| </ListItem> | ||
| ) | ||
|
Contributor
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. Blocker: Test Suites: 1 failed, 2 passed, 3 total Failure: ImportDialog.test.tsx — expectations not updated for new behavior: Suggestion: (ImportDialog.test.tsx line ~103–122): |
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,50 @@ | ||
| /* | ||
| * Licensed to the Apache Software Foundation (ASF) under one or more | ||
| * contributor license agreements. See the NOTICE file distributed with | ||
| * this work for additional information regarding copyright ownership. | ||
| * The ASF licenses this file to You under the Apache License, Version 2.0 | ||
| * (the "License"); you may not use this file except in compliance with | ||
| * the License. You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
|
|
||
| import { | ||
| buildGlossaryImportFailureSummary, | ||
| formatGlossaryImportFailure | ||
| } from "../glossaryImportUtils"; | ||
|
|
||
| describe("glossaryImportUtils", () => { | ||
| it("formats failure with glossary term label", () => { | ||
| expect( | ||
| formatGlossaryImportFailure({ | ||
| childObjectName: "Patient", | ||
| parentObjectName: "Healthcare Glossary", | ||
| remarks: "Reference not found" | ||
| }) | ||
| ).toBe("Patient@Healthcare Glossary: Reference not found"); | ||
| }); | ||
|
|
||
| it("builds import summary with success and failure counts", () => { | ||
| expect( | ||
| buildGlossaryImportFailureSummary({ | ||
| successImportInfoList: [{ childObjectName: "A" }], | ||
| failedImportInfoList: [ | ||
| { | ||
| childObjectName: "Patient", | ||
| parentObjectName: "Healthcare Glossary", | ||
| remarks: "Invalid relation" | ||
| } | ||
| ] | ||
| }) | ||
| ).toBe( | ||
| "Glossary import completed with 1 failure(s) out of 2 term(s). See error details." | ||
| ); | ||
| }); | ||
| }); | ||
|
Contributor
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. Missing negative / edge cases: Only remarks (no names) suggestion: |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,49 @@ | ||
| /* | ||
| * Licensed to the Apache Software Foundation (ASF) under one or more | ||
| * contributor license agreements. See the NOTICE file distributed with | ||
| * this work for additional information regarding copyright ownership. | ||
| * The ASF licenses this file to You under the Apache License, Version 2.0 | ||
| * (the "License"); you may not use this file except in compliance with | ||
| * the License. You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
|
|
||
| export interface GlossaryImportFailure { | ||
| childObjectName?: string; | ||
| parentObjectName?: string; | ||
| remarks?: string; | ||
| rowNumber?: number; | ||
| } | ||
|
|
||
| export interface GlossaryImportResponse { | ||
| failedImportInfoList?: GlossaryImportFailure[]; | ||
| successImportInfoList?: GlossaryImportFailure[]; | ||
| } | ||
|
|
||
| export const formatGlossaryImportFailure = ( | ||
| failure: GlossaryImportFailure | ||
| ): string => { | ||
| const termLabel = | ||
|
Contributor
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. Requires both childObjectName and parentObjectName for @ format -add test for childObjectName-only case (same-glossary shorthand failures from backend) |
||
| failure.childObjectName && failure.parentObjectName | ||
| ? `${failure.childObjectName}@${failure.parentObjectName}` | ||
| : failure.childObjectName || "Unknown term"; | ||
|
|
||
| return `${termLabel}: ${failure.remarks || "Import failed"}`; | ||
| }; | ||
|
|
||
| export const buildGlossaryImportFailureSummary = ( | ||
| response: GlossaryImportResponse | ||
| ): string => { | ||
| const failedCount = response.failedImportInfoList?.length || 0; | ||
| const successCount = response.successImportInfoList?.length || 0; | ||
| const totalCount = failedCount + successCount; | ||
|
|
||
| return `Glossary import completed with ${failedCount} failure(s) out of ${totalCount} term(s). See error details.`; | ||
|
Contributor
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. Summary string is hardcoded glossary-specific ("Glossary import completed...")
|
||
| }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,6 +28,11 @@ import { | |
| postGlossaryImportFormData | ||
| } from "@utils/glossaryImportFlow"; | ||
| import { getApiErrorToastMessage } from "@utils/apiErrorToastMessage"; | ||
| import { | ||
| buildGlossaryImportFailureSummary, | ||
| formatGlossaryImportFailure, | ||
| GlossaryImportFailure | ||
| } from "@utils/glossaryImportUtils"; | ||
| import { toast } from "react-toastify"; | ||
| import { useCallback, useEffect, useRef, useState } from "react"; | ||
| import type { MouseEvent } from "react"; | ||
|
|
@@ -163,7 +168,7 @@ const AddUpdateGlossaryForm = (props: { | |
| if (importResp.data.failedImportInfoList != undefined) { | ||
| toast.dismiss(toastId.current); | ||
| toastId.current = toast.error( | ||
| importResp.data.failedImportInfoList[0].remarks | ||
| buildGlossaryImportFailureSummary(importResp.data) | ||
|
Contributor
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. Please add tests for glossary import failure: summary toast, formatted error list, and back-to-upload navigation — similar to ImportDialog.test.tsx. |
||
| ); | ||
| setImportErrorDetails(true); | ||
| } | ||
|
|
@@ -361,17 +366,15 @@ const AddUpdateGlossaryForm = (props: { | |
| > | ||
| <List> | ||
| {importData.failedImportInfoList.map( | ||
| ( | ||
| value: { | ||
| index: number; | ||
| remarks: string; | ||
| }, | ||
| index: number | ||
| ) => ( | ||
| <ListItem key={value.index} disableGutters disablePadding> | ||
| (value: GlossaryImportFailure, index: number) => ( | ||
| <ListItem | ||
| key={`${value.childObjectName || "term"}-${index}`} | ||
| disableGutters | ||
| disablePadding | ||
| > | ||
| <ListItemText | ||
| className="dropzone-listitem" | ||
| primary={`${index + 1}. ${value.remarks}`} | ||
| primary={`${index + 1}. ${formatGlossaryImportFailure(value)}`} | ||
| /> | ||
| </ListItem> | ||
| ) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -117,18 +117,25 @@ define([ | |
| var success = true; | ||
| if (response.failedImportInfoList && response.failedImportInfoList.length) { | ||
| var errorStr = '', | ||
| notificationMsg = ''; | ||
| failedCount = response.failedImportInfoList.length, | ||
| successCount = (response.successImportInfoList && response.successImportInfoList.length) || 0, | ||
| totalCount = failedCount + successCount, | ||
| notificationMsg = 'Glossary import completed with ' + failedCount + ' failure(s) out of ' + totalCount + ' term(s). See error details.'; | ||
|
Contributor
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. hardcoded "Glossary import completed..." Use that.isGlossary for notification text; BM import should not say "Glossary import completed...". Suggestion: |
||
| success = false; | ||
| that.ui.errorDetails.empty(); | ||
| Utils.defaultErrorHandler(null, file.xhr, { defaultErrorMessage: response.failedImportInfoList[0].remarks }); | ||
| if (response.failedImportInfoList.length > 1) { | ||
| var modalTitle = '<div class="back-button importBackBtn" title="Back to import file"><i class="fa fa-angle-left "></i> </div> <div class="modal-name">Error Details</div>'; | ||
| _.each(response.failedImportInfoList, function(err_obj) { | ||
| errorStr += '<li>' + _.escape(err_obj.remarks) + '</li>'; | ||
| }); | ||
| that.ui.errorDetails.append(errorStr); | ||
| that.toggleErrorAndDropZoneView({ title: modalTitle, isErrorView: true }); | ||
| } | ||
| Utils.notifyError({ | ||
| content: notificationMsg | ||
| }); | ||
| var modalTitle = '<div class="back-button importBackBtn" title="Back to import file"><i class="fa fa-angle-left "></i> </div> <div class="modal-name">Error Details</div>'; | ||
| _.each(response.failedImportInfoList, function(err_obj, index) { | ||
| var termLabel = err_obj.childObjectName || 'Unknown term'; | ||
|
Contributor
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. line 131–134 Suggestion: |
||
| if (err_obj.parentObjectName) { | ||
| termLabel = err_obj.childObjectName + '@' + err_obj.parentObjectName; | ||
| } | ||
| errorStr += '<li>' + (index + 1) + '. ' + _.escape(termLabel) + ': ' + _.escape(err_obj.remarks || '') + '</li>'; | ||
| }); | ||
|
Contributor
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. line 131-136 Duplicated formatting logic vs React util |
||
| that.ui.errorDetails.append(errorStr); | ||
| that.toggleErrorAndDropZoneView({ title: modalTitle, isErrorView: true }); | ||
| } | ||
| if (success) { | ||
| that.modal.trigger("cancel"); | ||
|
|
||
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.
Critical issue Blovker: — Business Metadata regression:
ImportDialog serves both glossary and business metadata (title == "Import Business Metadata"). The PR applies glossary-specific formatting to the shared failure block:
-And error details use formatGlossaryImportFailure(), which assumes glossary semantics (child@parent). For business metadata, backend stores fields differently (parentObjectName = guid, childObjectName = attributes), producing misleading labels like attributes@guid: error.
solution:
ImportDialog is shared between glossary and business metadata imports. Please guard glossary-specific formatting:
Same guard needed in the error details list (line 207–218).