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] website_lazy_load_image: HTML parsing #651

Closed
wants to merge 3 commits into from
Closed

[FIX] website_lazy_load_image: HTML parsing #651

wants to merge 3 commits into from

Conversation

hugho-ad
Copy link

@hugho-ad hugho-ad commented Jul 16, 2019 •
edited

fromstring function.

Before this commit, the render_template has an issue with views retrieved
from a controller, like the wizard of event detail registration, the
the content was introduced into an html DOM, this leads a wrong modal
behavior.

Now the first attempt is with fromstring function if fails, then use
the original process with html function.

Before
image

After
image

Before
image

After
image

@hugho-ad hugho-ad force-pushed the 11.0-website-lazy-dev-hugho-ad branch from d618894 to c3b23d5 Compare July 16, 2019 17:08
@hugho-ad
Copy link
Author

@oscarolar could you review this please

simahawk
simahawk requested changes Jul 17, 2019
View reviewed changes
Copy link

@simahawk simahawk left a comment

Choose a reason for hiding this comment

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

Please check #571 and #551
If we change this should do it safely and only once. And we should have specific tests to cover the use cases that were broken.
In any case IMO we should not parse as HTML something that is not.

@simahawk simahawk changed the title [FIX] website_lazy_load_image: Fix missing encoding='UTF-8' and use [FIX] website_lazy_load_image: HTML parsing Jul 17, 2019
@simahawk simahawk added this to the 11.0 milestone Jul 17, 2019
@hugho-ad hugho-ad force-pushed the 11.0-website-lazy-dev-hugho-ad branch from c3b23d5 to 20adf47 Compare July 17, 2019 14:47
@hugho-ad
Copy link
Author

@simahawk Thanks for your feedback!

I committed your changes proposed at #571 (comment)
I before passing res to html.fromstring method I decode res, I mean:

html = ehtml.fromstring(res.decode())

What do you think?

This was referenced Jul 18, 2019
Closed
Closed
simahawk
simahawk requested changes Jul 19, 2019
View reviewed changes
website_lazy_load_image/models/ir_ui_view.py Outdated
@@ -2,7 +2,7 @@
# License AGPL-3.0 or later (http://www.gnu.org/licenses/agpl).

from odoo import api, models
from lxml import etree
from lxml import html as ehtml, etree
Copy link

Choose a reason for hiding this comment

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

just import lxml use lxml.etree and lxml.html

website_lazy_load_image/models/ir_ui_view.py Outdated
@@ -22,7 +22,7 @@ def render_template(self, template, values=None, engine='ir.qweb'):
website_id = self.env.context.get('website_id')
if website_id and not \
self.env['website'].browse(website_id).is_publisher():
html = etree.HTML(res)
html = ehtml.fromstring(res.decode())
Copy link

Choose a reason for hiding this comment

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

hmm I don't understand: where is the check on whether this is or not an HTML source? 🤔

Copy link
Author

Choose a reason for hiding this comment

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

as you mentioned, lxml.html.fromstring keep the content as it's if it's not an html content.
I just tested it

Copy link
Author

Choose a reason for hiding this comment

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

Also, I didn't face any problem with robots.txt as mentioned on the issue #551

Copy link

Choose a reason for hiding this comment

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

ok, cool! Can you update tests here to avoid regressions?

@hugho-ad
[FIX] website_lazy_load_image: Fix missing encoding='UTF-8' and use
0b8d011 
html.fromstring function.

Before this commit, the render_template has an issue with views retrieved
from a controller, like the wizard of event detail registration, the
content was wrapped into an html DOM, this leads a wrong modal
behavior.

FIX OCA#551
@hugho-ad hugho-ad force-pushed the 11.0-website-lazy-dev-hugho-ad branch from 20adf47 to 0b8d011 Compare July 19, 2019 15:33
@hugho-ad
Copy link
Author

@simahawk
Done

Before
image
After
image

Before
image
image

@hugho-ad hugho-ad force-pushed the 11.0-website-lazy-dev-hugho-ad branch from 9eaaac6 to 32acf5e Compare July 22, 2019 23:51
@hugho-ad hugho-ad force-pushed the 11.0-website-lazy-dev-hugho-ad branch 2 times, most recently from 9268e7a to 2a84a7c Compare July 23, 2019 16:07
@simahawk
Copy link

@hugho-ad pls check travis.
@tarteo can you pass by for a review?

@hugho-ad hugho-ad force-pushed the 11.0-website-lazy-dev-hugho-ad branch from 2a84a7c to 00edff0 Compare July 23, 2019 18:20
@hugho-ad
Copy link
Author

Travis red job build no related to this
image

I just forced a travis rebuild

@hugho-ad
Copy link
Author

@tarteo could you review please

@hugho-ad hugho-ad force-pushed the 11.0-website-lazy-dev-hugho-ad branch from 00edff0 to a6d9ce9 Compare July 26, 2019 01:28
moylop260
moylop260 approved these changes Jul 26, 2019
View reviewed changes
Copy link

@moylop260 moylop260 left a comment

Choose a reason for hiding this comment

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

👍
Thanks!

@moylop260
Copy link

@oscarolar
Could you check it, please?

@OCA-git-bot
Copy link
Contributor

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

@hugho-ad
Copy link
Author

@yajo could you merge please

@simahawk
Copy link

/ocabot merge minor

@OCA-git-bot
Copy link
Contributor

On my way to merge this fine PR!
Rebased to 11.0-ocabot-merge-pr-651-by-simahawk-bump-minor, awaiting test results.

OCA-git-bot added a commit that referenced this pull request Jul 27, 2019
@OCA-git-bot
Merge PR #651 into 11.0
36dc3cc 
Signed-off-by simahawk
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at 36dc3cc. Thanks a lot for contributing to OCA. ❤️

PS: Don't worry if GitHub says there are unmerged commits: it is due to a rebase before merge. All commits of this PR have been merged into 11.0.

@hugho-ad hugho-ad mentioned this pull request Jul 30, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@simahawk simahawk simahawk approved these changes

@moylop260 moylop260 moylop260 approved these changes

Assignees
No one assigned
Labels
approved merged 🎉 needs review ready to merge
Projects
None yet
Milestone
11.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@hugho-ad @simahawk @moylop260 @OCA-git-bot