ref(groupingInfo): Refactor grouping info to avoid storing redundant data #101135
Conversation
github-actions
bot
added
the
Scope: Frontend
❌ 1 Tests Failed:
View the top 1 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
shayna-ch
changed the title
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
lobsterkatie
left a comment
lobsterkatie
left a comment
There was a problem hiding this comment.
LGTM! I assume you've tested sending both types of payloads?
| const grouping_config = old | ||
| ? ( | ||
| Object.values(old).find( | ||
| variant => 'config' in variant && variant.config?.id | ||
| ) as any | ||
| )?.config?.id | ||
| : null; | ||
| return old | ||
| ? { | ||
| grouping_config, | ||
| variants: old, | ||
| } | ||
| : null; |
There was a problem hiding this comment.
Nit, totally not worth changing in this temporary function, but just an option to keep in mind. If you do an early out (if (!old), {return null;} as your first statement), then it'll save you from having to nest so deeply in the rest of the function, which will in turn make it easier to read.
| function isOld( | ||
| data: EventGroupingInfoResponseOld | EventGroupingInfoResponse | null | ||
| ): boolean { |
There was a problem hiding this comment.
Again, more important in principle than in practice since this is all temporary, but if you make this a type predicate, then you won't have to do the casting in your groupInfoNew value below.
| event: Event; | ||
| group: Group | undefined; | ||
| }): EventGroupingInfoResponse | null { | ||
| }): EventGroupingInfoResponseOld | EventGroupingInfoResponse | null { |
There was a problem hiding this comment.
If you add groupingConfig: null to the return here, I think bugbot will stop yelling at you.
shayna-ch
force-pushed
the
shayna-ch/groupinginfo-fe-restructure-1
branch
from
cf9cf47 to
bf44c8c
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| expect(screen.getByText('123')).toBeInTheDocument(); | ||
| // Should not make grouping-info request | ||
| expect(groupingInfoRequest).not.toHaveBeenCalled(); | ||
| }); |
There was a problem hiding this comment.
Bug: Unnecessary API Mock in Local Data Test
The test "gets performance new grouping info from group/event data" mocks an API response for grouping info, then asserts the API call isn't made. Since performance grouping data is derived locally, this mock is unnecessary and creates a confusing contradiction in the test.
|
PR reverted: aca83d5 |
…dundant data (#101135)" This reverts commit ff9e466. Co-authored-by: shayna-ch <[email protected]>
…dundant data (#101135)" This reverts commit ff9e466. Co-authored-by: shayna-ch <[email protected]>
…dundant data (#101135)" This reverts commit ff9e466. Co-authored-by: shayna-ch <[email protected]>
First PR in a series to refactor grouping info data structure (see linear issue).
Make front end accept and implement new grouping info data structure with
grouping_configpulled out of individual grouping variants. Now the front end can accept either the correct newEventGroupingInfoResponseor the oldEventGroupingInfoResponseOldwhich it will convert into the correctEventGroupingInfoResponsedata type.