[DT-2775] Improve Data Submission Error Messaging #2790
Conversation
kevinmarete
changed the title
This reverts commit a91e061.
|
|
I would suggest pulling all the |
| if (registration.getAssets() != null) { | ||
| // Validate models | ||
| List<AIModel> models = registration.getModels(); | ||
| for (AIModel model : models) { |
There was a problem hiding this comment.
I would suggest moving each class' validation logic into the class itself so the logic can be accessed from anywhere the class is used rather than in this monster-class. I think that would make it easier to collect errors up into some kind of summary.
| (timestamp, type, jsonSerializationContext) -> | ||
| new JsonPrimitive(timestamp.getTime())) | ||
| .create(); | ||
| Gson gson = GsonUtil.gsonBuilderWithAdapters().create(); |
There was a problem hiding this comment.
I believe this is functionally equivalent, but test coverage here is low. Would be nice to ensure no regressions with this.
| }, | ||
| "tags": { "type": "array", "items": { "type": "string" } } | ||
| }, | ||
| "required": ["name", "url", "format", "license", "maintainer"] |
There was a problem hiding this comment.
This is one of the reasons we've tried to get away from using the schema for rule-based validation. Having a schema purely for format/type has been helpful, but logical conditions have made any work in this area extremely slow. This change is effectively a schema version change and theoretically, would require APIs to be backward compatible, or we write new versions to accommodate the new schema version.
Additionally, the UI reads this file when performing validation. Any changes here might require additional UI work to ensure these new properties don't cause regressions.
| try (var response = | ||
| resource.updateVoteRationale( | ||
| authUser, GsonUtil.gsonBuilderWithAdapters().create().toJson(update))) { |
There was a problem hiding this comment.
In this class, we have a constructed gson on line 50 that could be reused in all of the try statements.
| @@ -0,0 +1,34 @@ | |||
| package org.broadinstitute.consent.http.util.gson; | |||
|
|
|||
| import static org.junit.jupiter.api.Assertions.*; | |||
There was a problem hiding this comment.
We don't use star imports. I know we had a project level setting at one point, not sure if that's still the case.
There was a problem hiding this comment.
At a high level, what I was thinking about with this ticket was making changes in line with this documentation here:
https://github.com/networknt/json-schema-validator/blob/1.5.9/doc/cust-msg.md
The examples demonstrate how to enrich the schema with more friendly error messages that can align with the UI expectations we have.
|
Closing this PR, in favor of this #2792 |
Addresses
https://broadworkbench.atlassian.net/browse/DT-2775
Summary
This PR improves submission validation errors so users can easily understand what’s missing and where to fix it. Instead of showing internal schema paths and raw validation output, error messages now use user-facing field names and group missing required fields by asset type (e.g., Study, Dataset, Publication).
When required fields are missing, the system displays a single consolidated error message that lists all missing fields by asset type, helping users quickly navigate to the correct section of the form. Errors are cleared once all required fields are fixed and validation passes.
Have you read CONTRIBUTING.md lately? If not, do that first.