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

refactor: Upgrade ldapjs from 2.3.3 to 3.0.2 #8891

Closed
wants to merge 14 commits into from

Conversation

onurhanife
Copy link
Contributor

Pull Request

Issue

Closes: #8592

Approach

#8623 cloned, merged to alpha and conflicts are fixed

Tasks

  • Add tests
  • Add changes to documentation (guides, repository pages, code comments)
  • Add security check
  • Add new Parse Error codes to Parse JS SDK

Copy link

Thanks for opening this pull request!

Copy link

codecov bot commented Jan 16, 2024

Codecov Report

Attention: Patch coverage is 88.23529% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 94.19%. Comparing base (ac7399b) to head (f933b56).
Report is 5 commits behind head on alpha.

❗ Current head f933b56 differs from pull request most recent head 39cd931. Consider uploading reports for the commit 39cd931 to get more accurate results

Files Patch % Lines
src/Adapters/Cache/RedisCacheAdapter.js 50.00% 1 Missing ⚠️
src/cli/utils/runner.js 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##            alpha    #8891      +/-   ##
==========================================
- Coverage   94.20%   94.19%   -0.01%     
==========================================
  Files         186      186              
  Lines       14797    14802       +5     
==========================================
+ Hits        13939    13943       +4     
- Misses        858      859       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@onurhanife onurhanife marked this pull request as ready for review January 16, 2024 09:00
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.

There seems to be a conflict again, after I've merged some of the open PRs. Could you please resolve them, so we can merge this? Thanks

@onurhanife
Copy link
Contributor Author

@mtrezza i resolved conflicts but there some new tests that you wrote about ignoreEmailVerification and page routing tests related to emailVerifications are fails. FYI

@mtrezza
Copy link
Member

mtrezza commented Feb 22, 2024

Which tests are you referring to? They all seem to pass except for some flaky tests.

@mtrezza mtrezza changed the title feat: Update ldap refactor: Upgrade ldapjs from 2.3.3 to 3.0.2 Feb 22, 2024
@mtrezza
Copy link
Member

mtrezza commented Feb 22, 2024

@onurhanife This PR also contains quite a few code style changes. Not sure where they come from, but could you please make sure they are not included in the PR? The PR should only contain the changes that are necessary for the fix.

@onurhanife
Copy link
Contributor Author

onurhanife commented Feb 22, 2024

@mtrezza My prettiers require config file option enabled, and I prefer using double quotes in my Prettier configurations. However, I'm not sure where these style changes are coming from. I also noticed that the .prettierrc file is not in JSON format, which might be why it's not working correctly. Is it intentionally like this, or I can fix it with this PR?

@mtrezza
Copy link
Member

mtrezza commented Feb 23, 2024

The code style changes I see in this PR make sense, but they shouldn't be in this PR. If easily possible, could you revert them and only keep the necessary changes? I believe it's just 1 code line that is required, and the package upgrade. It may be easier to just create a new PR?

@onurhanife onurhanife marked this pull request as draft February 26, 2024 08:57
@mtrezza
Copy link
Member

mtrezza commented Feb 26, 2024

It seems something went wrong here. I suggest to either revert all changes by just replacing the files, or open a new PR.

@mtrezza
Copy link
Member

mtrezza commented Feb 26, 2024

Closing via #8947

@mtrezza mtrezza closed this Feb 26, 2024
@mtrezza
Copy link
Member

mtrezza commented Feb 26, 2024

@onurhanife Kindly see #8947; the PR was simple and required just 1 line of code, when removing the code style changes.

@onurhanife
Copy link
Contributor Author

@mtrezza I was aware that we could handle it this simply, I just hadn't had the chance to focus on this side yet, but I see that you've resolved the issue in my place. Thank you.

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.

4 participants