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.

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

fix: remove standalone #18157

Merged
merged 7 commits into from
Jan 26, 2022
Merged

fix: remove standalone #18157

merged 7 commits into from
Jan 26, 2022

Conversation

AAfghahi
Copy link
Member

SUMMARY

Having standalone 3 in the url prevented tabbed dashboards from showing in a report.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@codecov
Copy link

codecov bot commented Jan 25, 2022

Codecov Report

Merging #18157 (e667c12) into master (fa104fe) will decrease coverage by 0.07%.
The diff coverage is 85.09%.

❗ Current head e667c12 differs from pull request most recent head 8c7e2f8. Consider uploading reports for the commit 8c7e2f8 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master   #18157      +/-   ##
==========================================
- Coverage   65.95%   65.87%   -0.08%     
==========================================
  Files        1584     1591       +7     
  Lines       62063    62409     +346     
  Branches     6273     6283      +10     
==========================================
+ Hits        40934    41114     +180     
- Misses      19508    19674     +166     
  Partials     1621     1621              
Flag Coverage Δ
hive ?
mysql 81.28% <92.21%> (+0.09%) ⬆️
postgres 81.32% <92.21%> (+0.09%) ⬆️
presto ?
python 81.41% <92.21%> (-0.26%) ⬇️
sqlite 81.02% <92.21%> (+0.10%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
.../superset-ui-core/src/connection/SupersetClient.ts 100.00% <ø> (ø)
...ackages/superset-ui-core/src/utils/featureFlags.ts 100.00% <ø> (ø)
superset-frontend/src/embedded/index.tsx 0.00% <0.00%> (ø)
...rc/explore/components/controls/TextAreaControl.jsx 80.00% <0.00%> (-4.22%) ⬇️
superset-frontend/src/preamble.ts 0.00% <ø> (ø)
superset-frontend/src/setup/setupClient.ts 0.00% <0.00%> (ø)
superset-frontend/src/views/App.tsx 0.00% <0.00%> (ø)
...perset-frontend/src/views/RootContextProviders.tsx 0.00% <0.00%> (ø)
superset/views/core.py 77.74% <0.00%> (ø)
superset/views/dashboard/views.py 65.16% <46.15%> (-4.07%) ⬇️
... and 38 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fa104fe...8c7e2f8. Read the comment docs.

@AAfghahi
Copy link
Member Author

/testenv up FEATURE_ALERT_REPORTS=True

@github-actions
Copy link
Contributor

@AAfghahi Ephemeral environment spinning up at http://35.88.157.30:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

@AAfghahi AAfghahi force-pushed the ch35037_reportUrl branch 4 times, most recently from b702980 to 43b26ce Compare January 25, 2022 22:11
Copy link
Member

@betodealmeida betodealmeida left a comment

Choose a reason for hiding this comment

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

Looks good! The signature of the function has incorrect types (there should be no Optional), but I also thought of a way of making the function more generic which might be good.

@@ -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

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?

@AAfghahi AAfghahi force-pushed the ch35037_reportUrl branch 2 times, most recently from 2760891 to f0b74be Compare January 26, 2022 16:52
Copy link
Member

@betodealmeida betodealmeida left a comment

Choose a reason for hiding this comment

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

Awesome!

superset/reports/notifications/email.py Outdated Show resolved Hide resolved
@AAfghahi AAfghahi merged commit fa11a97 into apache:master Jan 26, 2022
@github-actions
Copy link
Contributor

Ephemeral environment shutdown and build artifacts deleted.

shcoderAlex pushed a commit to casual-precision/superset that referenced this pull request Feb 7, 2022
* removed standalone

* Update tests/integration_tests/reports/commands_tests.py

* changed standalone

* added tests

* made function more generic

* Update superset/reports/notifications/email.py

Co-authored-by: Beto Dealmeida <[email protected]>

Co-authored-by: Beto Dealmeida <[email protected]>
ofekisr pushed a commit to nielsen-oss/superset that referenced this pull request Feb 8, 2022
* removed standalone

* Update tests/integration_tests/reports/commands_tests.py

* changed standalone

* added tests

* made function more generic

* Update superset/reports/notifications/email.py

Co-authored-by: Beto Dealmeida <[email protected]>

Co-authored-by: Beto Dealmeida <[email protected]>
bwang221 pushed a commit to casual-precision/superset that referenced this pull request Feb 10, 2022
* removed standalone

* Update tests/integration_tests/reports/commands_tests.py

* changed standalone

* added tests

* made function more generic

* Update superset/reports/notifications/email.py

Co-authored-by: Beto Dealmeida <[email protected]>

Co-authored-by: Beto Dealmeida <[email protected]>
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.5.0 labels Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/M 🚢 1.5.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants