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

Overall link situation #30891

Open
paoliniluis opened this issue May 20, 2023 · 18 comments · Fixed by #41508
Open

Overall link situation #30891

paoliniluis opened this issue May 20, 2023 · 18 comments · Fixed by #41508
Assignees
Labels
Embedding/ Use this label when unsure which flavor of embedding is impacted .Team/Embedding Type:Bug Product defects
Milestone

Comments

@paoliniluis
Copy link
Contributor

paoliniluis commented May 20, 2023

Describe the bug

Wanted to make an entire issue regarding links:

Overal situation:
made a dashboard that has:
link cards to an internal dashboard and an internal question
link cards to an absolute url and a relative url
link card to the same Metabase URL of the second card, but as an absolute URL instead of a URL that was generated with autocomplete
a table with absolute and relative urls, and also urls that take you to Metabase resources (both absolute and relative)
image

Situation 1:
go to that dashboard inside Metabase (not embedding)

  • clicking on card link: opens in new tab the card
  • clicking on dashboard link: opens in new tab the dashboard
  • clicking on absolute url: opens in new tab the absolute url
  • clicking on relative url: opens a non-existing resource (localhost:9090/www.google.com doesn't exist)
  • clicking on the absolute URL that takes you to a Metabase resource: same as the second link
  • clicking on the table on absolute url: opens in new tab the absolute url
  • clicking on the table on relative url: opens a non-existing resource (localhost:9090/www.google.com doesn't exist)
  • clicking on the table on the absolute question url: opens in the same tab the question
  • clicking on the table on the absolute dashboard url: opens in the same tab the dashboard
  • clicking on the table on the relative dashboard url: opens in the same tab the dashboard URL but encoding the url, so it will return a 404: /dashboard%2F1-abc

Situation 2 (testing with https://github.com/metabase/sso-examples):
go to that dashboard from within a full app embedding (Metabase runs on the root path)

  • clicking on card link: opens new tab (escapes embedding)
  • clicking on dashboard link: opens new tab (escapes embedding)
  • clicking on absolute url: opens new tab
  • clicking on relative url: opens new tab (escapes embedding), leads to a non-existent resource
  • clicking on the absolute URL that takes you to a Metabase resource: opens new tab (escapes embedding)
  • clicking on the table on absolute url: opens new tab
  • clicking on the table on relative url: opens same tab, leading to a 404
  • clicking on the table on the absolute question url: opens same tab, does not escape embedding
  • clicking on the table on the absolute dashboard url: opens same tab, does not escape embedding
  • clicking on the table on the relative dashboard url: opens in the same tab the dashboard URL but encoding the url, so it will return a 404: /dashboard%2F1-abc, does not escape embedding

Situation 3 (testing with https://github.com/paoliniluis/metabase-subpath):
go to that dashboard inside Metabase which is on a sub-path, but not embedded

  • clicking on card link: opens in new tab
  • clicking on dashboard link: opens in new tab
  • clicking on absolute url: doesn't work, opens Metabase path in a new tab
  • clicking on relative url: opens in a new tab a 404
  • clicking on the absolute URL that takes you to a Metabase resource: opens in a new tab the resource
  • clicking on the table on absolute url: opens in a new tab the absolute url
  • clicking on the table on relative url: opens in the same tab, leading to a 404
  • clicking on the table on the absolute question url: opens in the same tab, goes to the question
  • clicking on the table on the absolute dashboard url: opens in the same tab, goes to the dashboard
  • clicking on the table on the relative dashboard url: opens in the same tab the dashboard URL but encoding the url, so it will return a 404: /dashboard%2F1-abc

Situation 4 (testing with https://github.com/paoliniluis/metabase-subpath) :
go to that dashboard from within a full app embedding but Metabase is on a sub-path
works the same as situation 2

Situation 4:
go to that dashboard from a signed embedding
works the same as situation 5

Situation 5:
go to that dashboard from a public link
works the same as situation 1 and 3, but whenever you hit Metabase it asks for authentication

Situation 6 (a deviation from situation 3):
let's say that you have your app running on app.something.com, and metabase running in app.something.com/mb/
If you put a link to app.something.com, Metabase will open it on the same iframe, because it believes that it's the same application. Cause is #30895

To Reproduce

NA

Expected behavior

NA

Logs

NA

Information about your Metabase installation

1.46.2

Severity

P2

Additional context

NA

@paoliniluis paoliniluis added Type:Bug Product defects .Needs Triage Embedding/ Use this label when unsure which flavor of embedding is impacted and removed .Needs Triage labels May 20, 2023
@paoliniluis
Copy link
Contributor Author

There are 3 things we should get from this:

  1. links that go to the Metabase URL shouldn't open in new tabs when embedded (bug)
  2. the relative links in tables get encoded and fail (bug)
  3. clicking on link cards that have an absolute link in embedding + metabase working on a sub-path fails

@paoliniluis
Copy link
Contributor Author

related to #30851

@paoliniluis
Copy link
Contributor Author

related to #25613

@paoliniluis
Copy link
Contributor Author

related to #30893

@paoliniluis
Copy link
Contributor Author

related to #30894

@paoliniluis
Copy link
Contributor Author

related to #30895

@paoliniluis
Copy link
Contributor Author

related to #15026 and #14009

@paoliniluis paoliniluis changed the title Overal link situation Overall link situation Jun 17, 2023
@likeshumidity
Copy link
Contributor

related to #28269

@likeshumidity
Copy link
Contributor

related to #32013

@WiNloSt
Copy link
Member

WiNloSt commented Dec 4, 2023

The fix #35975 should solve some of the problems here. We should wait until that fix was merged before attempting other fixes here.

@WiNloSt
Copy link
Member

WiNloSt commented Feb 22, 2024

The link logic is a bit all over the place.

  • For link cards, the link logic is determined by LinkViz.tsx
  • For links inside the table, the link logic is determined by value.tsx
  • For click actions, the link logic is determined by dom.js. Here we have a logic to handle subpath + determine whether the absolute URL should be open in the same tab, new tab, with client router or as a link.

The three places should deal with similar kind of links which I link eventually should be consolidated, or at least, under the hood, calls the same utilities so we have a single place that handle links.

@heypoom heypoom self-assigned this Apr 16, 2024
@heypoom
Copy link
Contributor

heypoom commented Apr 16, 2024

As this issue is 10 months old and some issues are partially fixed by @WiNloSt, I will first reproduce the 3 issues indicated above and see which one is still an issue.

@heypoom
Copy link
Contributor

heypoom commented Apr 16, 2024

I've verified that links to Metabase URLs (i.e. questions and dashboards) in link cards still opens in a new tab when embedded. However, links in tables now opens correctly in the same embedded view. I'll create a fix for the link cards so they open in the same view tomorrow.

For the second issue regarding relative links per #30894, it appears that relative links are no longer highlighted in table views. Would this be the expected behaviour @albertoperdomo? If so, we should consider closing #30894 as it no longer applies.

CleanShot 2024-04-16 at 9  27 14@2x

I'll take a look at sub-issue 3 (clicking on link cards that have an absolute link in embedding + metabase working on a sub-path fails) tomorrow.

@albertoperdomo
Copy link
Member

I'll create a fix for the link cards so they open in the same view tomorrow.

Sounds good!

For the second issue regarding relative links per #30894, it appears that relative links are no longer highlighted in table views. Would this be the expected behaviour @albertoperdomo? If so, we should consider closing #30894 as it no longer applies.

I left a comment there after spending a few minutes reproducing. It's still an issue based on current master, but I think someone from DashViz could look at it.

@heypoom
Copy link
Contributor

heypoom commented Apr 17, 2024

Thanks! I created a fix for the first issue. For the third issue, I believe it is already covered and fixed by #32731, but it would be good to double-check and verify this.

@bshepherdson bshepherdson added this to the 0.49.7 milestone Apr 22, 2024
@WiNloSt WiNloSt reopened this Apr 26, 2024
@WiNloSt
Copy link
Member

WiNloSt commented Apr 26, 2024

Was accidentally closed by #41508

@WiNloSt
Copy link
Member

WiNloSt commented Apr 26, 2024

We huddle about this @oisincoveney

@paoliniluis
Copy link
Contributor Author

BTW: this needs to be updated now that there were many fixes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Embedding/ Use this label when unsure which flavor of embedding is impacted .Team/Embedding Type:Bug Product defects
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants