-
Notifications
You must be signed in to change notification settings - Fork 5
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
EES-5564 - added error handling when fetching data set details or changelog results #5413
EES-5564 - added error handling when fetching data set details or changelog results #5413
Conversation
@@ -28,11 +29,14 @@ export default function ReleaseApiDataSetChangelogPage() { | |||
data: dataSet, | |||
isLoading: isLoadingDataSet, | |||
refetch: refetchDataSet, | |||
isError: errorFetchingDataSet, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't necessary as we can infer this when dataSet
is undefined and loading has completed. In the error state, dataSet
is not defined.
See later comment on why this check is beneficial type-wise as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No worries, will do. As someone who was not so familiar with the front-end codebase conventions that we have, I found the error flag as being very explicit in what it's conveying over and above a null check, but I see the advantages you're getting at.
const { | ||
data: changes, | ||
isLoading: isLoadingChanges, | ||
isError: errorFetchingChanges, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As above - we can infer this when changes
is undefined and loading has completed. In the error state, changes
is not defined.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done ta
<h2>{dataSet?.title}</h2> | ||
</div> | ||
</div> | ||
{!errorFetchingDataSet && !errorFetchingChanges ? ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As the requests are independent, there's no need to couple both into the same conditional like this.
We could just check that only dataSet
is defined (see earlier comments about not need to use isError
) and render as much UI as possible before showing the error message for just the section relating to the changelog specifically.
Specifically would be looking for the following:
{dataSet ? (
...
) : (
<WarningMessage>Could not load API data set</WarningMessage>
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done ta
<div className="govuk-grid-row"> | ||
<div className="govuk-grid-column-three-quarters"> | ||
<span className="govuk-caption-l">API data set changelog</span> | ||
<h2>{dataSet?.title}</h2> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A benefit of checking that dataSet
is defined (after checking loading has finished) is that the TS compiler can also treat the variable as defined and we don't need to use optional chaining here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've also added dataSetVersion
as part of the check, as this must be defined if `dataSet is defined, unlesss something has gone disastrously wrong! We can now remove a bunch of null-checking for dataSetVersion as well.
{changes && dataSetVersion && ( | ||
<ApiDataSetChangelog | ||
majorChanges={changes.majorChanges} | ||
minorChanges={changes.minorChanges} | ||
version={dataSetVersion.version} | ||
/> | ||
)} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would move the 'Could not load changelog' error message to be part of this conditional
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done ta
expect(screen.queryByText('Data set title')).not.toBeInTheDocument(); | ||
|
||
expect( | ||
await screen.findByText('Could not load changelog'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As per other comments - this would be 'Could not load API data set'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done ta
… null checks after loading is complete. Made display of overarching data set details independent from whether or not the changelog is fetched successfully.
6475fcd
to
549a296
Compare
This PR:
Could not load methodology
.