EES-4368 api data set preview token log #5125
Conversation
| id: 'token-id-1', | ||
| label: 'Test label 1', | ||
| status: 'Active', | ||
| created: new Date('01-01-2024 14:00').toISOString(), |
There was a problem hiding this comment.
Would always declare dates with the standard ISO8601 format to avoid any ambiguity and be consistent with the rest of the codebase.
Also, constructing a Date to turn it back into an ISO8601 string is redundant. It'd be better to just omit this and write the ISO8601 string directly i.e. 2024-01-01T14:00Z.
Same applies to dates below.
There was a problem hiding this comment.
Sorted, don't know why I did that 😆
| to={generatePath<ReleaseDataSetPreviewTokenRouteParams>( | ||
| releaseApiDataSetPreviewTokenRoute.path, | ||
| { | ||
| publicationId, | ||
| releaseId, | ||
| dataSetId, | ||
| previewTokenId: token.id, | ||
| }, | ||
| )} |
There was a problem hiding this comment.
One issue here is that because we're also redirecting to this route after the user generates a new token, the breadcrumb doesn't quite make sense in both contexts:
If the user has come from the preview page (after generating the token), 'Go back to API data set details' makes sense.
If the user has come from the preview token log, 'Go back to API preview token log' would make more sense.
We could fix this by one of the following:
- Passing extra location state via the
toprop and / orhistory.push. This state can then be accessed viauseLocationin the preview token page. - Using the
useLastLocationhook on preview token page to check which path the user has come from. - Changing the breadcrumb to always go back to the token log.
Option 3 is the simplest and may be sufficient, but feel free to consider the other options as well.
There was a problem hiding this comment.
I've used useLastLocation to check the previous page and set the back link appropriately.
| const history = createMemoryHistory(); | ||
|
|
||
| history.push( | ||
| generatePath<ReleaseDataSetRouteParams>( | ||
| releaseApiDataSetPreviewTokenLogRoute.path, | ||
| { | ||
| publicationId: testRelease.publicationId, | ||
| releaseId: testRelease.id, | ||
| dataSetId: 'data-set-id', | ||
| }, | ||
| ), | ||
| ); |
There was a problem hiding this comment.
Not a biggy, but if we're not planning on checking the history, we could just use MemoryRouter with this route as an initial entry?
|
|
||
| expect(previewTokenService.revokePreviewToken).toHaveBeenCalledWith( | ||
| 'token-id-1', | ||
| ); |
There was a problem hiding this comment.
Shouldn't we check that the table has been updated with the revoked token?
| <ButtonText className="govuk-!-margin-left-2"> | ||
| Revoke | ||
| <VisuallyHidden> {token.label}</VisuallyHidden> | ||
| </ButtonText> |
There was a problem hiding this comment.
Similar to comment above - there's a revoke token button on the preview token page. Upon revoking the token through this button, the user gets redirected to a page that might not necessarily make sense.
If the user has come from the preview page (after generating a token), then redirecting back to the preview page upon revoking the token makes sense.
If the user has come from the token log, then redirecting to the token log upon revoking the token makes more sense.
We'll want to consider a similar approach to the above comment to change this behaviour based on where the user has come from.
There was a problem hiding this comment.
I've used useLastLocation to check the previous page and redirect appropriately after revoking.
| to={generatePath<ReleaseDataSetRouteParams>( | ||
| releaseApiDataSetPreviewTokenLogRoute.path, | ||
| { | ||
| publicationId, | ||
| releaseId, | ||
| dataSetId, | ||
| }, | ||
| )} |
There was a problem hiding this comment.
Could use the tokenLogPagePath variable here?
There was a problem hiding this comment.
yep, done now cheers.
Adds the preview token log page.