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

Fix User not found exception #4506

Merged
merged 2 commits into from
Dec 20, 2022

Conversation

karniv00l
Copy link
Contributor

What does this PR do?

Currently, there's a bug with the way User not found exception is created. This PR fixes it.

Screenshot 2022-10-14 at 18 19 24

Screenshot 2022-10-14 at 18 08 58

Test Plan

You can see on the screenshot that phpstan recognizes it right away, however ideally there should be also a test case for this if statement. I'm not very familiar with the codebase, If anyone could point me to the appropriate test location, I could do it.

Related PRs and Issues

(If this PR is related to any other PR or resolves any issue or related to any issue link all related PR and issues here.)

Have you added your change to the Changelog?

(The CHANGES.md file tracks all the changes that make it to the main branch. Add your change to this file in the following format)

  • One line description of your PR [#pr_number](Link to your PR)

Have you read the Contributing Guidelines on issues?

yes

Copy link
Contributor

@stnguyen90 stnguyen90 left a comment

Choose a reason for hiding this comment

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

@karniv00l
Copy link
Contributor Author

@stnguyen90 I've added missing test cases

tests/e2e/Services/Users/UsersBase.php Outdated Show resolved Hide resolved
tests/e2e/Services/Users/UsersBase.php Outdated Show resolved Hide resolved
@karniv00l karniv00l force-pushed the user-not-found-exception branch 2 times, most recently from a28efb8 to 2e60508 Compare October 15, 2022 18:57
@karniv00l
Copy link
Contributor Author

@stnguyen90 not sure why tests failed, should we try again?

Copy link
Contributor

@stnguyen90 stnguyen90 left a comment

Choose a reason for hiding this comment

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

@karniv00l Hey 👋 awesome work on your PR! We've approved your work and it'll be merged soon!

@stnguyen90 stnguyen90 added the hacktoberfest-accepted Accepted for Hacktoberfest, will be merged later label Oct 18, 2022
@christyjacob4 christyjacob4 merged commit 6b31edf into appwrite:master Dec 20, 2022
@christyjacob4
Copy link
Member

THANK YOU! All changes merged 🥳

Please reach out to me on our Discord server if you would like to claim your Appwrite swags! As a way of saying thank you, we would also love to invite you to join the Appwrite organization on GitHub. Please share your GitHub username with us on Discord.  

You can accept the invite by visiting https://github.com/orgs/appwrite/invitation. By joining our team, you will officially be an Appwrite maintainer on GitHub.

You can change your membership visibility settings, so your new Appwrite team membership badge will show up on your personal GitHub profile.

Please feel free to look for more PRs you might be interested in helping with on our long list of Hacktoberfest friendly issues and help make Appwrite better :)

@karniv00l karniv00l deleted the user-not-found-exception branch December 20, 2022 08:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest-accepted Accepted for Hacktoberfest, will be merged later
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants