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

Handle tables and queries API errors on the schema page #2644

Closed
rajatvijay opened this issue Mar 7, 2023 · 23 comments · Fixed by #3323
Closed

Handle tables and queries API errors on the schema page #2644

rajatvijay opened this issue Mar 7, 2023 · 23 comments · Fixed by #3323
Labels
affects: ux Related to user experience good first issue Everything in "Help wanted", PLUS being relatively easy and straightforward to implement. help wanted Community contributors can implement this ready Ready for implementation type: bug Something isn't working work: frontend Related to frontend code in the mathesar_ui directory
Milestone

Comments

@rajatvijay
Copy link
Contributor

rajatvijay commented Mar 7, 2023

Description

The schema page directly relies on two API calls

  1. Fetching tables
  2. Fetching queries(explorations)

Current behavior

When either one of the APIs fails the page assumes a success state with no results hence showing the button either create a new table or go to the data explorer to create a new exploration.
Screenshot 2023-03-07 at 8 00 55 AM

Expected behavior

When either one of the APIs fails the page should show an error state with a button to retry the failed API call and a button to go back to the database page.

@rajatvijay rajatvijay added type: bug Something isn't working good first issue Everything in "Help wanted", PLUS being relatively easy and straightforward to implement. affects: ux Related to user experience work: frontend Related to frontend code in the mathesar_ui directory ready Ready for implementation labels Mar 7, 2023
@rajatvijay
Copy link
Contributor Author

rajatvijay commented Mar 7, 2023

@kgodey @ghislaineguerin tagging you just in case you want to have a proper design for this error state.

@pavish
Copy link
Member

pavish commented Mar 7, 2023

@rajatvijay

When either one of the APIs fails the page should show an error state with a button to go back to the database page.

I think it'd be better to have a retry button along with the error instead of a go to database page button.

@MANISH-LAB
Copy link

MANISH-LAB commented Mar 7, 2023

assign me this issue @rajatvijay @pavish , i would love to contribute to it.

@Anish9901
Copy link
Member

Go ahead @MANISH-LAB.

@rajatvijay
Copy link
Contributor Author

@pavish that was my initial thought too. But then shouldn't these apis have default retry mechanism for let's say 3 times in built?

@pavish
Copy link
Member

pavish commented Mar 8, 2023

@rajatvijay That's a backend concern (and as far as I know, we don't do anything of that sort) and I don't think we should account for it in the UX. I don't see any specific advantage in having a button move to database page, but I do see an advantage in a retry button, because it lets the user take an action to recover the error state.

@faraz16iqbal
Copy link

@rajatvijay should be a fairly simple check to check for data from both API calls right? But what should be the desired behavior if the request fails even after retrying? We might also want to rate limit the retry calls to our backend. What do you think?

@rajatvijay
Copy link
Contributor Author

@faraz16iqbal

But what should be the desired behavior if the request fails even after retrying?

This is the main reason for having the go-to database page button too. I have already added this in the Expected section of the issue description.

We might also want to rate limit the retry calls to our backend. What do you think?

I do not think that we should bother ourselves with this since this is similar to the user coming back to this page again and again which ends up making the same no of API calls.

@rajatvijay
Copy link
Contributor Author

Unassigning due to in-activity

@rajatvijay rajatvijay added the help wanted Community contributors can implement this label Mar 21, 2023
@rajatvijay rajatvijay added this to the Backlog milestone Mar 21, 2023
@monstajoe2002
Copy link

monstajoe2002 commented Mar 25, 2023

Can I be assigned on this issue?

@monstajoe2002
Copy link

Do you need a refresh button that makes a GET request?

@rajatvijay
Copy link
Contributor Author

@monstajoe2002

Can I be assigned on this issue?

No, but you can start working on this. See #1490 (comment)

Do you need a refresh button that makes a GET request?

Please see the "Expected behaviour" section of the issue description.

@monstajoe2002
Copy link

How do I trigger 504 error?

@Aritra8438
Copy link
Member

Aritra8438 commented Apr 7, 2023

How do I trigger 504 error?

You can send a response with status code 504 from the backend.

Also, @rajatvijay, if none is working on the issue, can I be assigned?

@rajatvijay
Copy link
Contributor Author

@Aritra8438 assigning this to you due to in activity from earlier contributor

@Aritra8438
Copy link
Member

Hi, @rajatvijay, as you have already mentioned, the error state should have a proper design.

While submitting the PR, I used a crude template to get things done. After the design, I will modify the PR accordingly.

Screenshot 2023-04-20 005210

@rajatvijay
Copy link
Contributor Author

@ghislaineguerin assigning this to you to provide proper designs for this.

@ghislaineguerin
Copy link
Contributor

@rajatvijay The design is already added in #2829

@ghislaineguerin ghislaineguerin removed their assignment Jun 30, 2023
@seancolsen
Copy link
Contributor

Unassigning due to inactivity

@Tweniee
Copy link

Tweniee commented Nov 17, 2023

can this be assigned to me?

@seancolsen
Copy link
Contributor

@Tweniee You're welcome to work on this and submit a PR, but in general we don't assign tickets to new contributors, as noted in our Contributor Guide. Once we've already merged a PR of yours, then we'll be happy to assign tickets to you after that. The reason for this policy is that in the past it has been a maintenance burden for us to manage ticket assignments while having an overwhelming volume of people fail to submit their first PR when initially expressing intent to contribute.

@nikhilhenry
Copy link
Contributor

Hi, I have created PR #3323 to address this issue.

Could this issue please be assigned to me?

Thanks :)

@pavish
Copy link
Member

pavish commented Nov 27, 2023

@nikhilhenry As noted in our Contributor Guide and in the previous comment by Sean, we assign new tickets to contributors after one of their PRs is merged.

Thanks for the PR #3323. After we've reviewed and merged it, if you wish to work on other open issues, we'd happily assign them to you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects: ux Related to user experience good first issue Everything in "Help wanted", PLUS being relatively easy and straightforward to implement. help wanted Community contributors can implement this ready Ready for implementation type: bug Something isn't working work: frontend Related to frontend code in the mathesar_ui directory
Projects
No open projects
Development

Successfully merging a pull request may close this issue.