Skip to content

Commit

Permalink
[ODS-6253] Improve error message for RequiredWithNonDefault values (#…
Browse files Browse the repository at this point in the history 
…1023)
  • Loading branch information
@simpat-jesus
simpat-jesus authored Apr 18, 2024
1 parent b5c64bc commit fd65fb8
Show file tree
Hide file tree
Showing 5 changed files with 89 additions and 23 deletions.
24 changes: 17 additions & 7 deletions Application/EdFi.Ods.Common/Attributes/RequiredWithNonDefaultAttribute.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,6 @@

using System.ComponentModel.DataAnnotations;
using EdFi.Ods.Common.Extensions;
using EdFi.Ods.Common.Security;
using EdFi.Ods.Common.Validation;

namespace EdFi.Ods.Common.Attributes
{
Expand All @@ -21,10 +19,12 @@ protected override ValidationResult IsValid(object value, ValidationContext vali
{
if (value is string s)
{
if (!string.IsNullOrEmpty(s))
if (string.IsNullOrEmpty(s))
{
return ValidationResult.Success;
return BuildValidationResult($"{validationContext.DisplayName} is required and should not be left empty.");
}

return ValidationResult.Success;
}
else if (value is bool)
{
Expand All @@ -36,13 +36,23 @@ protected override ValidationResult IsValid(object value, ValidationContext vali
// In case of decimal types, accept default values
return ValidationResult.Success;
}
else if (value != null && !value.Equals(value.GetType().GetDefaultValue()))
else if (value != null)
{
if (value.Equals(value.GetType().GetDefaultValue()))
{
return BuildValidationResult($"{validationContext.DisplayName} value must be different than {value.GetType().GetDefaultValue()}.");
}

return ValidationResult.Success;
}

return new ValidationResult($"{validationContext.DisplayName} is required.",
new [] { validationContext.MemberNamePath() });
return BuildValidationResult($"{validationContext.DisplayName} is required.");

ValidationResult BuildValidationResult(string message)
{
return new ValidationResult(message,
new[] { validationContext.MemberNamePath() });
}
}
}
}
8 changes: 4 additions & 4 deletions Application/EdFi.Ods.Tests/EdFi.Ods.Api/Helpers/AssemblyLoaderHelperTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -136,12 +136,12 @@ public void Should_return_all_plugin_assemblies_from_plugin_folder()
.ToArray();

// Assert
returnedAssemblies.ShouldBeEquivalentTo(
returnedAssemblies.ShouldBe(
new[]
{
"EdFi.Ods.Extensions.Sample.dll",
"EdFi.Ods.Profiles.Sample.dll"
});
}, ignoreOrder: true);
}

[Test]
Expand Down Expand Up @@ -172,12 +172,12 @@ public void Should_not_include_non_plugin_assemblies_located_in_plugin_folder()
// Assert
File.Exists(Path.Combine(_unitTestPluginWithNonPluginAssemblyFolder, NonPluginAssemblyFileName)).ShouldBeTrue();

returnedAssemblies.ShouldBeEquivalentTo(
returnedAssemblies.ShouldBe(
new[]
{
"EdFi.Ods.Extensions.Sample.dll",
"EdFi.Ods.Profiles.Sample.dll"
});
}, ignoreOrder: true);
}

[Test]
Expand Down
2 changes: 1 addition & 1 deletion ...te/Ed-Fi ODS-API ChangeQueries Key Changes and Deletes Test Suite.postman_collection.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5305,7 +5305,7 @@
" pm.expect(problemDetails.type).to.equal(\"urn:ed-fi:api:bad-request:data\");\r",
" pm.expect(problemDetails.detail).to.equal(\"Data validation failed. See 'validationErrors' for details.\");\r",
" pm.expect(problemDetails.validationErrors['$.classPeriodName']).to.contain(\"ClassPeriodName must be between 1 and 60 characters in length.\");\r",
" pm.expect(problemDetails.validationErrors['$.classPeriodName']).to.contain(\"ClassPeriodName is required.\")\r",
" pm.expect(problemDetails.validationErrors['$.classPeriodName']).to.contain(\"ClassPeriodName is required and should not be left empty.\")\r",
"});"
],
"type": "text/javascript"
Expand Down
4 changes: 2 additions & 2 deletions ...man Test Suite/Ed-Fi ODS-API Integration Test Suite ResponseTests.postman_collection.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3981,7 +3981,7 @@
"",
" pm.expect(problemDetails.validationErrors).to.deep.equal({",
" \"$.lastSurname\": [",
" \"LastSurname is required.\",",
" \"LastSurname is required and should not be left empty.\",",
" \"LastSurname must be between 1 and 75 characters in length.\"",
" ]",
" });",
Expand Down Expand Up @@ -6212,7 +6212,7 @@
"});",
"",
"pm.test(\"Should return a message indicating that Validation of 'Student' failed.LastSurname is required.\", () => {",
" pm.expect(problemDetails.validationErrors[\"$.lastSurname\"]).to.contain(\"LastSurname is required.\");",
" pm.expect(problemDetails.validationErrors[\"$.lastSurname\"]).to.contain(\"LastSurname is required and should not be left empty.\");",
"});",
""
],
Expand Down
74 changes: 65 additions & 9 deletions Postman Test Suite/Ed-Fi ODS-API Integration Test Suite.postman_collection.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1006,7 +1006,7 @@
"pm.test(\"Should return the item with WeekIdentifier is required string\", () => {",
" pm.expect(problemDetails.validationErrors).to.deep.equals({",
" \"$.weekIdentifier\": [",
" \"WeekIdentifier is required.\",",
" \"WeekIdentifier is required and should not be left empty.\",",
" \"WeekIdentifier must be between 5 and 80 characters in length.\"",
" ]",
" });",
Expand Down Expand Up @@ -1416,18 +1416,18 @@
" pm.expect(problemDetails.detail).to.equal(\"Data validation failed. See 'validationErrors' for details.\");",
"});",
"",
"pm.test(\"Should return the item with BeginDate must be required\", () => {",
"pm.test(\"Should return the item with BeginDate must be different than default value\", () => {",
" pm.expect(problemDetails.validationErrors).to.deep.include({",
" \"$.beginDate\": [",
" \"BeginDate is required.\"",
" \"BeginDate value must be different than 1/1/0001 12:00:00 AM.\"",
" ]",
" });",
"});",
"",
"pm.test(\"Should return the item with EndDate must be required\", () => {",
"pm.test(\"Should return the item with EndDate must be different than default value\", () => {",
" pm.expect(problemDetails.validationErrors).to.deep.include({",
" \"$.endDate\": [",
" \"EndDate is required.\"",
" \"EndDate value must be different than 1/1/0001 12:00:00 AM.\"",
" ]",
" });",
"});"
Expand Down Expand Up @@ -1982,18 +1982,18 @@
" pm.expect(problemDetails.detail).to.equal(\"Data validation failed. See 'validationErrors' for details.\");",
"});",
"",
"pm.test(\"Should return the item with BeginDate must be required\", () => {",
"pm.test(\"Should return the item with BeginDate must be different than default value\", () => {",
" pm.expect(problemDetails.validationErrors).to.deep.include({",
" \"$.beginDate\": [",
" \"BeginDate is required.\"",
" \"BeginDate value must be different than 1/1/0001 12:00:00 AM.\"",
" ]",
" });",
"});",
"",
"pm.test(\"Should return the item with EndDate must be required\", () => {",
"pm.test(\"Should return the item with EndDate must be different than default value\", () => {",
" pm.expect(problemDetails.validationErrors).to.deep.include({",
" \"$.endDate\": [",
" \"EndDate is required.\"",
" \"EndDate value must be different than 1/1/0001 12:00:00 AM.\"",
" ]",
" });",
"});"
Expand Down Expand Up @@ -10258,6 +10258,62 @@
"response": []
}
]
},
{
"name": "LocalEducationAgencyId",
"item": [
{
"name": "Creating LEA With a Default LocalEducationAgencyId Fails",
"event": [
{
"listen": "test",
"script": {
"exec": [
"pm.test(\"Status code is 400\", () => {\r",
" pm.expect(pm.response.code).to.equal(400);\r",
"});\r",
"\r",
"const problemDetails = pm.response.json();\r",
"\r",
"pm.test(\"The validationErrors response should indicate that the value must be different than the default value.\", () => {\r",
" pm.expect(pm.response.json().validationErrors['$.localEducationAgencyId'][0]).equal(\"LocalEducationAgencyId value must be different than 0.\")\r",
"});\r",
""
],
"type": "text/javascript",
"packages": {}
}
}
],
"request": {
"method": "POST",
"header": [
{
"key": "Content-Type",
"value": "application/json",
"type": "text"
}
],
"body": {
"mode": "raw",
"raw": "{\r\n \"localEducationAgencyId\": 0,\r\n \"categories\": [\r\n {\r\n \"educationOrganizationCategoryDescriptor\": \"uri://ed-fi.org/EducationOrganizationCategoryDescriptor#Local Education Agency\"\r\n }\r\n ],\r\n \"localEducationAgencyCategoryDescriptor\": \"uri://ed-fi.org/LocalEducationAgencyCategoryDescriptor#Charter\",\r\n \"nameOfInstitution\": \"LEA-100001\"\r\n}"
},
"url": {
"raw": "{{ApiBaseUrl}}/data/v3/ed-fi/localEducationAgencies",
"host": [
"{{ApiBaseUrl}}"
],
"path": [
"data",
"v3",
"ed-fi",
"localEducationAgencies"
]
}
},
"response": []
}
]
}
]
}
Expand Down

0 comments on commit fd65fb8

Please sign in to comment.