Skip to content
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

DT-617: Hide spinner after loading failure #2704

Closed
wants to merge 1 commit into from

Conversation

rushtong
Copy link
Contributor

Addresses

https://broadworkbench.atlassian.net/browse/DT-617

Summary

If the dataset search endpoint fails, this PR closes out the loading spinner.

To test, I started up a local Consent (without a local ES instance) not on the VPN so that it would error out when querying ES. Then point local UI to local Consent and visit the data library page. The loading spinner is closed.

Something I did not do was to percolate the error down to the table itself. The table just sees an empty list of datasets and thinks there are none for the current library filter. This could be expanded if we would like to see more info in that condition, but since this is a fairly infrequent use case, I didn't pursue this.


Have you read Terra's Contributing Guide lately? If not, do that first.

  • Label PR with a Jira ticket number and include a link to the ticket
  • Label PR with a security risk modifier [no, low, medium, high]
  • PR describes scope of changes
  • Get a minimum of one thumbs worth of review, preferably two if enough team members are available
  • Get PO sign-off for all non-trivial UI or workflow changes
  • Verify all tests go green
  • Test this change deployed correctly and works on dev environment after deployment

@rushtong rushtong requested a review from a team as a code owner October 31, 2024 14:53
@rushtong rushtong requested review from pshapiro4broad and fboulnois and removed request for a team October 31, 2024 14:53
Copy link
Contributor

@fboulnois fboulnois left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See below:

@@ -298,6 +298,7 @@ export const DatasetSearch = (props) => {
});
} catch (error) {
Notifications.showError({ text: 'Failed to load Elasticsearch index' });
setLoading(false);
Copy link
Contributor

@fboulnois fboulnois Oct 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is desirable, since it shows an empty table which may be misleading when on a custom data library. This means that we will not be able to tell the difference between a data table that has no data vs a data library that failed to load.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is true - the table doesn't know why there are no datasets, but it's also technically true that loading is complete. I noted in the PR description that I'm purposefully choosing not to try and modify the logic in the display to include an error state. If we were to go down that path, this would a larger refactor that I would prefer to do in a ticket with clear requirements in that case.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a user I cannot take an action if the index fails to load, that's why I don't think it's desirable to go from a loading state (where it is clear that I cannot take an action) to an empty table (where it is unclear if I can take any actions)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a user I cannot take an action if the index fails to load, that's why I don't think it's desirable to go from a loading state (where it is clear that I cannot take an action) to an empty table (where it is unclear if I can take any actions)

IIUC, is having the loading spinner remain visible in the error state the intended functionality? If so, I'll be happy to close this and close out the ticket.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Until we have a more defined approach to dealing with index failures (perhaps replacing the spinner with an error icon?), the spinner-as-error was the intended failure mode.

@rushtong rushtong closed this Oct 31, 2024
@rushtong rushtong deleted the gr-DT-617-fix-spinner branch October 31, 2024 17:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants