rusackas commented on PR #39469: URL: https://github.com/apache/superset/pull/39469#issuecomment-4780974822
Really nice scope here, and thanks for the docs + tests — this is a meaty one. CI is red across the board though (pre-commit, lint-frontend, unit-tests, and every DB integration variant), so we can't merge as-is. A couple of those look like real bugs rather than flakes: In `auth_user_db` you read `current_app.config["AUTH_DB_FAKE_PASSWORD_HASH_CHECK"]` as a subscript, but I don't see that key defined anywhere in `config.py` (only `AUTH_DB_CONFIG` got added). That'll `KeyError` on every failed-login path — was it meant to live under `AUTH_DB_CONFIG`, or get its own default? And in `tests/integration_tests/users/api_tests.py` `test_put_my_password_invalidates_cloned_session_client` calls a bare `login(client_a, ADMIN_USERNAME)` that's never imported. `self.login(...)` is the only one in scope. That'd `NameError` the test, which probably explains part of the red. There's also a healthy stack of unresolved bot threads. If you don't mind giving those a pass to assess/address/resolve (the AUTH_TYPE gating on the admin Users-list reset action and the frontend/backend digit-rule mismatch in `generateAuthDbPassword.ts` looked worth a real look). Once CI's green we'll re-review. Holler if you want a hand. 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
