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

[16.0][FIX] l10n_nl_xaf_auditfile_export: parse EntityRef error #398

Merged
merged 1 commit into from
Feb 12, 2024

Conversation

astirpe
Copy link
Member

@astirpe astirpe commented Sep 20, 2023

When an account has an "&" character in its name, the XAF auditfile export fails.

Here are 2 different errors I was able to reproduce:

Steps to reproduce:

  • install any chart of account, in this example we use l10n_nl
  • create and confirm an invoice
  • create an Audifile export and verify that the xaf file is successfully generated
  • modify the name of account 110000 Debiteuren (that for sure is listed in the auditfile), for example name it: "Debiteuren & test"
  • create a new Audifile export and verify that an error is raised this time

After some investigations, it seems that value_to_html() returns a markup instead of a string. This seems making the etree.XMLParser() unhappy in case of any "&" character present. Converting the markup to string solves the issue. Not sure if there's a better way to solve it.

@thomaspaulb
Copy link
Contributor

What do you mean by 'returns a markup' ?

If I look at what I think is the super function of the one you're modifying (I'm not 100% sure because there are several in ir_qweb_fields.py), and I naively run the code in Odoo shell, then I get this:

>>> from odoo.tools import pycompat, html_escape
>>> html_escape(pycompat.to_text('a & b'))
'a & b'
>>> str(html_escape(pycompat.to_text('a & b')))
'a & b'

I don't see a difference between before and after str() - both results are strings.

@astirpe
Copy link
Member Author

astirpe commented Sep 20, 2023

@thomaspaulb did you try with the steps to reproduce?

@thomaspaulb
Copy link
Contributor

@astirpe Not yet, but I believe you that I can reproduce it functionally that way. Before I go into the trouble of replicating the setup and setting a breakpoint to get the answer to my question, I'd ask you first. So, what do you mean by "returns a markup" ?

@astirpe
Copy link
Member Author

astirpe commented Sep 20, 2023

@thomaspaulb it means that returns a type "Markup" instead of "String".

@@ -13,7 +13,8 @@ class IrQwebAuditfileStringWidget999(models.AbstractModel):
@api.model
def value_to_html(self, value, options):
value = value[: self._max_length] if value else ""
return super().value_to_html(value, options)
res = super().value_to_html(value, options)
return str(res) # From markup to string
Copy link
Contributor

Choose a reason for hiding this comment

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

I did some tests with the help of #399, and indeed I'm running into the xmlParseEntityRef: no name error in cases where a field contains an ampersand.

However your fix only addresses cases where the widget is used. I found that changing xml.unescape() to str(xml) over here addresses the issue for all fields, regardless of the widget used (mine was here)

Copy link
Member Author

Choose a reason for hiding this comment

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

I like your approach, I think is the correct way. Is it ok for you to fix it directly in your PR, so that we can close this one?

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's keep both things separated.

Copy link
Contributor

Choose a reason for hiding this comment

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

Now done in #410

Copy link

@ThijsvOers ThijsvOers left a comment

Choose a reason for hiding this comment

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

Tested the change and it works now

Copy link
Member

@hbrunn hbrunn left a comment

Choose a reason for hiding this comment

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

I'll merge this as it already fixes part of the problem

/ocabot merge

@OCA-git-bot
Copy link
Contributor

Hi @hbrunn. Your command failed:

Required option bumpversion_mode for command merge. Possible values : major, minor, patch, nobump.

Ocabot commands

  • ocabot merge major|minor|patch|nobump
  • ocabot rebase
  • ocabot migration {MODULE_NAME}

More information

@hbrunn
Copy link
Member

hbrunn commented Feb 12, 2024

/ocabot merge patch

@OCA-git-bot
Copy link
Contributor

What a great day to merge this nice PR. Let's do it!
Prepared branch 16.0-ocabot-merge-pr-398-by-hbrunn-bump-patch, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 716ca6c into OCA:16.0 Feb 12, 2024
6 of 7 checks passed
@OCA-git-bot
Copy link
Contributor

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

thomaspaulb added a commit to sunflowerit/l10n-netherlands that referenced this pull request Feb 21, 2024
thomaspaulb added a commit to sunflowerit/l10n-netherlands that referenced this pull request Apr 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants