Skip to content

Commit

Permalink
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
improve code & tests
Browse files Browse the repository at this point in the history
mistercrunch committed Apr 11, 2024

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
1 parent 3df10a3 commit 1162ff7
Showing 8 changed files with 62 additions and 128 deletions.
1 change: 0 additions & 1 deletion .gitignore
Original file line number Diff line number Diff line change
@@ -110,7 +110,6 @@ release.json
messages.mo

docker/requirements-local.txt
docker/.env-local

cache/
docker/*local*
30 changes: 5 additions & 25 deletions docker-compose-image-tag.yml
Original file line number Diff line number Diff line change
@@ -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"
30 changes: 5 additions & 25 deletions docker-compose-non-dev.yml
Original file line number Diff line number Diff line change
@@ -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"
44 changes: 8 additions & 36 deletions docker-compose.yml
Original file line number Diff line number Diff line change
@@ -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
1 change: 1 addition & 0 deletions scripts/tests/run.sh
Original file line number Diff line number Diff line change
@@ -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
23 changes: 2 additions & 21 deletions superset/models/user_attributes.py
Original file line number Diff line number Diff line change
@@ -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
40 changes: 20 additions & 20 deletions superset/views/users/api.py
Original file line number Diff line number Diff line change
@@ -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)
21 changes: 21 additions & 0 deletions tests/integration_tests/users/api_tests.py
Original file line number Diff line number Diff line change
@@ -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

0 comments on commit 1162ff7

Please sign in to comment.