pawarprasad123 commented on code in PR #712: URL: https://github.com/apache/atlas/pull/712#discussion_r3720018190
########## dashboard/src/utils/glossaryImportUtils.ts: ########## @@ -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 = Review Comment: Requires both childObjectName and parentObjectName for @ format -add test for childObjectName-only case (same-glossary shorthand failures from backend) ########## dashboardv2/public/js/views/import/ImportLayoutView.js: ########## @@ -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.'; Review Comment: hardcoded "Glossary import completed..." Use that.isGlossary for notification text; BM import should not say "Glossary import completed...". Suggestion: ImportLayoutView is used for BM too (isGlossary flag exists). Use that.isGlossary to pick message ########## dashboardv2/public/js/views/import/ImportLayoutView.js: ########## @@ -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.'; 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'; Review Comment: line 131–134 issue: termLabel bug: if parentObjectName exists but childObjectName is missing, label becomes "undefined@Glossary" Suggestion: Match React logic: only use @ when both exist ``` var termLabel = 'Unknown term'; if (err_obj.childObjectName && err_obj.parentObjectName) { termLabel = err_obj.childObjectName + '@' + err_obj.parentObjectName; } else if (err_obj.childObjectName) { termLabel = err_obj.childObjectName; } ``` ########## dashboardv2/public/js/views/import/ImportLayoutView.js: ########## @@ -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.'; 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'; + if (err_obj.parentObjectName) { + termLabel = err_obj.childObjectName + '@' + err_obj.parentObjectName; + } + errorStr += '<li>' + (index + 1) + '. ' + _.escape(termLabel) + ': ' + _.escape(err_obj.remarks || '') + '</li>'; + }); Review Comment: line 131-136 Duplicated formatting logic vs React util Acceptable for legacy JS, but consider extracting shared helper if both dashboards must stay in sync ########## dashboard/src/utils/glossaryImportUtils.ts: ########## @@ -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 = + 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.`; Review Comment: Summary string is hardcoded glossary-specific ("Glossary import completed...") - Consider renaming to make scope explicit, e.g. buildGlossaryImportFailureSummary, and do not reuse for Business Metadata ########## dashboard/src/components/ImportDialog.tsx: ########## @@ -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) Review Comment: 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: ``` toastId.current = toast.error( buildGlossaryImportFailureSummary(importResp.data) // ← "Glossary import completed..." ); ``` -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: ``` const isGlossaryImport = title !== "Import Business Metadata"; toast.error( isGlossaryImport ? buildGlossaryImportFailureSummary(importResp.data) : importResp.data.failedImportInfoList[0]?.remarks ?? "Import failed" ); ``` Same guard needed in the error details list (line 207–218). ########## dashboard/src/views/Glossary/AddUpdateGlossaryForm.tsx: ########## @@ -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) Review Comment: Please add tests for glossary import failure: summary toast, formatted error list, and back-to-upload navigation — similar to ImportDialog.test.tsx. ########## dashboard/src/utils/__tests__/glossaryImportUtils.test.ts: ########## @@ -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." + ); + }); +}); Review Comment: Missing negative / edge cases: Only remarks (no names) Only childObjectName Empty both lists → "0 failure(s) out of 0 term(s)" Only failures, no successes Empty remarks → fallback "Import failed" suggestion: line 23: Please add negative/edge tests: remarks-only, child-only, empty lists, failures-only (no successes). ########## dashboard/src/components/ImportDialog.tsx: ########## @@ -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> ) Review Comment: Blocker: Test Suites: 1 failed, 2 passed, 3 total Tests: 1 failed, 48 passed, 49 total Failure: ImportDialog.test.tsx — expectations not updated for new behavior: Suggestion: (ImportDialog.test.tsx line ~103–122): Tests must be updated to match new toast summary and formatted error lines. Also add a Business Metadata failure test to ensure glossary formatting is not applied there. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
