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

update msg to be more friendly in farms page #1703

Closed
wants to merge 7 commits into from

Conversation

AlaaElattar
Copy link
Contributor

@AlaaElattar AlaaElattar commented Dec 14, 2023

Description

  • Update error msg when there's problem getting user farms.

Changes

Error msg updated to Failed to get farms. Please check your connection.

Related Issues

#1599

Checklist

  • Tests included
  • Build pass
  • Documentation
  • Code format and docstrings
  • Screenshots/Video attached (needed for UI changes)

@MohamedElmdary
Copy link
Member

I think we should use normalizeError method and that string should be the fallback error
because now if any error happen user won't know @AlaaElattar

@AlaaElattar
Copy link
Contributor Author

I think we should use normalizeError method and that string should be the fallback error because now if any error happen user won't know @AlaaElattar

yes, u're right.

@AlaaElattar AlaaElattar marked this pull request as draft December 14, 2023 15:51
@AlaaElattar AlaaElattar marked this pull request as ready for review December 17, 2023 11:17
@AlaaElattar
Copy link
Contributor Author

I think we should use normalizeError method and that string should be the fallback error because now if any error happen user won't know @AlaaElattar

should be fixed in 29e54d8

Copy link
Contributor

@amiraabouhadid amiraabouhadid left a comment

Choose a reason for hiding this comment

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

I got this error msg instead
Screenshot from 2023-12-17 14-06-49
Screenshot from 2023-12-17 14-06-09

@AlaaElattar
Copy link
Contributor Author

I got this error msg instead Screenshot from 2023-12-17 14-06-49 Screenshot from 2023-12-17 14-06-09

Fixed. will always show friendly error msg.

Copy link
Contributor

@zaelgohary zaelgohary left a comment

Choose a reason for hiding this comment

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

Are we certain that a connection error is the only potential issue? What if there are problems with the Grid Proxy itself? Shouldn't the error message be more inclusive or at least address other possible scenarios beyond network issues?

@AlaaElattar
Copy link
Contributor Author

Are we certain that a connection error is the only potential issue? What if there are problems with the Grid Proxy itself? Shouldn't the error message be more inclusive or at least address other possible scenarios beyond network issues?

  • Here i tried if i can't reach proxy and i got same logs. Do u have an idea how to differentiate the connection error from any other error ?

image

@0oM4R
Copy link
Contributor

0oM4R commented Dec 18, 2023

Are we certain that a connection error is the only potential issue? What if there are problems with the Grid Proxy itself? Shouldn't the error message be more inclusive or at least address other possible scenarios beyond network issues?

  • Here i tried if i can't reach proxy and i got same logs. Do u have an idea how to differentiate the connection error from any other error ?

image

@AlaaElattar
you can check on the error type, all errors should be an instance of a class on types package

example:

  • import the type you want to check on here i will import validationError

then check on the catch error
if (error instanceof ValidationError)

@AlaaElattar AlaaElattar marked this pull request as draft December 18, 2023 13:54
@AlaaElattar AlaaElattar marked this pull request as ready for review December 19, 2023 14:46
@AlaaElattar
Copy link
Contributor Author

Are we certain that a connection error is the only potential issue? What if there are problems with the Grid Proxy itself? Shouldn't the error message be more inclusive or at least address other possible scenarios beyond network issues?

  • Here i tried if i can't reach proxy and i got same logs. Do u have an idea how to differentiate the connection error from any other error ?

image

@AlaaElattar you can check on the error type, all errors should be an instance of a class on types package

example:

  • import the type you want to check on here i will import validationError

then check on the catch error if (error instanceof ValidationError)

Done. I checked on error, there're 2 possibilities either connection error or error from gridproxy.

@amiraabouhadid
Copy link
Contributor

found this issue while testing #1733

if (error instanceof ConnectionError) {
createCustomToast("Failed to get farms. Please check your connection.", ToastType.danger);
} else {
createCustomToast("Failed to get farms.", ToastType.danger);
Copy link
Contributor

@amiraabouhadid amiraabouhadid Dec 19, 2023

Choose a reason for hiding this comment

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

i got this . can you please add "Please check your connection. " here too? you can also just save the toast in a variable and reuse it instead of repeating code
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If i added same msg , what is the point then of checking on error type ?

Copy link
Member

Choose a reason for hiding this comment

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

This issue should be fixed here #1735 @AlaaElattar @amiraabouhadid

Copy link
Contributor

Choose a reason for hiding this comment

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

@amiraabouhadid Another error type not connection related might happen in the else, so I think it's better this way.

Copy link
Contributor

Choose a reason for hiding this comment

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

I checked #1735 and it indeed fixes the issue in case no connection is needed.

zaelgohary
zaelgohary previously approved these changes Dec 20, 2023
@zaelgohary zaelgohary dismissed their stale review December 20, 2023 15:06

Need changes after #1735 is merged

@AlaaElattar
Copy link
Contributor Author

@AlaaElattar AlaaElattar deleted the development_rephrase_error_farms branch December 20, 2023 15:20
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.

5 participants