Insight Short URLs #7259
Insight Short URLs #7259
Conversation
mariusandra
changed the title
frontend/src/lib/api.mock.ts
Outdated
| 'api/projects/@current/event_definitions/', | ||
| ].includes(pathname) | ||
| ) { | ||
| return { results: [] } | ||
| return { results: [], next: null } |
There was a problem hiding this comment.
This reduces some noise in tests, didn't seem to break or fix anything else.
| pageKey: dashboardItemId ? dashboardItemId : null, | ||
| }) | ||
| ) | ||
| const { diffType, groupedAnnotations } = useValues(annotationsLogic({ insightShortId: dashboardItemId })) |
There was a problem hiding this comment.
annotationsLogic's pageKey was only being used by dashboardItemIds, so let's call a spade a spade.
| @@ -213,32 +215,32 @@ export const dashboardLogic = kea<dashboardLogicType<DashboardLogicProps>>({ | |||
| ], | |||
| refreshStatus: [ | |||
| {} as Record< | |||
| number, | |||
| string, | |||
There was a problem hiding this comment.
TypeScript Records always are <string, something_else>, putting InsightShortId as the type of the Record's key here caused Object.values to not return the right type for the values. 🤷♂️
I also considered leaving it as number and using insight.id as the object's key, but that didn't seem suitable considering all the other refactors to shortId where possible...
| // Creating a nominal type: https://github.com/microsoft/TypeScript/issues/202#issuecomment-961853101 | ||
| export type InsightShortId = string & { readonly '': unique symbol } |
| const savedInsight: DashboardItemType = await api.update( | ||
| `api/projects/${teamLogic.values.currentTeamId}/insights/${insightId}`, | ||
| { ...values.insight, saved: true } |
There was a problem hiding this comment.
It would be nice if we don't have to make (at worst) two blocking api calls while trying to save an insight. I think it makes more sense that resolving the short id into an insight id happens all in the backend under a single POST call.
There was a problem hiding this comment.
99.99% of the time here, we have the insight.id and all is good. The async call is never made. The only time we have insightLogic with loaded data and there's no insight.id, is if we show the insight on a dashboard, and pre-populate its data with props.dashboardItemId, props.cachedResults and props.filters. I'd like to refactor those three to just one field insight that also includes the id field, but I fear this will make the PR even bigger as it's a sizeable refactor on its own. Let's defer it for after the PR please.
|
@Twixes this should be reviewable as well. Trying to get the final tests to go green, which should have happened now... |
* insight route refactor, part 1 * add fromItem to get redirects * fix some tests * adjust many more paths * fix test * move new insight creation into insight logic * fix a noisy test * simplify one test * open the url with the right filters * null fix * fix some more noisy tests * move saved insights to `/insights`, fix logic tests * fix cypress urls * fix some tests * fix even more insight urls * wait a bit longer * add old saved_insights redirect * this might not be there yet * rename newInsight -> insightNew * rename Scene.Insights -> Scene.Insight * also redirect old searches without fromItem * fix link * fix TS merge bugs * fix import * fix imports * fix tests * fix test * Run prettier * fix changes after merge * switch to a simpler scene * fix another test * fix "save as" reset * rerun tests * Insight Short URLs (#7259) * 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> Co-authored-by: Michael Matloka <dev@twixes.com>
Changes
short_idin the URLs and elsewhere else it made senseinsight.idfield for making API requestsInsightShortId, which is now the "primary key" for insights, as far as any frontend logic is concerned.dashboardItemId,insightId,insightShortId,shortId,idand any of the various forms that you might see around. That's for another PR.insightEditandinsightNewurls to use the short_idinsightLogicto work that way as well.How did you test this code?