improve code & tests

.gitignore

@@ -110,7 +110,6 @@ release.json
 messages.mo
 
 docker/requirements-local.txt
-docker/.env-local
 
 cache/
 docker/*local*

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"

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"

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

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

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

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)

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