From 1162ff7360c628c62a215ee9fe3274120fcd7a04 Mon Sep 17 00:00:00 2001 From: Maxime Beauchemin Date: Wed, 10 Apr 2024 23:13:42 -0700 Subject: [PATCH] improve code & tests --- .gitignore | 1 - docker-compose-image-tag.yml | 30 +++------------ docker-compose-non-dev.yml | 30 +++------------ docker-compose.yml | 44 ++++------------------ scripts/tests/run.sh | 1 + superset/models/user_attributes.py | 23 +---------- superset/views/users/api.py | 40 ++++++++++---------- tests/integration_tests/users/api_tests.py | 21 +++++++++++ 8 files changed, 62 insertions(+), 128 deletions(-) diff --git a/.gitignore b/.gitignore index bbfc640c9acda..02657eb0fa988 100644 --- a/.gitignore +++ b/.gitignore @@ -110,7 +110,6 @@ release.json messages.mo docker/requirements-local.txt -docker/.env-local cache/ docker/*local* diff --git a/docker-compose-image-tag.yml b/docker-compose-image-tag.yml index 6c017ef75e757..07f0d0dcb14b7 100644 --- a/docker-compose-image-tag.yml +++ b/docker-compose-image-tag.yml @@ -33,11 +33,7 @@ services: - redis:/data db: - env_file: - # defaults - - docker/.env - # gitignored overrides - - docker/.env-local + env_file: docker/.env image: postgres:15 container_name: superset_db restart: unless-stopped @@ -46,11 +42,7 @@ services: - ./docker/docker-entrypoint-initdb.d:/docker-entrypoint-initdb.d superset: - env_file: - # defaults - - docker/.env - # gitignored overrides - - docker/.env-local + env_file: docker/.env image: *superset-image container_name: superset_app command: ["/app/docker/docker-bootstrap.sh", "app-gunicorn"] @@ -65,11 +57,7 @@ services: image: *superset-image container_name: superset_init command: ["/app/docker/docker-init.sh"] - env_file: - # defaults - - docker/.env - # gitignored overrides - - docker/.env-local + env_file: docker/.env depends_on: *superset-depends-on user: "root" volumes: *superset-volumes @@ -80,11 +68,7 @@ services: image: *superset-image container_name: superset_worker command: ["/app/docker/docker-bootstrap.sh", "worker"] - env_file: - # defaults - - docker/.env - # gitignored overrides - - docker/.env-local + env_file: docker/.env restart: unless-stopped depends_on: *superset-depends-on user: "root" @@ -100,11 +84,7 @@ services: image: *superset-image container_name: superset_worker_beat command: ["/app/docker/docker-bootstrap.sh", "beat"] - env_file: - # defaults - - docker/.env - # gitignored overrides - - docker/.env-local + env_file: docker/.env restart: unless-stopped depends_on: *superset-depends-on user: "root" diff --git a/docker-compose-non-dev.yml b/docker-compose-non-dev.yml index 78d8cb9c35130..f537e26c38c15 100644 --- a/docker-compose-non-dev.yml +++ b/docker-compose-non-dev.yml @@ -38,11 +38,7 @@ services: - redis:/data db: - env_file: - # defaults - - docker/.env - # gitignored overrides - - docker/.env-local + env_file: docker/.env image: postgres:15 container_name: superset_db restart: unless-stopped @@ -51,11 +47,7 @@ services: - ./docker/docker-entrypoint-initdb.d:/docker-entrypoint-initdb.d superset: - env_file: - # defaults - - docker/.env - # gitignored overrides - - docker/.env-local + env_file: docker/.env build: <<: *common-build container_name: superset_app @@ -72,11 +64,7 @@ services: build: <<: *common-build command: ["/app/docker/docker-init.sh"] - env_file: - # defaults - - docker/.env - # gitignored overrides - - docker/.env-local + env_file: docker/.env depends_on: *superset-depends-on user: "root" volumes: *superset-volumes @@ -88,11 +76,7 @@ services: <<: *common-build container_name: superset_worker command: ["/app/docker/docker-bootstrap.sh", "worker"] - env_file: - # defaults - - docker/.env - # gitignored overrides - - docker/.env-local + env_file: docker/.env restart: unless-stopped depends_on: *superset-depends-on user: "root" @@ -109,11 +93,7 @@ services: <<: *common-build container_name: superset_worker_beat command: ["/app/docker/docker-bootstrap.sh", "beat"] - env_file: - # defaults - - docker/.env - # gitignored overrides - - docker/.env-local + env_file: docker/.env restart: unless-stopped depends_on: *superset-depends-on user: "root" diff --git a/docker-compose.yml b/docker-compose.yml index ca1fd0cc488c3..9252e599027c3 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -54,11 +54,7 @@ services: - redis:/data db: - env_file: - # defaults - - docker/.env - # gitignored overrides - - docker/.env-local + env_file: docker/.env image: postgres:15 container_name: superset_db restart: unless-stopped @@ -69,11 +65,7 @@ services: - ./docker/docker-entrypoint-initdb.d:/docker-entrypoint-initdb.d superset: - env_file: - # defaults - - docker/.env - # gitignored overrides - - docker/.env-local + env_file: docker/.env build: <<: *common-build container_name: superset_app @@ -124,11 +116,7 @@ services: <<: *common-build container_name: superset_init command: ["/app/docker/docker-init.sh"] - env_file: - # defaults - - docker/.env - # gitignored overrides - - docker/.env-local + env_file: docker/.env depends_on: *superset-depends-on user: *superset-user volumes: *superset-volumes @@ -144,14 +132,10 @@ services: # if you do so, you have to run this manually on the host, which should perform better! BUILD_SUPERSET_FRONTEND_IN_DOCKER: ${BUILD_SUPERSET_FRONTEND_IN_DOCKER:-true} SCARF_ANALYTICS: "${SCARF_ANALYTICS}" - PUPPETEER_SKIP_CHROMIUM_DOWNLOAD: ${PUPPETEER_SKIP_CHROMIUM_DOWNLOAD:-true} + PUPPETEER_SKIP_CHROMIUM_DOWNLOAD: ${BUILD_SUPERSET_FRONTEND_IN_DOCKER:-false} container_name: superset_node command: ["/app/docker/docker-frontend.sh"] - env_file: - # defaults - - docker/.env - # gitignored overrides - - docker/.env-local + env_file: docker/.env depends_on: *superset-depends-on volumes: *superset-volumes @@ -160,11 +144,7 @@ services: <<: *common-build container_name: superset_worker command: ["/app/docker/docker-bootstrap.sh", "worker"] - env_file: - # defaults - - docker/.env - # gitignored overrides - - docker/.env-local + env_file: docker/.env restart: unless-stopped depends_on: *superset-depends-on user: *superset-user @@ -182,11 +162,7 @@ services: <<: *common-build container_name: superset_worker_beat command: ["/app/docker/docker-bootstrap.sh", "beat"] - env_file: - # defaults - - docker/.env - # gitignored overrides - - docker/.env-local + env_file: docker/.env restart: unless-stopped depends_on: *superset-depends-on user: *superset-user @@ -199,11 +175,7 @@ services: <<: *common-build container_name: superset_tests_worker command: ["/app/docker/docker-bootstrap.sh", "worker"] - env_file: - # defaults - - docker/.env - # gitignored overrides - - docker/.env-local + env_file: docker/.env environment: DATABASE_HOST: localhost DATABASE_DB: test diff --git a/scripts/tests/run.sh b/scripts/tests/run.sh index 88fa41d44bd94..72d3ef6233195 100755 --- a/scripts/tests/run.sh +++ b/scripts/tests/run.sh @@ -67,6 +67,7 @@ RUN_INIT=1 RUN_RESET_DB=1 RUN_TESTS=1 TEST_MODULE="tests" +TEST_MODULE="tests/integration_tests/users/api_tests.py" PARAMS="" while (( "$#" )); do diff --git a/superset/models/user_attributes.py b/superset/models/user_attributes.py index 4ee6153af75e5..b2af44a188945 100644 --- a/superset/models/user_attributes.py +++ b/superset/models/user_attributes.py @@ -14,10 +14,8 @@ # KIND, either express or implied. See the License for the # specific language governing permissions and limitations # under the License. -from typing import Any - from flask_appbuilder import Model -from sqlalchemy import Column, ForeignKey, Integer, String +from sqlalchemy import Column, ForeignKey, Integer from sqlalchemy.orm import relationship from superset import security_manager @@ -41,23 +39,6 @@ class UserAttribute(Model, AuditMixinNullable): user = relationship( security_manager.user_model, backref="extra_attributes", foreign_keys=[user_id] ) + welcome_dashboard_id = Column(Integer, ForeignKey("dashboards.id")) welcome_dashboard = relationship("Dashboard") - avatar_url = Column(String(100)) - - -class OwnershipMixin: - @property - def owners_with_attributes(self) -> list[dict[str, Any]]: - owners = [] - for owner in self.owners: # type: ignore - extra_attribute_dict: dict[str, Any] = {} - extra_attribute = owner.extra_attributes[0] - owner_dict = { - "first_name": owner.first_name, - "last_name": owner.last_name, - "welcome_dashboard_id": extra_attribute.welcome_dashboard_id, - "avatar_url": extra_attribute.avatar_url, - } - owners.append(owner_dict) - return owners diff --git a/superset/views/users/api.py b/superset/views/users/api.py index 4b3b90c0bc94e..931d40c82b2e7 100644 --- a/superset/views/users/api.py +++ b/superset/views/users/api.py @@ -133,29 +133,29 @@ def avatar(self, user_id: str) -> Response: """ try: avatar_url = None - if not user_id: - return self.response_401() - # Fetching the user's avatar from the database - user_attrs = ( - db.session.query(UserAttribute).filter_by(user_id=user_id).first() + if not user_id.isdigit(): + return self.response_404() + + user = ( + db.session.query(security_manager.user_model) + .filter_by(id=user_id) + .first() ) + if not user: + return self.response_404() + + # fetch from the one-to-one relationship + if len(user.extra_attributes) > 0: + avatar_url = user.extra_attributes[0].avatar_url + + if not avatar_url and app.config.get("SLACK_ENABLE_AVATARS"): + avatar_url = get_user_avatar(user.email) - if not user_attrs or not user_attrs.avatar_url: - if app.config.get("SLACK_ENABLE_AVATARS"): - user = ( - db.session.query(security_manager.user_model) - .filter_by(id=user_id) - .first() - ) - avatar_url = get_user_avatar(user.email) - - # Saving the avatar url to the database - user_attrs = UserAttribute(user_id=user_id, avatar_url=avatar_url) - db.session.add(user_attrs) - db.session.commit() - elif user_attrs: - avatar_url = user_attrs.avatar_url + # Saving the avatar url to the database + user_attrs = UserAttribute(user_id=user_id, avatar_url=avatar_url) + db.session.add(user_attrs) + db.session.commit() if avatar_url: return redirect(avatar_url, code=301) diff --git a/tests/integration_tests/users/api_tests.py b/tests/integration_tests/users/api_tests.py index 5d7ebd61fbd6a..1dc242a596fc4 100644 --- a/tests/integration_tests/users/api_tests.py +++ b/tests/integration_tests/users/api_tests.py @@ -20,6 +20,7 @@ from unittest.mock import patch from superset import security_manager +from superset.utils import slack from tests.integration_tests.base_tests import SupersetTestCase from tests.integration_tests.constants import ADMIN_USERNAME @@ -62,3 +63,23 @@ def test_get_me_anonymous(self, mock_g): mock_g.user = security_manager.get_anonymous_user rv = self.client.get(meUri) self.assertEqual(401, rv.status_code) + + +class TestUserApi(SupersetTestCase): + AVATAR_URL = "https://example.com/avatar.png" + + def test_avatar_with_invalid_user(self): + self.login(ADMIN_USERNAME) + response = self.client.get("/api/v1/user/NOT_A_USER/avatar.png") + assert response.status_code == 404 # Assuming no user found leads to 404 + response = self.client.get("/api/v1/user/999/avatar.png") + assert response.status_code == 404 # Assuming no user found leads to 404 + + @patch("superset.views.users.api.get_user_avatar", return_value=AVATAR_URL) + def test_avatar_with_valid_user(self, mock): + self.login(ADMIN_USERNAME) + + response = self.client.get("/api/v1/user/1/avatar.png") + mock.assert_called_once_with(ADMIN_USERNAME) + assert response.status_code == 301 + assert response.headers["Location"] == self.AVATAR_URL