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.

Sign up for GitHub

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

Jump to bottom

fix : app crashes on sign up because of a server error #1176

Closed
wants to merge 1 commit into from
Closed

fix : app crashes on sign up because of a server error #1176

wants to merge 1 commit into from

Conversation

yveskalume
Copy link

@yveskalume yveskalume commented Nov 17, 2021 •
edited
Loading

Description

Fixes #1175

Type of Change:

  • Code

Code/Quality Assurance Only

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

open the app > go to Sign Up screen > complete the informations > click on Sign Up button > and see the error message without the app crashing

Checklist:

  • My PR follows the style guidelines of this project
  • I have performed a self-review of my own code or materials

vj-codes
vj-codes approved these changes Nov 23, 2021
View reviewed changes
Copy link
Member

@vj-codes vj-codes left a comment

Choose a reason for hiding this comment

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

The changes made in this PR were tested
locally. Following are the results:

  1. Code review - Done
  2. All possible responses (positive and negative tests) were tested as below:

Before : No error message was displayed and the app crashed

After : An error message of "Something went wrong" is displayed

Status of PR : After one more approving review, it will be ready to merge!

Screenshot_2021-11-24-01-17-49-937_org anitab mentorship

@vj-codes vj-codes requested a review from epicadk November 23, 2021 20:04
@vj-codes vj-codes added the Status: Needs Review PR needs an additional review or a maintainer's review. label Nov 23, 2021
epicadk
epicadk suggested changes Nov 24, 2021
View reviewed changes
app/src/main/java/org/anitab/mentorship/remote/datamanager/AuthDataManager.kt
@@ -34,6 +34,10 @@ class AuthDataManager(private val dispatcher: CoroutineDispatcher = Dispatchers.
* @return an Observable CustomResponse
*/
suspend fun register(register: Register): CustomResponse {
return withContext(dispatcher) { apiManager.authService.register(register) }
try {
Copy link
Member

Choose a reason for hiding this comment

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

Revert and make changes in CommonUtils.getErrorResponse because according to the log it's a GSON issue.

Copy link
Author

@yveskalume yveskalume Nov 30, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

the error message is "com.google.gson.JsonSyntaxException: java.lang.IllegalStateException: Expected BEGIN_OBJECT but was STRING at line 1 column 1 path $"

it seems like JSON string which return error message has not the correct structure to be parsed into an instance of CustomResponse

Gson is expecting JSON string to begin with an object opening brace. e.g.

{
But the string you have passed to it starts with an open quotes
"

This means that my PR is necessary to avoid this bug due to a server error.

Copy link
Member

Choose a reason for hiding this comment

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

Yes but you are throwing a generic exception so there is no way to know why the error actually occurred, for instance even if it is an IOExpection the snackbar in this case will say something went wrong. However before the change it would say Please check your internet connection .

Copy link
Member

Choose a reason for hiding this comment

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

Also the GSON instance you are talking about is in CommonUtils.getErrorResponse

@epicadk epicadk requested a review from vj-codes November 24, 2021 05:43
@epicadk epicadk added Status: Changes Requested Changes are required to be done by the PR author. and removed Status: Needs Review PR needs an additional review or a maintainer's review. labels Nov 24, 2021
@epicadk
Copy link
Member

epicadk commented Nov 30, 2021

Any updates?

@epicadk
Copy link
Member

epicadk commented Dec 1, 2021

@vj-codes have you tried sending the request without an active internet connection while testing?

@epicadk
Copy link
Member

epicadk commented Jan 13, 2022

@yveskalume any updates here?

@yveskalume
Copy link
Author

@epicadk calling httpException.response()?.errorBody()?.string() is returning HTML instead of JSON.

The value of the response variable after calling this is

<html>
    <head>
        <title>Internal Server Error</title>
    </head>
    <body>
        <h1><p>Internal Server Error</p></h1>
    </body>
</html>

I think we can just return an instance of CustomResponse with the error message in the constructor instead of using gson :

replace return gson.fromJson(response.toString(), CustomResponse::class.java) with return CustomResponse(response.toString())

@vj-codes
Copy link
Member

@yveskalume closing the PR due to inactivity.
Thank you for your contribution:)

@vj-codes vj-codes closed this Jan 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@epicadk epicadk epicadk requested changes

@isabelcosta isabelcosta Awaiting requested review from isabelcosta isabelcosta is a code owner

@vj-codes vj-codes Awaiting requested review from vj-codes

Assignees
No one assigned
Labels
Status: Changes Requested Changes are required to be done by the PR author.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Bug : the application is not supposed to crash because of a server error
3 participants
@yveskalume @epicadk @vj-codes