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

feat: Add customizable log level for 'username already exists' error #9336

Open
wants to merge 23 commits into
base: alpha
Choose a base branch
from

Conversation

KrishDave1
Copy link

Pull Request

Issue

#9329

Closes: #9329

Approach

So I have created a new interface LogEventsOptions interface in src/Options/index.js to incorporate the custom logEvents option in case of username already taken error.

Then I have modified the RestWrite.js file to check for and apply the custom log level when a username already exists error occurs.I have also added documentation for the new option in src/Options/docs.js.

I have also included a new test case in spec/ParseUser.spec.js to verify the functionality of the custom log level.

If any changes are required , please inform me , I will do it.

Tasks

  • Add tests
  • Add changes to documentation (guides, repository pages, code comments)

Copy link

Thanks for opening this pull request!

src/Options/index.js Outdated Show resolved Hide resolved
src/RestWrite.js Outdated Show resolved Hide resolved
src/Options/docs.js Outdated Show resolved Hide resolved
spec/ParseUser.spec.js Outdated Show resolved Hide resolved
@KrishDave1
Copy link
Author

KrishDave1 commented Oct 11, 2024

Thanks for the suggestions. I have made the changes according to the suggestions. @mtrezza Please have a look and tell if anything else needs to be changed

@KrishDave1 KrishDave1 requested a review from mtrezza October 12, 2024 07:01
src/RestWrite.js Outdated Show resolved Hide resolved
src/Options/index.js Outdated Show resolved Hide resolved
src/Options/index.js Outdated Show resolved Hide resolved
spec/ParseUser.spec.js Outdated Show resolved Hide resolved
spec/ParseUser.spec.js Outdated Show resolved Hide resolved
@KrishDave1 KrishDave1 requested a review from mtrezza October 12, 2024 08:25
spec/ParseUser.spec.js Outdated Show resolved Hide resolved
@KrishDave1 KrishDave1 requested a review from mtrezza October 12, 2024 09:25
@KrishDave1
Copy link
Author

KrishDave1 commented Oct 12, 2024

@mtrezza I do not have a indepth knowledge of testing, earlier when you merged alpha branch into this branch, some tests were failing.I am eager to learn, and sorry if this question sounds dumb but could I know why?

spec/ParseUser.spec.js Outdated Show resolved Hide resolved
@mtrezza
Copy link
Member

mtrezza commented Oct 12, 2024

earlier when you merged alpha branch into this branch, some tests were failing.

That is because with your previous commits, the CI never ran fully. If you click on the red "X" beside your commits, you can see that only 2 CI jobs ran. My merge started a full CI run. To make sure all tests pass locally you would run the full test suite locally with npm run test. You can replace describe with fdescribe or it with fit to run only single tests or sections of tests, but a full run locally should always be done at some point because you want to make sure all tests pass.

@KrishDave1
Copy link
Author

Hey @mtrezza thanks for explaining.I have modified the tests as you suggested.Please review it and let me know

spec/ParseUser.spec.js Outdated Show resolved Hide resolved
spec/ParseUser.spec.js Outdated Show resolved Hide resolved
spec/ParseUser.spec.js Outdated Show resolved Hide resolved
spec/ParseUser.spec.js Outdated Show resolved Hide resolved
spec/ParseUser.spec.js Outdated Show resolved Hide resolved
spec/ParseUser.spec.js Outdated Show resolved Hide resolved
spec/ParseUser.spec.js Outdated Show resolved Hide resolved
@KrishDave1 KrishDave1 requested a review from mtrezza October 12, 2024 17:45
@KrishDave1
Copy link
Author

I have made the required changes.Please check

@KrishDave1
Copy link
Author

Are there any more changes required? @mtrezza .Sorry for asking.

@mtrezza
Copy link
Member

mtrezza commented Oct 13, 2024

I have refactored the test to make it more robust and concise. Observations:

  • There are 3 places where this error can be thrown; I have added the logger to the 2 missing places --> not sure how they are invoked, we'll likely need to add tests for test coverage.
  • Added testing for log levels
    • silent which seems to be a special case in which the logger must not be called
    • undefined, i.e. default value, to make sure that if no logLevel is set, the error is still logged with default log level, which is error in this case.
  • The CI only fails for logLevel error, as it's logged twice --> needs to be investigated

@KrishDave1 KrishDave1 requested a review from mtrezza October 13, 2024 21:54
Copy link
Member

@mtrezza mtrezza left a comment

Choose a reason for hiding this comment

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

You requested a review but you did not push any new commit. Review feedback see #9336 (comment).

@KrishDave1
Copy link
Author

I believe while we create a user in RestWrite.js we already check for usernamealreadyexists and log it there so , no need of logging it again.Please check for the change I did.

@KrishDave1 KrishDave1 requested a review from mtrezza October 15, 2024 05:38
@mtrezza
Copy link
Member

mtrezza commented Oct 15, 2024

That may be right. Why is it throwing the specific error there then? Is that error internally caught?

@KrishDave1
Copy link
Author

Why are the tests failing?I read the test error.Isn't this what we are supposed to do?

@mtrezza
Copy link
Member

mtrezza commented Oct 15, 2024

If you run the test locally, does it pass? I assume this is only related to one of the log levels?

@mtrezza
Copy link
Member

mtrezza commented Oct 16, 2024

Good catch in 386d2c1; let's see if the whole CI passes.

spec/ParseUser.spec.js Outdated Show resolved Hide resolved
Signed-off-by: Manuel <[email protected]>
@KrishDave1
Copy link
Author

Why is the CI failing even though the logic seems correct?

@mtrezza
Copy link
Member

mtrezza commented Oct 16, 2024

I have refactored the test to make it easier to debug.

  • The only log levels that are recognized by the logger are info, warn, error.
  • The Parse Server config allows to also set undefined and silent as log level, but they are not real log levels interpreted by the logger. Instead, undefined means the default log level (error in this case) will be used; silent means that the logger will not be invoked at all.

To debug the test easier, I have modified it to only run with usernameAlreadyExists: 'info'. The refactored test will now check which one of the internal logger methods info, warn, error have been called. The result is that even with log level info, an event of level error is logged.

The reason seems to be that the error is logged here:

throw new Parse.Error(
Parse.Error.USERNAME_TAKEN,
'Account already exists for this username.'
);

None of the other 2 places where this PR currently places the log condition is called.

It seems this is then passed up the chain and sent to the logger at some point in the code. I believe the solution is to find that point in code where it's sent to the logger, test for the error code (and maybe error text :-/ ) and put a condition there to log accordingly.

@KrishDave1
Copy link
Author

I think just checking what type of log levels is there should solve the issue.Please merge to check again with CI

@mtrezza
Copy link
Member

mtrezza commented Oct 16, 2024

Could you please run the test locally to see whether it passes? I doubt that this fixes it, because when I debugged the test, it didn't even run over these lines, see my comment above. Also, we probably shouldn't use this logic because there may be more or different logic level in the future. In fact, there is also a debug level, so I'd revert your last commit.

@KrishDave1
Copy link
Author

Could you help you here I am unable to find the error.Could you help you here?Thanks

@KrishDave1
Copy link
Author

I am a bit stuck here.Could you help me out here?I am unable to understand where is this log of error is coming from.

@KrishDave1
Copy link
Author

The tests are still failing.Is there anything I can do?

@mtrezza
Copy link
Member

mtrezza commented Oct 24, 2024

This would need to be investigated; unfortunately I don't have the resources at the moment to support beyond the investigation I've posted above.

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.

Add log option for username taken event
2 participants