Skip to content

[FIX] html_builder: error message for invalid URL #4601

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

Merged

Conversation

emge-odoo
Copy link

[LEBL] Select a text, add a link, add "http://", save, click on the link, traceback occurs
https://pad.odoo.com/p/mysterious-egg-builder-editor-bug

@robodoo
Copy link

robodoo commented May 7, 2025

This PR targets the un-managed branch odoo-dev/odoo:master-mysterious-egg, it needs to be retargeted before it can be merged.

@Jinjiu96
Copy link

Jinjiu96 commented May 8, 2025

Hello 👋 I had a small discussion with @robinlej yesterday before I left, so a better way to fix it would be at deduceUrl or applyDeducedUrl in link_popover.js. When we have url as http://or https:// it's better to consider it as an empty input. When we have an empty url and click apply, we don't create a link. By doing this, we avoid to have a link element with an invalid URL which causes this traceback 🙏

@emge-odoo emge-odoo force-pushed the master-mysterious-egg-fix-link-error-emge branch from be2bffc to 83a1fb2 Compare May 8, 2025 10:40
@emge-odoo
Copy link
Author

Hello 👋 I had a small discussion with @robinlej yesterday before I left, so a better way to fix it would be at deduceUrl or applyDeducedUrl in link_popover.js. When we have url as http://or https:// it's better to consider it as an empty input. When we have an empty url and click apply, we don't create a link. By doing this, we avoid to have a link element with an invalid URL which causes this traceback 🙏

Hello, I made a change in the fix before I saw your message. I did not do exactly what you asked. If the URL is not valid I simply don't call the onApply function. The advantage of doing this is that it works for any invalid url not only "https://" and "http://". One drawback might be that now, the onApply call is done after loadAsyncLinkPreview has finished. If this is an issue, I can modify the code by splitting loadAsyncLinkPreview in two to avoid waiting on an asynchronous function before calling onApply. Let me know what you think.

@emge-odoo emge-odoo force-pushed the master-mysterious-egg-fix-link-error-emge branch from 83a1fb2 to adf86a7 Compare May 8, 2025 11:32
@emge-odoo
Copy link
Author

I also don"t really know if there are cases in which we want to call the onApply function even if the URL is invalid

@emge-odoo emge-odoo force-pushed the master-mysterious-egg-fix-link-error-emge branch from adf86a7 to e702b8c Compare May 8, 2025 11:58
Copy link

@Jinjiu96 Jinjiu96 left a comment

Choose a reason for hiding this comment

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

Here is my feedback, hope it helps 🙏

@emge-odoo emge-odoo force-pushed the master-mysterious-egg-fix-link-error-emge branch 3 times, most recently from f243f33 to 06c1bb5 Compare May 8, 2025 14:14
@emge-odoo emge-odoo force-pushed the master-mysterious-egg-fix-link-error-emge branch from 06c1bb5 to 55423d5 Compare May 9, 2025 10:33
@Jinjiu96
Copy link

Jinjiu96 commented May 9, 2025

Looks good to me, just remember the translation remark from Robin and then we are good to go 😉

@emge-odoo emge-odoo force-pushed the master-mysterious-egg-fix-link-error-emge branch from 55423d5 to 686e695 Compare May 9, 2025 15:01
@robinlej robinlej merged commit 1473ec2 into master-mysterious-egg May 9, 2025
@robinlej robinlej deleted the master-mysterious-egg-fix-link-error-emge branch May 9, 2025 15:47
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