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.

Sign up for GitHub

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

Jump to bottom

fix(UVE): RunnableLink construction for page tool analytics #29206 #29649

Merged
merged 4 commits into from
Aug 21, 2024
Merged

fix(UVE): RunnableLink construction for page tool analytics #29206 #29649

merged 4 commits into from
Aug 21, 2024

Conversation

valentinogiardino
Copy link
Contributor

@valentinogiardino valentinogiardino commented Aug 19, 2024 •
edited by github-actions bot
Loading

Proposed Changes

  • Update URL construction logic to remove protocol and port for Mozilla tool.
  • Adjust URL construction for Wave and Security Headers tools using URL().origin .

Additional Info

This change aligns the URL formats with the requirements of the analytics tools, ensuring compatibility and preventing potential errors due to unsupported URL components.

This PR fixes #29206

Differences

Mozilla Observatory (Before and After)

image

This PR fixes: #29206

@valentinogiardino
#29206 fix runnableLink construction for page tool analytics
db3f7a4 
- Remove protocol and port from the URL for Mozilla tool as it doesn't support them.
- Ensure Wave and Security Headers tools are constructed without port information as they do not support it.
@valentinogiardino valentinogiardino self-assigned this Aug 19, 2024
nicobytes
nicobytes approved these changes Aug 20, 2024
View reviewed changes
Copy link
Contributor

@nicobytes nicobytes left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM!

Just a thought, why in some tests use any when the function params are typed? Any edge case?

Screenshot 2024-08-20 at 8 58 20 AM

@valentinogiardino
Copy link
Contributor Author

valentinogiardino commented Aug 20, 2024 •
edited
Loading

LGTM!

Just a thought, why in some tests use any when the function params are typed? Any edge case?

Screenshot 2024-08-20 at 8 58 20 AM

@nicobytes Since ellipsizeText is a utility function, I aimed to ensure it handles edge cases and practices defensive programming. Even with TypeScript type enforcement, these tests provide an extra safety net and document how the function handles unexpected inputs. However, if you feel it's more appropriate, I can remove them.

zJaaal
zJaaal requested changes Aug 20, 2024
View reviewed changes
@dotcms-sonarqube dotCMS-sonarqube
Copy link

Quality Gate passed Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube

@valentinogiardino valentinogiardino added this pull request to the merge queue Aug 20, 2024
Merged via the queue into master with commit f758873 Aug 21, 2024
18 checks passed
@valentinogiardino valentinogiardino deleted the issue-29206-uve-page-tools-fix-host-and-port branch August 21, 2024 00:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicobytes nicobytes nicobytes approved these changes

@zJaaal zJaaal zJaaal approved these changes

@oidacra oidacra Awaiting requested review from oidacra

@hmoreras hmoreras Awaiting requested review from hmoreras

@rjvelazco rjvelazco Awaiting requested review from rjvelazco

@KevinDavilaDotCMS KevinDavilaDotCMS Awaiting requested review from KevinDavilaDotCMS

Assignees

@valentinogiardino valentinogiardino

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

UVE: Page Tools Not Working with Traditional Rendering
3 participants
@valentinogiardino @nicobytes @zJaaal