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 URL encoding issue for links #446

Merged
merged 3 commits into from
Mar 7, 2024

Conversation

jean-the-coder
Copy link
Contributor

Description

Fixes #437. Its possible for ids to include non-url-safe characters. Since these ids are being interpolated into a url string without encoding, it is causing the url to fail.

To fix this, I've updated the code so that we are passing urls as arrays of strings rather than an interpolated string. By passing as an array of strings, the built in router knows to encode each string if needed.

There were a few places in components where we were using navigateByUrl with an interpolated string. navigateByUrl doesn't accept an array of strings so I've switched those few places to use navigate. I'm not sure if there was a specific reason for using navigateByUrl over navigate in those spots. If there is a need to use navigateByUrl please let me know and I will update those spots to html-encode another way.

Changes

  • Switch to using an array of strings making up the parts of the url rather than string interpolation
  • Switch to using .navigate instead of .navigateByUrl to make it easier to ensure proper url encoding

@AnalogJ
Copy link
Member

AnalogJ commented Mar 7, 2024

honestly, now that I look at this, I think the reason I used navigateByUrl instead of navigate is because that's the only one I was familar with. I rarely use navigate and I had no idea that it handled url-encoding differently.

@AnalogJ
Copy link
Member

AnalogJ commented Mar 7, 2024

if possible, I'd love to see a "toy" test confirming that navigate will correctly url-encode the = in the base64 encoded ID from #437. Is that something you can add @jean-the-coder ?

@jean-the-coder
Copy link
Contributor Author

I rarely use navigate and I had no idea that it handled url-encoding differently.

I will say, tbf, if you pass an already interpolated string to navigate, then I think it would be similar to navigateByUrl and not encode it. They both seem to assume that a single string passed is exactly what you want. It's only when you pass an array of multiple values that navigate encodes the sections properly.

if possible, I'd love to see a "toy" test confirming that navigate will correctly url-encode the = in the base64 encoded ID from #437.

added a quick test to one of the smaller components 986fd13

Copy link
Member

@AnalogJ AnalogJ left a comment

Choose a reason for hiding this comment

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

this looks great, it should solve the issue we were seeing in #437

Fantastic work, thanks for the PR! 🥳

const link = fixture.debugElement.nativeElement.querySelector('a');
console.log(link);
expect(link.href).toContain(
'/explore/123-456-789%3D/resource/aHR0cHM6Ly93d3cubWVyY3kubmV0L3NpdGVzL2RlZmF1bHQvZmlsZXMvZG9jdG9yX2ltYWdlcy9kZXNrdG9wLzE2NTk4ODYwNTktbS5qcGc%3D'
Copy link
Member

Choose a reason for hiding this comment

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

👍

@AnalogJ AnalogJ merged commit 43579df into fastenhealth:main Mar 7, 2024
4 checks passed
@jean-the-coder jean-the-coder deleted the url-encoding-437 branch March 8, 2024 01:35
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.

[Bug]: Explorer page, binary tab, fails to show images.
2 participants