-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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: added correct verification endpoint & validation logic for alegra #3437
Merged
kashifkhan0771
merged 5 commits into
trufflesecurity:main
from
sahil9001:alegra-verfication-endpoint-fix
Oct 24, 2024
Merged
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
7b6339f
fix: added correct verification endpoint for alegra
sahil9001 d6eb1a9
fix: fixed email regex
sahil9001 ea88988
Merge remote-tracking branch 'upstream/main' into alegra-verfication-…
sahil9001 735c34d
fix: added correct tests and validation
sahil9001 d734bd0
fix: fixed alegra tests
sahil9001 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I created an account on Alegra and generated a token. Initially, I tested the
/users
API endpoint using an API client, and it worked as expected, returning the same response as/users/self
. However, the Alegra documentation only mentions the/users/self
endpoint, and there’s no reference to/users
, so I'm unsure why it’s functioning similarly.Additionally, I noticed an issue with the detector failing to identify my user ID, which I believe is always the email used during signup. The regex used for detecting IDs has a length constraint of 25 to 30 characters. However, I was able to create an account with an email shorter than this limit, and despite using that email (as my username) along with my token to successfully call the API via the API client, the detector did not pick it up.
I suggest testing and adjusting the regex for detecting user IDs to ensure it covers valid email lengths.
As for the API behavior, I’ll leave that to @zricethezav and @abmussani. Both
/users
and/users/self
worked for me, though only the/users/self
endpoint is mentioned in the documentation.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the detailed review, @kashifkhan0771 !
I have expanded the email regex to include all case for any possible email.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would feel more confident approving this if you could also test the current email regex to confirm that it isn't working correctly(I tested it but a second round of testing from the PR owner will be good I believe. You can create an account and test this email regex change). A screenshot or any form of documentation from your testing would be sufficient.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have added the testing screenshot, please check
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New Changes are picking up the key and ID correctly now and verifying. Wait for @zricethezav and @abmussani to respond about the API change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you apply the label for Hacktoberfest @kashifkhan0771?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once the PR is approved. We can add it 😃 - Once again thanks for the all the fixes and contributions!