Insight URL cleanup #7201

mariusandra · 2021-11-18T11:00:48Z

Changes

Changes the URLs to look like this
- /insights == urls.savedInsights()
- /insights/new == urls.newInsight() -- redirects after creation to the edit page
- /insights/123 == urls.insightsView(123)
- /insights/123?insight=TRENDS == urls.insightsView(123, { insight: 'TRENDS' })
- /insights/123/edit == urls.insightsEdit(123)
- /insights/123/edit?insight=TRENDS == urls.insightsEdit(123, { insight: 'TRENDS' })
newInsight vs insightNew vs insightsNew? What to use?

For another PR

Update the filters into something like /insights/123#q={insight:'TRENDS'}

How did you test this code?

In the browser, tried to go through all touched code paths.
Also updated the tests to work

mariusandra

This is ready for a look.

mariusandra · 2021-11-18T21:18:13Z

frontend/src/scenes/insights/ActionFilter/renameModalLogic.test.ts

+    let logic: ReturnType<typeof renameModalLogic.build>
+    let relevantEntityFilterLogic: ReturnType<typeof entityFilterLogic.build>


This file is a kind of flyby cleanup 🤷

mariusandra · 2021-11-18T21:19:53Z

frontend/src/scenes/insights/InsightDateFilter/insightDateFilterLogic.ts

@@ -65,7 +65,7 @@ export const insightDateFilterLogic = kea<insightDateFilterLogicType>({
        },
    }),
    urlToAction: ({ actions, values }) => ({
-        '/insights': (_: any, { date_from, date_to }: UrlParams) => {
+        '/insights/': (_: any, { date_from, date_to }: UrlParams) => {


With / in the end to catch /insights/1 and not catch /insights. This will be refactored very soon (the filters shouldn't talk through the url, but with the insight directly), so keeping as a quick fix.

mariusandra · 2021-11-18T21:26:57Z

frontend/src/scenes/insights/insightLogic.tsx

+                    (!loadedFromAnotherLogic && !objectsEqual(cleanSearchParams, values.filters)) ||
+                    insightModeFromUrl !== values.insightMode
+                ) {
+                    actions.setFilters(cleanSearchParams, insightModeFromUrl)


I had to merge setFilters and setInsightMode, since otherwise they would change the URL in multiple steps (go back/forward between e.g. /insights/12 and /insights/23/edit would cause a lot of url noise with first the edit and then the filters being added)

mariusandra · 2021-11-18T21:27:27Z

frontend/src/scenes/paths/pathsLogic.test.ts

+        if (
+            [
+                `api/projects/${MOCK_TEAM_ID}/insights/path`,
+                `api/projects/${MOCK_TEAM_ID}/insights/paths/`,
+                `api/projects/${MOCK_TEAM_ID}/insights/123`,
+                `api/projects/${MOCK_TEAM_ID}/insights`,
+            ].includes(pathname)


Flyby change with the goal of reducing noise in jest tests.

Twixes

Overall looks great! Just a couple of details I think it'd be good to hash out

frontend/src/scenes/urls.ts

frontend/src/scenes/scenes.ts

Twixes · 2021-11-19T13:26:21Z

frontend/src/scenes/saved-insights/savedInsightsLogic.ts

+            if (hashParams.fromItem) {
+                // `fromItem` for legacy /insights url redirect support


This gives us backwards compatibility for fromItem URLs, but those without fromItem don't work anymore. I think it may be worth retaining support for this to avoid frustrating users who have old links. Though that would probably require creating a new insight record every time such a link is accessed (createAndRedirectToNewInsight-style), which is not ideal. Your call, let's just make the decision explicit.

timgl · 2021-11-19T14:25:51Z

Any thoughts on using short_id instead of normal id? Will result in slightly more appealing urls + we don't leak our usage data as obviously

Twixes · 2021-11-22T16:01:20Z

As for the last commit, that's fixed in master now (just removed the extra table implementation-specific class, th is enough)

Twixes

There's just one thing: "Save as new insight" results in a new insight that's a blank slate (this is also the case on #7259). I looked into it for a bit but not sure why.

mariusandra · 2021-11-24T18:51:42Z

Hmm... seems to work for me, possibly merging with master fixed it?

mariusandra · 2021-11-25T13:47:49Z

I didn't understand the issue initially. This is probably the fix you wanted to see:

@Twixes anything else blocking here?

Twixes

Yup! All is well now

* convert a bunch of things to user short_id instead of id * fix more TS errors * fix test * fix jest tests * various fixes * add wise words * type InsightShortId-s to make life easier * reduce a bit of test noise * use the InsightShortId in the URL * fix type * fix test * fix insight url preloading * pass dive dashboards as having insight short ids * fix short url redirect * mock scenelogic api * better types and tests * type fixes * fix bug of linking to ourselves * add back "id" * get rid of some "getInsightId" calls * two more * few more * refactor last usage of getInsightId * move files around and improve errors * make it simpler * small fixes * redirect to new url from old hashParam=42 * fix regression * alert the user if we could not find an insight with the old ID format * switch to a simpler scene * fix another test * Fix annotation creation * Make short ID friendlier * remove comments * simplify insight links from dashboards and saved insights * remove insight router * fix TS * Revert "remove insight router" This reverts commit e52f474. Co-authored-by: Michael Matloka <dev@twixes.com>

mariusandra added 2 commits November 18, 2021 11:58

insight route refactor, part 1

1e46c5a

add fromItem to get redirects

0923905

mariusandra mentioned this pull request Nov 18, 2021

Epic - Nail Seamless Analytics Experience #6513

Closed

mariusandra added 15 commits November 18, 2021 13:46

fix some tests

e628ea1

adjust many more paths

09e1b1e

fix test

6ea8afb

move new insight creation into insight logic

84d9a91

fix a noisy test

02608d8

simplify one test

6c20554

open the url with the right filters

f32128b

null fix

c9407e4

fix some more noisy tests

9bccaf0

move saved insights to /insights, fix logic tests

ca63825

fix cypress urls

ebaa346

fix some tests

a294b40

fix even more insight urls

0f6c0a7

wait a bit longer

85110b6

add old saved_insights redirect

2a47ae0

mariusandra marked this pull request as ready for review November 18, 2021 23:03

mariusandra requested a review from Twixes November 18, 2021 23:03

mariusandra commented Nov 18, 2021

View reviewed changes

mariusandra and others added 3 commits November 19, 2021 11:02

Merge branch 'master' into insight-urls-one-more-time

c9965f9

this might not be there yet

e5db7e3

Merge branch 'master' into insight-urls-one-more-time

e6be27c

Twixes mentioned this pull request Nov 19, 2021

Fix insight urls #6918

Closed

Twixes reviewed Nov 19, 2021

View reviewed changes

Twixes mentioned this pull request Nov 19, 2021

Reorganize insight logics #4293

Closed

mariusandra added 2 commits November 19, 2021 16:31

rename newInsight -> insightNew

b7296ca

rename Scene.Insights -> Scene.Insight

b384c37

mariusandra added 7 commits November 22, 2021 10:52

Merge branch 'master' into insight-urls-one-more-time

237ca61

Merge branch 'master' into insight-urls-one-more-time

080b035

fix TS merge bugs

b2fbfc8

fix import

c33f9aa

fix imports

94749b3

fix tests

76b5d91

fix test

46bc51b

Twixes and others added 9 commits November 22, 2021 17:56

Merge branch 'master' into insight-urls-one-more-time

817aa6f

Merge branch 'master' into insight-urls-one-more-time

4bae9b5

Run prettier

e26802d

Merge branch 'master' into insight-urls-one-more-time

51dbde4

Merge branch 'master' into insight-urls-one-more-time

f00c491

fix changes after merge

d47b11b

Merge branch 'master' into insight-urls-one-more-time

fb304f0

switch to a simpler scene

c3ebe1f

fix another test

7bcdfe1

Twixes requested changes Nov 24, 2021

View reviewed changes

Merge branch 'master' into insight-urls-one-more-time

1b8c8dd

mariusandra requested a review from Twixes November 24, 2021 18:56

mariusandra added 2 commits November 25, 2021 14:45

fix "save as" reset

e73b3c1

Merge branch 'master' into insight-urls-one-more-time

a6d210b

mariusandra force-pushed the insight-urls-one-more-time branch from 0719343 to a6d210b Compare November 25, 2021 14:03

rerun tests

7ae3ced

Twixes approved these changes Nov 25, 2021

View reviewed changes

mariusandra merged commit fbba787 into master Nov 25, 2021

mariusandra deleted the insight-urls-one-more-time branch November 25, 2021 14:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Insight URL cleanup #7201

Insight URL cleanup #7201

mariusandra commented Nov 18, 2021 •

edited

mariusandra left a comment

mariusandra Nov 18, 2021

mariusandra Nov 18, 2021

mariusandra Nov 18, 2021

mariusandra Nov 18, 2021

Twixes left a comment

Twixes Nov 19, 2021 •

edited

mariusandra Nov 19, 2021

timgl commented Nov 19, 2021

Twixes commented Nov 22, 2021

Twixes left a comment

mariusandra commented Nov 24, 2021

mariusandra commented Nov 25, 2021

Twixes left a comment

		let logic: ReturnType<typeof renameModalLogic.build>
		let relevantEntityFilterLogic: ReturnType<typeof entityFilterLogic.build>

		if (hashParams.fromItem) {
		// `fromItem` for legacy /insights url redirect support

Insight URL cleanup #7201

Insight URL cleanup #7201

Conversation

mariusandra commented Nov 18, 2021 • edited

Changes

For another PR

How did you test this code?

mariusandra left a comment

Choose a reason for hiding this comment

mariusandra Nov 18, 2021

Choose a reason for hiding this comment

mariusandra Nov 18, 2021

Choose a reason for hiding this comment

mariusandra Nov 18, 2021

Choose a reason for hiding this comment

mariusandra Nov 18, 2021

Choose a reason for hiding this comment

Twixes left a comment

Choose a reason for hiding this comment

Twixes Nov 19, 2021 • edited

Choose a reason for hiding this comment

mariusandra Nov 19, 2021

Choose a reason for hiding this comment

timgl commented Nov 19, 2021

Twixes commented Nov 22, 2021

Twixes left a comment

Choose a reason for hiding this comment

mariusandra commented Nov 24, 2021

mariusandra commented Nov 25, 2021

Twixes left a comment

Choose a reason for hiding this comment

mariusandra commented Nov 18, 2021 •

edited

Twixes Nov 19, 2021 •

edited