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

Krishna2323/issue/49286 #813

Merged
merged 4 commits into from
Oct 22, 2024

Conversation

Krishna2323
Copy link
Contributor

@Krishna2323 Krishna2323 commented Oct 15, 2024

Fixed Issues

$ GH_LINK Expensify/App#49286

Details: This PR updates the sanitizeURL utility function to accept a new parameter, scheme. The scheme parameter allows us to specify the scheme as needed. The default value is https, but we can pass http or any other scheme as the scheme parameter to override it.

Tests

  1. What unit/integration tests cover your change? What autoQA tests cover your change?
  2. What tests did you perform that validates your changed worked?

QA

  1. What does QA need to do to validate your changes?
  2. What areas to they need to test for regressions?

Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
@Krishna2323 Krishna2323 requested a review from a team as a code owner October 15, 2024 19:02
@melvin-bot melvin-bot bot requested review from rlinoz and removed request for a team October 15, 2024 19:03
lib/str.ts Outdated
* @returns The formatted URL
*/
sanitizeURL(url: string): string {
sanitizeURL(url: string, scheme = 'https'): string {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
sanitizeURL(url: string, scheme = 'https'): string {
sanitizeURL(url: string, defaultScheme = 'https'): string {

Copy link
Contributor

@ahmedGaber93 ahmedGaber93 Oct 15, 2024

Choose a reason for hiding this comment

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

I think defaultScheme is more meaningful because it uses only as a default value if the URL didn't have a scheme. We also need to update the comment and its usage.

Copy link
Contributor

Choose a reason for hiding this comment

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

We can also add a unit to confirm that

expect(Str.sanitizeURL('https://example.com', 'http')).toBe('https://example.com');

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

@marcaaron marcaaron requested review from marcaaron and removed request for rlinoz October 16, 2024 22:12
@marcaaron
Copy link
Contributor

@rlinoz I'll take over as I'm assigned to the parent issue

@ahmedGaber93
Copy link
Contributor

@Krishna2323 bump for the above review

Signed-off-by: krishna2323 <belivethatkg@gmail.com>
expect(Str.sanitizeURL('FOO.com/blah_BLAH')).toBe('https://foo.com/blah_BLAH');
+expect(Str.sanitizeURL('FOO.com/blah_BLAH', 'http')).toBe('http://foo.com/blah_BLAH');
+expect(Str.sanitizeURL('example.com', 'http')).toBe('http://example.com');
expect(Str.sanitizeURL('https://example.com', 'http')).toBe('http://example.com');
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
expect(Str.sanitizeURL('https://example.com', 'http')).toBe('http://example.com');
expect(Str.sanitizeURL('https://example.com', 'http')).toBe('https://example.com');

Copy link
Contributor

Choose a reason for hiding this comment

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

@Krishna2323 Let's finish it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Copy link
Contributor

@ahmedGaber93 ahmedGaber93 left a comment

Choose a reason for hiding this comment

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

LGTM!

@marcaaron marcaaron merged commit 2fc4e3b into Expensify:main Oct 22, 2024
6 checks passed
@os-botify
Copy link
Contributor

os-botify bot commented Oct 22, 2024

🚀 Published to npm in 2.0.103 🎉

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.

3 participants