You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2022/05/17 10:44:24 UTC
[GitHub] [superset] prosdev0107 opened a new pull request, #20095: fix(database): make to display validation error msg when all cases
prosdev0107 opened a new pull request, #20095:
URL: https://github.com/apache/superset/pull/20095
### SUMMARY
The dynamic form to connect to PostgreSQL is not returning connection errors to the user.
No errors display when the error_type is `GENERIC_DB_ENGINE_ERROR ` and so error alerts are displayed at this case.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
BEFORE:
https://user-images.githubusercontent.com/47900232/168793711-b715d27c-2fc2-4504-856d-11a32eeda9f2.mov
AFTER:
https://user-images.githubusercontent.com/47900232/168793251-5f81c1a3-70c8-46e9-bb29-518d86396671.mov
### TESTING INSTRUCTIONS
**How to reproduce the bug**
1. Hover over the + icon > Data > Connect database.
2. Select PostgreSQL.
3. Input everything correctly with the exception of the DB name.
4. Click on CONNECT. No error is shown.
5. Fix the DB name, and now enter an incorrect username.
6. Click on CONNECT. No error is shown.
7. Replace the password to an incorrect value.
8. Click on CONNECT. No error is shown.
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] michael-s-molina commented on pull request #20095: fix(database): make to display validation error msg when all cases
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on PR #20095:
URL: https://github.com/apache/superset/pull/20095#issuecomment-1167647851
> @michael-s-molina I believe the message is actually generic right now - even if your username is wrong, it gives you the same message saying that the password provided doesn't work for the username entered
I'm not sure if this is another error but when I try with an unknown user I get another message:
<img width="513" alt="Screen Shot 2022-06-27 at 2 23 22 PM" src="https://user-images.githubusercontent.com/70410625/175999441-d86a549f-91b6-4320-87fd-338688662938.png">
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] rusackas merged pull request #20095: fix(database): make to display validation error msg when all cases
Posted by GitBox <gi...@apache.org>.
rusackas merged PR #20095:
URL: https://github.com/apache/superset/pull/20095
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] yousoph commented on pull request #20095: fix(database): make to display validation error msg when all cases
Posted by GitBox <gi...@apache.org>.
yousoph commented on PR #20095:
URL: https://github.com/apache/superset/pull/20095#issuecomment-1167668643
Oops, you're right! It does look like it's a diff error when the username is wrong
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] rusackas commented on a diff in pull request #20095: fix(database): make to display validation error msg when all cases
Posted by GitBox <gi...@apache.org>.
rusackas commented on code in PR #20095:
URL: https://github.com/apache/superset/pull/20095#discussion_r894836565
##########
superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.tsx:
##########
@@ -1086,22 +1092,24 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
// eslint-disable-next-line consistent-return
const errorAlert = () => {
let alertErrors: string[] = [];
- if (isEmpty(dbErrors) === false) {
+ if (!isEmpty(dbErrors)) {
Review Comment:
Unit tests to cover this area of code and prove it works would be ideal. Spinning up an ephemeral for manual testing now that CI has passed. Please let us know what manual testing steps would help here.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] rusackas commented on a diff in pull request #20095: fix(database): make to display validation error msg when all cases
Posted by GitBox <gi...@apache.org>.
rusackas commented on code in PR #20095:
URL: https://github.com/apache/superset/pull/20095#discussion_r894777098
##########
superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.tsx:
##########
@@ -1086,22 +1092,24 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
// eslint-disable-next-line consistent-return
const errorAlert = () => {
let alertErrors: string[] = [];
- if (isEmpty(dbErrors) === false) {
+ if (!isEmpty(dbErrors)) {
Review Comment:
Previously, this was `if (isEmpty(dbErrors) === false)`
This new line of `if (!isEmpty(dbErrors))` is logically the same, which is great...
However, now I'm a little worried. Our QA round didn't catch anything wrong when this logic was indeed wrong. So... what does this exactly DO, and how do we test it to make sure it's working as expected?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] stephenLYZ commented on a diff in pull request #20095: fix(database): make to display validation error msg when all cases
Posted by GitBox <gi...@apache.org>.
stephenLYZ commented on code in PR #20095:
URL: https://github.com/apache/superset/pull/20095#discussion_r875399169
##########
superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.tsx:
##########
@@ -1088,7 +1088,7 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
let alertErrors: string[] = [];
if (isEmpty(dbErrors) === false) {
Review Comment:
Can we also modify this condition to keep the code readable to be consistent? Thanks!
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] yousoph commented on pull request #20095: fix(database): make to display validation error msg when all cases
Posted by GitBox <gi...@apache.org>.
yousoph commented on PR #20095:
URL: https://github.com/apache/superset/pull/20095#issuecomment-1167635719
@michael-s-molina I believe the message is actually generic right now - even if your username is wrong, it gives you the same message saying that the password provided doesn't work for the username entered
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] github-actions[bot] commented on pull request #20095: fix(database): make to display validation error msg when all cases
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #20095:
URL: https://github.com/apache/superset/pull/20095#issuecomment-1148220532
@yousoph Ephemeral environment spinning up at http://34.219.229.127:8080. Credentials are `admin`/`admin`. Please allow several minutes for bootstrapping and startup.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] prosdev0107 commented on a diff in pull request #20095: fix(database): make to display validation error msg when all cases
Posted by GitBox <gi...@apache.org>.
prosdev0107 commented on code in PR #20095:
URL: https://github.com/apache/superset/pull/20095#discussion_r875224315
##########
superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.tsx:
##########
@@ -1088,7 +1088,7 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
let alertErrors: string[] = [];
if (isEmpty(dbErrors) === false) {
alertErrors = typeof dbErrors === 'object' ? Object.values(dbErrors) : [];
- } else if (db?.engine === Engines.Snowflake) {
+ } else if (isEmpty(validationErrors) === false) {
Review Comment:
It is because that the error alerts should be displayed when all error cases at all db engine as well as snowflake
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] stephenLYZ commented on a diff in pull request #20095: fix(database): make to display validation error msg when all cases
Posted by GitBox <gi...@apache.org>.
stephenLYZ commented on code in PR #20095:
URL: https://github.com/apache/superset/pull/20095#discussion_r888945565
##########
superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.tsx:
##########
@@ -114,6 +114,10 @@ const TabsStyled = styled(Tabs)`
}
`;
+const ErrorAlertContainer = styled.div`
+ margin: 32px 16px;
Review Comment:
suggest using Emotion theme variables here.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] yousoph commented on pull request #20095: fix(database): make to display validation error msg when all cases
Posted by GitBox <gi...@apache.org>.
yousoph commented on PR #20095:
URL: https://github.com/apache/superset/pull/20095#issuecomment-1147163356
@prosdev0107
this is looking good! Two small changes:
1. In the copy, let's put "See more" in quotes. The second sentence should start:
> Click "See more" for database-provided... etc etc
2. Can we move "See more" to the bottom of the error message? See the [design guidelines](https://github.com/apache/superset/wiki/Error-message-design-guidelines) for an example
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] yousoph commented on pull request #20095: fix(database): make to display validation error msg when all cases
Posted by GitBox <gi...@apache.org>.
yousoph commented on PR #20095:
URL: https://github.com/apache/superset/pull/20095#issuecomment-1148226917
Thanks for the changes! Looking good to me
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] rusackas commented on pull request #20095: fix(database): make to display validation error msg when all cases
Posted by GitBox <gi...@apache.org>.
rusackas commented on PR #20095:
URL: https://github.com/apache/superset/pull/20095#issuecomment-1152670814
/testenv up
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] prosdev0107 commented on pull request #20095: fix(database): make to display validation error msg when all cases
Posted by GitBox <gi...@apache.org>.
prosdev0107 commented on PR #20095:
URL: https://github.com/apache/superset/pull/20095#issuecomment-1147727637
> @prosdev0107 this is looking good! Two small changes:
>
> 1. In the copy, let's put "See more" in quotes. The second sentence should start:
>
> > Click "See more" for database-provided... etc etc
>
> 2. Can we move "See more" to the bottom of the error message? See the [design guidelines](https://github.com/apache/superset/wiki/Error-message-design-guidelines) for an example
>
> 
@yousoph
Thank you for your feedback and Resolved them now!
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] yousoph commented on pull request #20095: fix(database): make to display validation error msg when all cases
Posted by GitBox <gi...@apache.org>.
yousoph commented on PR #20095:
URL: https://github.com/apache/superset/pull/20095#issuecomment-1133215014
/testenv up
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] github-actions[bot] commented on pull request #20095: fix(database): make to display validation error msg when all cases
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #20095:
URL: https://github.com/apache/superset/pull/20095#issuecomment-1145307682
@rusackas Ephemeral environment spinning up at http://54.149.117.75:8080. Credentials are `admin`/`admin`. Please allow several minutes for bootstrapping and startup.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] rusackas commented on pull request #20095: fix(database): make to display validation error msg when all cases
Posted by GitBox <gi...@apache.org>.
rusackas commented on PR #20095:
URL: https://github.com/apache/superset/pull/20095#issuecomment-1209618163
Conflicts resolved... let's merge if/when CI passes
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] stephenLYZ commented on a diff in pull request #20095: fix(database): make to display validation error msg when all cases
Posted by GitBox <gi...@apache.org>.
stephenLYZ commented on code in PR #20095:
URL: https://github.com/apache/superset/pull/20095#discussion_r888943482
##########
superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.tsx:
##########
@@ -1086,22 +1090,24 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
// eslint-disable-next-line consistent-return
const errorAlert = () => {
let alertErrors: string[] = [];
- if (isEmpty(dbErrors) === false) {
+ if (isEmpty(dbErrors)) {
Review Comment:
```suggestion
if (!isEmpty(dbErrors)) {
```
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] rusackas commented on pull request #20095: fix(database): make to display validation error msg when all cases
Posted by GitBox <gi...@apache.org>.
rusackas commented on PR #20095:
URL: https://github.com/apache/superset/pull/20095#issuecomment-1145303993
/testenv up
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] rusackas commented on pull request #20095: fix(database): make to display validation error msg when all cases
Posted by GitBox <gi...@apache.org>.
rusackas commented on PR #20095:
URL: https://github.com/apache/superset/pull/20095#issuecomment-1177049206
@yousoph is this OK to merge if the errors displayed are at the whim of the database? We can catch/throw an exception as Yongjie suggests - but maybe we can do that in a separate PR? ¯\_(ツ)_/¯
Otherwise, @prosdev0107 I think this just needs a rebase or conflict resolution
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] yousoph commented on pull request #20095: fix(database): make to display validation error msg when all cases
Posted by GitBox <gi...@apache.org>.
yousoph commented on PR #20095:
URL: https://github.com/apache/superset/pull/20095#issuecomment-1148218139
/testenv up
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] github-actions[bot] commented on pull request #20095: fix(database): make to display validation error msg when all cases
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #20095:
URL: https://github.com/apache/superset/pull/20095#issuecomment-1152675665
@rusackas Ephemeral environment spinning up at http://35.85.218.215:8080. Credentials are `admin`/`admin`. Please allow several minutes for bootstrapping and startup.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] michael-s-molina commented on pull request #20095: fix(database): make to display validation error msg when all cases
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on PR #20095:
URL: https://github.com/apache/superset/pull/20095#issuecomment-1167590359
Hi @prosdev0107. Thanks for the PR. I have two suggestions:
Can we treat invalid usernames and passwords with a generic message? Right now the message confirms that my username is valid and only my password is invalid which raises security concerns. Maybe something like "Invalid credentials" for both the username and password?
<img width="498" alt="Screen Shot 2022-06-27 at 1 35 18 PM" src="https://user-images.githubusercontent.com/70410625/175991002-97464095-3473-45f4-9ffd-467ee1f5622b.png">
The second suggestion is to automatically scroll to the bottom when there's an error because it's not very clear right now.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] prosdev0107 commented on a diff in pull request #20095: fix(database): make to display validation error msg when all cases
Posted by GitBox <gi...@apache.org>.
prosdev0107 commented on code in PR #20095:
URL: https://github.com/apache/superset/pull/20095#discussion_r896212972
##########
superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.tsx:
##########
@@ -1086,22 +1092,24 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
// eslint-disable-next-line consistent-return
const errorAlert = () => {
let alertErrors: string[] = [];
- if (isEmpty(dbErrors) === false) {
+ if (!isEmpty(dbErrors)) {
Review Comment:
@evans
I tried to add the unit test for this case but couldn't do it because of that getValidation function is a call in a hooks and so it is invalid hook call in jest file. And so I add manual testing steps into the Test instructions
##########
superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.tsx:
##########
@@ -1086,22 +1092,24 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
// eslint-disable-next-line consistent-return
const errorAlert = () => {
let alertErrors: string[] = [];
- if (isEmpty(dbErrors) === false) {
+ if (!isEmpty(dbErrors)) {
Review Comment:
@rusackas
I tried to add the unit test for this case but couldn't do it because of that getValidation function is a call in a hooks and so it is invalid hook call in jest file. And so I add manual testing steps into the Test instructions
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov[bot] commented on pull request #20095: fix(database): make to display validation error msg when all cases
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on PR #20095:
URL: https://github.com/apache/superset/pull/20095#issuecomment-1128756943
# [Codecov](https://codecov.io/gh/apache/superset/pull/20095?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#20095](https://codecov.io/gh/apache/superset/pull/20095?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (42bcc0a) into [master](https://codecov.io/gh/apache/superset/commit/e69f6292c210d32548308769acd8e670630e9ecd?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (e69f629) will **not change** coverage.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #20095 +/- ##
=======================================
Coverage 66.45% 66.45%
=======================================
Files 1721 1721
Lines 64481 64481
Branches 6796 6796
=======================================
Hits 42854 42854
Misses 19897 19897
Partials 1730 1730
```
| Flag | Coverage Δ | |
|---|---|---|
| javascript | `51.36% <0.00%> (ø)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/20095?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...c/views/CRUD/data/database/DatabaseModal/index.tsx](https://codecov.io/gh/apache/superset/pull/20095/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvZGF0YS9kYXRhYmFzZS9EYXRhYmFzZU1vZGFsL2luZGV4LnRzeA==) | `32.33% <0.00%> (ø)` | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/20095?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/20095?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [e69f629...42bcc0a](https://codecov.io/gh/apache/superset/pull/20095?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] stephenLYZ commented on a diff in pull request #20095: fix(database): make to display validation error msg when all cases
Posted by GitBox <gi...@apache.org>.
stephenLYZ commented on code in PR #20095:
URL: https://github.com/apache/superset/pull/20095#discussion_r875028455
##########
superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.tsx:
##########
@@ -1088,7 +1088,7 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
let alertErrors: string[] = [];
if (isEmpty(dbErrors) === false) {
alertErrors = typeof dbErrors === 'object' ? Object.values(dbErrors) : [];
- } else if (db?.engine === Engines.Snowflake) {
+ } else if (isEmpty(validationErrors) === false) {
Review Comment:
looks like `!isEmpty(validationErrors)` is more cleaner. So I'm curious that why we need to remove d`b?.engine === Engines.Snowflake` condition.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] github-actions[bot] commented on pull request #20095: fix(database): make to display validation error msg when all cases
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #20095:
URL: https://github.com/apache/superset/pull/20095#issuecomment-1133216443
@yousoph Ephemeral environment spinning up at http://54.214.197.133:8080. Credentials are `admin`/`admin`. Please allow several minutes for bootstrapping and startup.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] github-actions[bot] commented on pull request #20095: fix(database): make to display validation error msg when all cases
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #20095:
URL: https://github.com/apache/superset/pull/20095#issuecomment-1226082960
Ephemeral environment shutdown and build artifacts deleted.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org