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

[Bug]- Error handling after response retrieval not checking for all cases. #673

Closed
CyberWake opened this issue Apr 17, 2021 · 9 comments
Closed
Assignees
Labels
bug Something isn't working

Comments

@CyberWake
Copy link
Contributor

  • Issue Description:
    There are two use case when the server don't retrieve the data:
    Access Token has expired. Please refresh session.
    User is not authenticated
    The error handling on every screen only checks for one i.e. User is not authenticated
    This needs to be upgraded as it causes error on some actions i found there can be more as well:

    • mark Like unlike a post
    • Remove organization
    • Joining organization
    • Creating organization
      Added all these actions throw error on auto login,
      Error handling currently is checked after converting the exception to string and then slicing. Which causes a lot of mess for calculating the indices of the error and then comparing.
  • Issue Severity:
    High, as this hinders a lot of user actions.

  • Environment Details:

    • Organization settings page
    • join organization page
    • create organization page
    • settings page
  • Observed Behaviour:
    Currently many action return a error response from server on auto login in user. Due to which the user is not able to have access to those features. The response returned needs to be validated and then new token needs to be generated again followed by the action performed.

  • Expected Behaviour:
    Server response handling for both the server response for not providing data should be added. Plus exception need to be checked directly with the inbuilt object provided by the package itself rather than converting it to string and than slicing followed by checking the error response start index.

  • Steps to reproduce issue:
    Important this only occurs on auto login only not on fresh account or fresh logins.
    Perform any of the actions stated above. If more case are found feel free to add them in comments.

  • Snapshots/Videos:
    Liking the post:
    https://thepalisadoes-dyb6419.slack.com/files/U01QZEFJSCE/F01U9TQEG22/talawa
    Removing and organization:
    https://thepalisadoes-dyb6419.slack.com/files/U01QZEFJSCE/F01UVA9B68Z/talawa

  • Does this issue need immediate attention?
    Yes this issue needs to be fixed to it earliest.

  • Are you willing to work on this issue:
    Yes, I am will to work on this please assign this issue to me.

  • Other information:
    Anyone up for help can comment need to fix this to it earliest and is needed to be done every page seeking a response from server.

@github-actions github-actions bot added the unapproved Unapproved, needs to be triaged label Apr 17, 2021
@Sagar2366 Sagar2366 added bug Something isn't working help wanted and removed unapproved Unapproved, needs to be triaged labels Apr 19, 2021
@StrangeNoob
Copy link
Contributor

StrangeNoob commented Apr 22, 2021

@CyberWake Do you find any other than these exceptions ?

@CyberWake
Copy link
Contributor Author

not yet other than the one stated in new issue #687 and I feel this doesn't need to have multiple issues we have to implement it as a whole in every page which retrieves data.

@StrangeNoob
Copy link
Contributor

StrangeNoob commented Apr 23, 2021

@CyberWake rather than checking the substring of the exception, how about we use contains() to check the exception? I think it is the more robust method? What's your take?
BTW Can you tell me the file you will be working on so that we won't get any merge conflicts?

@CyberWake
Copy link
Contributor Author

Yeah the contain method is being used by me. Have created a list of the two errors user not authenticated and access token expired
Then check for these two errors if condition stands true generating a new token else show the particular graphql error to user if there is no graphql error check for server expections show the same two the user.

@CyberWake
Copy link
Contributor Author

Have worked upon join,create and remove actions stated in the PR. After we fix the found actions we can updated the handling on each page fetching data from server.

@CyberWake
Copy link
Contributor Author

Btw you found the comment action was also not working. I adding this in the discription as well. Let's fix the actions found first together and then work on other pages.

@CyberWake
Copy link
Contributor Author

@Sagar2366 sir can you please guide us on how should we create PR as there will be a lot of files to be changed as almost every page fetches data from server.
Help us with a solution for fast reviewal of PRs for the fix.

@palisadoes
Copy link
Contributor

@CyberWake

  1. Have you written successful tests for your change?
  2. If so, please make sure the code lints correctly and submit the pull request.

It looks like an important bug, so we will give it priority.

@CyberWake
Copy link
Contributor Author

@palisadoes sir I'll make sure to update the branch with the lint before sending a PR.

Ayush0Chaudhary pushed a commit to Ayush0Chaudhary/talawa that referenced this issue Mar 29, 2023
…dation#673)

* test: create event mutation & events query unit tests

* test: events query unit test fix

* test: fix events query unit tests

* fix: type error in events query

* test: events query tests using orderby dynamically

* log: generated logs for resolving errors

* fix: changed type checking

* log: generated logs for resolving errors

* refactor: cleaned up logs

* test: updated constants
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants