-
Notifications
You must be signed in to change notification settings - Fork 14.4k
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
feat: keep modal open when saving database failed #11618
feat: keep modal open when saving database failed #11618
Conversation
Codecov Report
@@ Coverage Diff @@
## master #11618 +/- ##
==========================================
- Coverage 65.76% 62.26% -3.50%
==========================================
Files 873 873
Lines 42316 42321 +5
Branches 3972 3979 +7
==========================================
- Hits 27827 26350 -1477
- Misses 14374 15791 +1417
- Partials 115 180 +65
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
@ktmud can we just show the actual message from the field? the structure for the backend error messages follows the marshmallow way, so, we get for each field an error message, this way the user can be warned about multiple errors from multiple fields in one go. |
That would require a much bigger refactor so maybe let's work it in another PR. |
c9b7c62
to
b9389a0
Compare
b9389a0
to
845da97
Compare
845da97
to
ba23133
Compare
27decdc
to
779d558
Compare
? `${t('ERROR: ')}${ | ||
typeof error.message === 'string' | ||
? error.message | ||
: (error.message as Record<string, string[]>).sqlalchemy_uri |
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.
The API sometimes has message
being a dictionary of validation errors by field name, but the frontend is not able to handle that. This is just a one-off stop-gap fix which is not very scalable.
We should do a larger refactor to either explode the validation errors as multiple Error objects from the server side, handle them within getClientErrorObject
, or more preferably, in superset-ui/core
's makeApi
.
I'll add this to my todo but if anyone will be working on this area soon, feel free to take over.
cc @etr2460 since you've been working on client-side error messages.
Yes, this LGTM. As far as per-field error handling, this is an issue for all our create/edit modals. We really should have a form state with per-field errors so that we can display them inline on the form instead of in toasts. That's beyond the scope of this PR, but it is an issue with these new react crud views/forms. |
I'll do some manual testing in a bit before ✅ |
SUMMARY
When adding or editing a database, users may fail to save the database because of a typo in their connection string. Keep the modal open makes it easier for them to correct the typo because they don't have to open the modal again.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
After
TEST PLAN
ADDITIONAL INFORMATION
cc @riahk @nytai