Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: remove standalone #18157

Merged
merged 7 commits into from
Jan 26, 2022
Merged

fix: remove standalone #18157

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
e83a9b8
removed standalone
AAfghahi Jan 25, 2022
3e75c14
Update tests/integration_tests/reports/commands_tests.py
AAfghahi Jan 25, 2022
ca2b881
changed standalone
AAfghahi Jan 25, 2022
602ebba
changed standalone
AAfghahi Jan 25, 2022
565e2c1
added tests
AAfghahi Jan 25, 2022
89d7e58
made function more generic
AAfghahi Jan 26, 2022
8c7e2f8
Update superset/reports/notifications/email.py
AAfghahi Jan 26, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion superset/reports/notifications/email.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
from superset.reports.notifications.base import BaseNotification
from superset.reports.notifications.exceptions import NotificationError
from superset.utils.core import send_email_smtp
from superset.utils.urls import get_screenshot_explorelink

logger = logging.getLogger(__name__)

Expand Down Expand Up @@ -94,6 +95,7 @@ def _get_content(self) -> EmailContent:
html_table = ""

call_to_action = __("Explore in Superset")
url = get_screenshot_explorelink(self._content.url)
img_tags = []
for msgid in images.keys():
img_tags.append(
Expand Down Expand Up @@ -122,7 +124,7 @@ def _get_content(self) -> EmailContent:
</head>
<body>
<p>{description}</p>
<b><a href="{self._content.url}">{call_to_action}</a></b><p></p>
<b><a href="{url}">{call_to_action}</a></b><p></p>
{html_table}
{img_tag}
</body>
Expand Down
11 changes: 10 additions & 1 deletion superset/utils/urls.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
# specific language governing permissions and limitations
# under the License.
import urllib
from typing import Any
from typing import Any, Optional

from flask import current_app, url_for

Expand All @@ -33,3 +33,12 @@ def headless_url(path: str, user_friendly: bool = False) -> str:
def get_url_path(view: str, user_friendly: bool = False, **kwargs: Any) -> str:
with current_app.test_request_context():
return headless_url(url_for(view, **kwargs), user_friendly=user_friendly)


def get_screenshot_explorelink(url: Optional[str]) -> Optional[str]:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a docstring to the function explaining what it does? As for types, it's better to have the signature be (url: str) -> str:, otherwise you need to handle the case where url is None.

One thing I was thinking is that since this function is currently very specific we could make it a bit more generic, so it has utility outside of your use case. Eg:

def modify_url_query(url: str, **kwargs: Any) -> str:
    """
    Replace or add parameters to a URL.
    """
    parts = list(urllib.parse.urlsplit(url))
    params = urllib.parse.parse_qs(parts[3])
    for k, v in kwargs.items():
        if not isinstance(v, list):
            v = [v]
        params[k] = v
    
    parts[3] = "&".join(f"{k}={urllib.parse.quote(v[0])}" for k, v in params.items())
    return urllib.parse.urlunsplit(parts)

Then you can call it for your use case as:

url = modify_url_query(self._content.url, standalone='0')

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok I think that is a cool generic function, thank you! When I had (url: string) -> string I got a type error, and since i was rushing against the cherry deadline I made it optional, will change back and think it through. Thank you!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you had an error with (url: str) -> str it means that in some place the function was potentially being called with a non-string value (probably None, since you fixed it by adding Optional). Since your original function did not handle the case when the argument was not a string, this means that at some point the code would break.

In other words, if your function has a signature (url: Optional[str]) it should be able to handle being passed url=None. But more importantly, the types in the signature should make sense — would we ever want to pass None as a URL to a function that changes parameters in the URL?

The proper way to fix the type checking is not relaxing the signature, but adding guards to the code:

if url is not None:
    explore_url = get_screenshot_explorelink(url)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking of adding an exception into the modify_url_query function, but this feels like it would look better in the email.

Copy link
Member Author

@AAfghahi AAfghahi Jan 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we do end up using the URL in the email body later I was thinking that we could do this:

        url = ""
        if self._content.url is not None:
            url = modify_url_query(self._content.url, standalone="0")

The code before was using self._content.url into the email. so if we were invoking None, would it be the same as invoking an empty string?

data = list(urllib.parse.urlsplit(url))
params = urllib.parse.parse_qs(data[3])
params["standalone"] = ["0"]
data[3] = "&".join(f"{k}={urllib.parse.quote(v[0])}" for k, v in params.items())
url = urllib.parse.urlunsplit(data)
return url
35 changes: 35 additions & 0 deletions tests/unit_tests/utils/urls_tests.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
# -*- coding: utf-8 -*-
# Licensed to the Apache Software Foundation (ASF) under one
# or more contributor license agreements. See the NOTICE file
# distributed with this work for additional information
# regarding copyright ownership. The ASF licenses this file
# to you under the Apache License, Version 2.0 (the
# "License"); you may not use this file except in compliance
# with the License. You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing,
# software distributed under the License is distributed on an
# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
from superset.utils.urls import get_screenshot_explorelink

EXPLORE_CHART_LINK = "http://localhost:9000/superset/explore/?form_data=%7B%22slice_id%22%3A+76%7D&standalone=true&force=false"

EXPLORE_DASHBOARD_LINK = "http://localhost:9000/superset/dashboard/3/?standalone=3"


def test_convert_chart_link() -> None:
test_url = get_screenshot_explorelink(EXPLORE_CHART_LINK)
assert (
test_url
== "http://localhost:9000/superset/explore/?form_data=%7B%22slice_id%22%3A%2076%7D&standalone=0&force=false"
)


def test_convert_dashboard_link() -> None:
test_url = get_screenshot_explorelink(EXPLORE_DASHBOARD_LINK)
assert test_url == "http://localhost:9000/superset/dashboard/3/?standalone=0"