Skip to content

Commit a5bfefa

Browse files
vchendrixvchendrix
committed
fix(validation): Improve annotations and showValidation error handling
- Addressed issue in `EML211EditorView.showValidation` to correctly display nested validation errors. - Enhanced `EML211.validate` to handle annotation validation more robustly, preventing null errors. - Added unit tests to cover the changes in `EML211EditorView.showValidation`. Fixes #2606
1 parent 1423787 commit a5bfefa

File tree

4 files changed

+189
-65
lines changed

4 files changed

+189
-65
lines changed

‎src/js/models/metadata/eml211/EML211.js

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1709,9 +1709,9 @@ define([
17091709
}
17101710
}
17111711

1712-
// Validate the EMLAnnotation models
1712+
// Validate the EMLAnnotation models, checking for the canonical dataset
17131713
const annotations = this.get("annotations");
1714-
const annotationErrors = annotations.validate();
1714+
const annotationErrors = annotations ? annotations.validate() : [];
17151715

17161716
if (annotationErrors?.length) {
17171717
errors.annotations = annotationErrors.filter(

‎src/js/views/metadata/EML211EditorView.js

Lines changed: 91 additions & 62 deletions
Original file line numberDiff line numberDiff line change
@@ -1218,7 +1218,10 @@ define([
12181218
}
12191219
},
12201220

1221-
/** Show any errors that occured when trying to save changes */
1221+
/**
1222+
* Show any errors that occurred when trying to save changes
1223+
*
1224+
*/
12221225
showValidation() {
12231226
// First clear all the error messaging
12241227
this.$(".notification.error").empty();
@@ -1228,75 +1231,101 @@ define([
12281231

12291232
const errors = this.model.validationError;
12301233

1231-
errors?.forEach((errorMsg, category) => {
1232-
const categoryEls = this.$(`[data-category='${category}']`);
1233-
const dataItemRow = categoryEls.parents(".data-package-item");
1234+
if (errors && typeof errors === "object") {
1235+
Object.entries(errors).forEach(([category, errorMsg]) => {
1236+
if (typeof errorMsg === "string") {
1237+
// Handle string error messages
1238+
this.showError(category, errorMsg);
1239+
} else if (typeof errorMsg === "object") {
1240+
// Handle object error messages by iterating over leaf nodes
1241+
this.showLeafErrors(category, errorMsg);
1242+
}
1243+
});
12341244

1235-
// If this field is in a DataItemView, then delegate to that view
1236-
if (dataItemRow.length && dataItemRow.data("view")) {
1237-
dataItemRow.data("view").showValidation(category, errorMsg);
1238-
return;
1239-
}
1240-
const elsWithViews = categoryEls.filter(
1241-
(el) =>
1242-
$(el).data("view") &&
1243-
$(el).data("view").showValidation &&
1244-
!$(el).data("view").isNew,
1245-
);
1245+
if (Object.keys(errors).length) {
1246+
// Create a list of errors to display in the error message shown to the user
1247+
const errorList = `<ul>${this.getErrorListItem(errors)}</ul>`;
12461248

1247-
if (elsWithViews.length) {
1248-
elsWithViews.forEach((el) => {
1249-
$(el).data("view").showValidation();
1250-
});
1251-
} else if (categoryEls.length) {
1252-
// Show the error message
1253-
categoryEls
1254-
.filter(".notification")
1255-
.addClass("error")
1256-
.text(errorMsg);
1257-
1258-
// Add the error message to inputs
1259-
categoryEls.filter("textarea, input").addClass("error");
1249+
MetacatUI.appView.showAlert(
1250+
`Fix the errors flagged below before submitting: ${errorList}`,
1251+
"alert-error",
1252+
this.$el,
1253+
null,
1254+
{
1255+
remove: true,
1256+
},
1257+
);
12601258
}
1259+
}
1260+
},
12611261

1262-
// Get the link in the table of contents navigation
1263-
let navigationLink = this.$(
1264-
`.side-nav-item[data-category='${category}']`,
1265-
);
1262+
/**
1263+
* Log an error message for a specific category
1264+
*
1265+
* @param category - The category of the error
1266+
* @param errorMsg - The error message to display
1267+
*/
1268+
showError(category, errorMsg) {
1269+
const categoryEls = this.$(`[data-category='${category}']`);
1270+
const dataItemRow = categoryEls.parents(".data-package-item");
12661271

1267-
if (!navigationLink.length) {
1268-
const section = categoryEls.parents("[data-section]");
1269-
navigationLink = this.$(
1270-
`.side-nav-item.${$(section).attr("data-section")}`,
1271-
);
1272-
}
1272+
// If this field is in a DataItemView, then delegate to that view
1273+
if (dataItemRow.length && dataItemRow.data("view")) {
1274+
dataItemRow.data("view").showValidation(category, errorMsg);
1275+
return;
1276+
}
1277+
const elsWithViews = categoryEls.filter(
1278+
(el) =>
1279+
$(el).data("view") &&
1280+
$(el).data("view").showValidation &&
1281+
!$(el).data("view").isNew,
1282+
);
12731283

1274-
// Show the error icon in the table of contents
1275-
navigationLink
1276-
.addClass("error")
1277-
.find(".icon")
1278-
.addClass("error")
1279-
.show();
1280-
1281-
this.model.off(`change:${category}`, this.model.checkValidity);
1282-
this.model.once(`change:${category}`, this.model.checkValidity);
1283-
}, this);
1284-
1285-
if (errors) {
1286-
// Create a list of errors to display in the error message shown to
1287-
// the user
1288-
const errorList = `<ul>${this.getErrorListItem(errors)}</ul>`;
1289-
1290-
MetacatUI.appView.showAlert(
1291-
`Fix the errors flagged below before submitting: ${errorList}`,
1292-
"alert-error",
1293-
this.$el,
1294-
null,
1295-
{
1296-
remove: true,
1297-
},
1284+
if (elsWithViews.length) {
1285+
elsWithViews.forEach((el) => {
1286+
$(el).data("view").showValidation();
1287+
});
1288+
} else if (categoryEls.length) {
1289+
// Show the error message
1290+
categoryEls.filter(".notification").addClass("error").text(errorMsg);
1291+
1292+
// Add the error message to inputs
1293+
categoryEls.filter("textarea, input").addClass("error");
1294+
}
1295+
1296+
// Get the link in the table of contents navigation
1297+
let navigationLink = this.$(
1298+
`.side-nav-item[data-category='${category}']`,
1299+
);
1300+
1301+
if (!navigationLink.length) {
1302+
const section = categoryEls.parents("[data-section]");
1303+
navigationLink = this.$(
1304+
`.side-nav-item.${$(section).attr("data-section")}`,
12981305
);
12991306
}
1307+
1308+
// Show the error icon in the table of contents
1309+
navigationLink.addClass("error").find(".icon").addClass("error").show();
1310+
1311+
this.model.off(`change:${category}`, this.model.checkValidity);
1312+
this.model.once(`change:${category}`, this.model.checkValidity);
1313+
},
1314+
1315+
/**
1316+
* Recursively log the leaf errors in the error object
1317+
*
1318+
* @param category - The category of the error
1319+
* @param errorObj - The object containing the error messages
1320+
*/
1321+
showLeafErrors(category, errorObj) {
1322+
Object.entries(errorObj).forEach(([subCategory, subErrorMsg]) => {
1323+
if (typeof subErrorMsg === "string") {
1324+
this.showError(`${category}`, subErrorMsg);
1325+
} else if (typeof subErrorMsg === "object") {
1326+
this.showLeafErrors(`${subCategory}`, subErrorMsg);
1327+
}
1328+
});
13001329
},
13011330

13021331
/** @inheritdoc */

‎test/config/tests.json

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -92,7 +92,8 @@
9292
"./js/specs/unit/models/ontologies/BioontologyOntology.spec.js",
9393
"./js/specs/unit/models/accordion/Accordion.spec.js",
9494
"./js/specs/unit/models/accordion/AccordionItem.spec.js",
95-
"./js/specs/unit/views/DataItemView.spec.js"
95+
"./js/specs/unit/views/DataItemView.spec.js",
96+
"./js/specs/unit/views/metadata/EML211EditorView.spec.js"
9697
],
9798
"integration": [
9899
"./js/specs/integration/collections/SolrResults.spec.js",

‎test/js/specs/unit/views/metadata/EML211EditorView.spec.js

Lines changed: 94 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,94 @@
1+
define(["jquery", "backbone", "views/metadata/EML211EditorView"], function (
2+
$,
3+
Backbone,
4+
EML211EditorView,
5+
) {
6+
describe("EML211EditorView", function () {
7+
let view, model, sandbox;
8+
9+
beforeEach(function () {
10+
sandbox = sinon.createSandbox();
11+
console.log("beforeEach called");
12+
// Create a mock model with validation errors
13+
model = new Backbone.Model();
14+
model.validationError = {
15+
title: "Error in title",
16+
abstract: "Error in abstract",
17+
methods: {
18+
methodSteps: "Error in step 1",
19+
},
20+
};
21+
22+
// Instantiate the view with the mock model
23+
view = new EML211EditorView({ model: model });
24+
25+
// Spy on the methods that interact with the DOM
26+
sandbox.spy(view, "showError");
27+
sandbox.spy(view, "showLeafErrors");
28+
});
29+
30+
afterEach(function () {
31+
sandbox.restore();
32+
});
33+
34+
it("should log validation errors correctly", function () {
35+
console.log("Test: should log validation errors correctly");
36+
// Call the showValidation method
37+
view.showValidation();
38+
39+
// Assert that showError and showLeafErrors were called with the expected arguments
40+
sinon.assert.calledWith(view.showError, "methods", "Error in step 1");
41+
sinon.assert.calledWith(view.showLeafErrors, "methods", {
42+
methodSteps: "Error in step 1",
43+
});
44+
sinon.assert.calledWith(view.showError, "title", "Error in title");
45+
sinon.assert.calledWith(view.showError, "abstract", "Error in abstract");
46+
});
47+
48+
it("should handle string error messages correctly in showError", function () {
49+
console.log(
50+
"Test: should handle string error messages correctly in showError",
51+
);
52+
// Mock the category elements
53+
view.$ = sandbox.stub().returns({
54+
addClass: sandbox.stub().returnsThis(),
55+
text: sandbox.stub(),
56+
filter: sandbox.stub().returnsThis(),
57+
parents: sandbox.stub().returnsThis(),
58+
data: sandbox.stub().returnsThis(),
59+
find: sandbox.stub().returnsThis(),
60+
show: sandbox.stub(),
61+
});
62+
63+
// Call the showError method
64+
view.showError("methodSteps.step1", "Error in step 1");
65+
66+
// Assert that the appropriate DOM manipulation methods were called
67+
sinon.assert.calledWith(view.$, "[data-category='methodSteps.step1']");
68+
});
69+
70+
it("should handle nested error objects correctly in showLeafErrors", function () {
71+
console.log(
72+
"Test: should handle nested error objects correctly in showLeafErrors",
73+
);
74+
75+
// Call the showLeafErrors method
76+
view.showLeafErrors("methodSteps.step2", {
77+
subStep1: "Error in sub-step 1",
78+
subStep2: "Error in sub-step 2",
79+
});
80+
81+
// Assert that showError was called with the expected arguments
82+
sinon.assert.calledWith(
83+
view.showError,
84+
"methodSteps.step2",
85+
"Error in sub-step 1",
86+
);
87+
sinon.assert.calledWith(
88+
view.showError,
89+
"methodSteps.step2",
90+
"Error in sub-step 2",
91+
);
92+
});
93+
});
94+
});

0 commit comments

Comments
 (0)