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

HIX value differs when calculated via tinymce vs pre save signal #2224

Closed
Tracked by #1928
timobrembeck opened this issue Apr 12, 2023 · 15 comments · Fixed by #2302 or #2439
Closed
Tracked by #1928

HIX value differs when calculated via tinymce vs pre save signal #2224

timobrembeck opened this issue Apr 12, 2023 · 15 comments · Fixed by #2302 or #2439
Assignees
Labels
🐛 bug Something isn't working 🆘 prio: urgent Needs to be resolved now(?)
Milestone

Comments

@timobrembeck
Copy link
Member

Describe the Bug

The HIX value differs depending on how it's calculated.

Steps to Reproduce

  1. Go to a page and calculate the HIX value via the "update" button in the hix widget
  2. Save page
  3. Inspect the object in the database or via Django admin: http://localhost:8000/admin/cms/pagetranslation/
  4. See different value

Expected Behavior

The value should be identical

Actual Behavior

The value differs

Additional Information

I assume this is due to the different way line breaks are handled in tinymce vs the database.
TinyMCE just uses \n as line break whereas the content in the database is \r\n.

@timobrembeck timobrembeck added 🐛 bug Something isn't working ‼️ prio: high Needs to be resolved ASAP. labels Apr 12, 2023
@timobrembeck timobrembeck added this to the 23Q2 milestone Apr 12, 2023
@charludo
Copy link
Contributor

Could you name a page where this is happening? I'm having trouble reproducing this

@timobrembeck
Copy link
Member Author

I noticed this with e.g. "Ausländerbehörde" in the test data...

I noticed that the content in the database (which is sent in thee pre-save signal) contains unicode (ä) characters and lines breaks in the form of \r\n (score: 17.46):

<div>\r\n<p>Die <strong>Ausl&#228;nderbeh&#246;rde</strong> ist&#160;f&#252;r die meisten Angelegenheiten zust&#228;ndig, die mit Ihrem Aufenthalt und&#160;Arbeitserlaubnis zusammenh&#228;ngen.</p>\r\n<p>Was Sie bei der<strong> Ausl&#228;nderbeh&#246;rde </strong>machen k&#246;nnen:</p>\r\n<ul>\r\n<li>Meldebescheinigung, An- und Abmelden des Wohnsitzes</li>\r\n<li>Aufenthaltstitel und Niederlassungserlaubnis</li>\r\n<li>\r\n<div>Verl&#228;ngerung des Ankunftsnachweises</div>\r\n</li>\r\n<li>\r\n<div>Aufenthaltsgestattung ausstellen und verl&#228;ngern (Asyl)</div>\r\n</li>\r\n<li>\r\n<div>Beantragung der Arbeitserlaubnis</div>\r\n</li>\r\n<li>\r\n<div>Aufenthaltserlaubnis und&#160;Ausnahmegenehmigungen f&#252;r Reisen</div>\r\n</li>\r\n<li>F&#252;hrungszeugnisse</li>\r\n<li>Einladung ausl&#228;ndischer G&#228;ste (Verpflichtungserkl&#228;rung)</li>\r\n<li>Einreiseangelegenheiten (z.B.: Familiennachzug)</li>\r\n<li>Hochschulbetreuungsstelle (Studierende, Studienbewerberinnen und Studienbewerber, Besucherinnen und Besucher von Deutsch-Intensivkursen)</li>\r\n<li>Aufenthaltsbeendigung</li>\r\n<li>Einb&#252;rgerung</li>\r\n</ul>\r\n<p>Bitte vereinbaren Sie vor Ihrem Besuch einen Termin. Das funktioniert am besten zwischen 7.30 und 8.00 Uhr am Morgen.&#160;Sie k&#246;nnen auch per Telefon oder online einen Termin ausmachen. Wenn sie Student sind k&#246;nnen Sie einfach eine Nachricht an die folgende E-Mail Adresse schicken:&#160;<span style="color: #99cc00;">aufenthalt.uni@augsburg.de</span></p>\r\n<p><strong>Wichtig:</strong> Falls Sie noch Schwierigkeiten mit der Deutschen Sprache haben,&#160;bringen Sie&#160;bitte eine Person mit, die Ihr Anliegen (den Grund des Besuchs) &#252;bersetzen kann.</p>\r\n<p>Wenn Sie beim Bundesamt f&#252;r Migration (BAMF) Ihren <strong>Asylantrag</strong> gestellt haben (1. Interview), bekommen Sie von der Ausl&#228;nderbeh&#246;rde einen Ausweis (Aufenthaltsgestattung). Bitte tragen Sie den Ausweis immer bei sich.</p>\r\n<p>Haben Sie <strong>Kinder</strong>, die in die Schule gehen m&#252;ssen? Kinder sind schulpflichtig zwischen 6 und 15 Jahren.&#160;Melden Sie&#160;bei der Ausl&#228;nderbeh&#246;rde Ihren Wohnort, wenn m&#246;glich vor dem ersten Termin zur Beratung beim Schulamt. Ohne die Registrierung bei der Ausl&#228;nderbeh&#246;rde kann Ihr Kind nicht f&#252;r die Schule angemeldet werden. Bitte bringen Sie alle Ihre Papiere und Dokumente und alle auf dem Ankunftsnachweis eingetragenen Personen zur Ausl&#228;nderbeh&#246;rde mit.</p>\r\n<p>Weitere Informationen finden Sie unter :&#160;<strong>H&#228;ufige Fragen (FAQ)</strong>&#160;auf der Webseite der&#160;<a href="https://www.augsburg.de/buergerservice-rathaus/buergerservice/aemter-behoerden/staedtische-dienststellen/b/buergeramt/auslaenderbehoerde" rel="noopener">Stadt Augsburg/ Ausl&#228;nderbeh&#246;rde</a></p>\r\n<div><strong>Die Kosten und die notwendigen Unterlagen erfahren Sie bei der zust&#228;ndigen Sachstelle!!! Rufen Sie dort an oder schicken Sie eine E-Mail.</strong></div>\r\n<div>&#160;</div>\r\n</div>

whereas the content from TinyMCE (that is sent from the form widget) uses the HTML-notation (&auml;) and separates just by \n (score: 14.22):

<div>\n<p>Die <strong>Ausl&auml;nderbeh&ouml;rde</strong> ist&nbsp;f&uuml;r die meisten Angelegenheiten zust&auml;ndig, die mit Ihrem Aufenthalt und&nbsp;Arbeitserlaubnis zusammenh&auml;ngen.</p>\n<p>Was Sie bei der<strong> Ausl&auml;nderbeh&ouml;rde </strong>machen k&ouml;nnen:</p>\n<ul>\n<li>Meldebescheinigung, An- und Abmelden des Wohnsitzes</li>\n<li>Aufenthaltstitel und Niederlassungserlaubnis</li>\n<li>\n<div>Verl&auml;ngerung des Ankunftsnachweises</div>\n</li>\n<li>\n<div>Aufenthaltsgestattung ausstellen und verl&auml;ngern (Asyl)</div>\n</li>\n<li>\n<div>Beantragung der Arbeitserlaubnis</div>\n</li>\n<li>\n<div>Aufenthaltserlaubnis und&nbsp;Ausnahmegenehmigungen f&uuml;r Reisen</div>\n</li>\n<li>F&uuml;hrungszeugnisse</li>\n<li>Einladung ausl&auml;ndischer G&auml;ste (Verpflichtungserkl&auml;rung)</li>\n<li>Einreiseangelegenheiten (z.B.: Familiennachzug)</li>\n<li>Hochschulbetreuungsstelle (Studierende, Studienbewerberinnen und Studienbewerber, Besucherinnen und Besucher von Deutsch-Intensivkursen)</li>\n<li>Aufenthaltsbeendigung</li>\n<li>Einb&uuml;rgerung</li>\n</ul>\n<p>Bitte vereinbaren Sie vor Ihrem Besuch einen Termin. Das funktioniert am besten zwischen 7.30 und 8.00 Uhr am Morgen.&nbsp;Sie k&ouml;nnen auch per Telefon oder online einen Termin ausmachen. Wenn sie Student sind k&ouml;nnen Sie einfach eine Nachricht an die folgende E-Mail Adresse schicken:&nbsp;<span style=\"color: #99cc00;\">aufenthalt.uni@augsburg.de</span></p>\n<p><strong>Wichtig:</strong> Falls Sie noch Schwierigkeiten mit der Deutschen Sprache haben,&nbsp;bringen Sie&nbsp;bitte eine Person mit, die Ihr Anliegen (den Grund des Besuchs) &uuml;bersetzen kann.</p>\n<p>Wenn Sie beim Bundesamt f&uuml;r Migration (BAMF) Ihren <strong>Asylantrag</strong> gestellt haben (1. Interview), bekommen Sie von der Ausl&auml;nderbeh&ouml;rde einen Ausweis (Aufenthaltsgestattung). Bitte tragen Sie den Ausweis immer bei sich.</p>\n<p>Haben Sie <strong>Kinder</strong>, die in die Schule gehen m&uuml;ssen? Kinder sind schulpflichtig zwischen 6 und 15 Jahren.&nbsp;Melden Sie&nbsp;bei der Ausl&auml;nderbeh&ouml;rde Ihren Wohnort, wenn m&ouml;glich vor dem ersten Termin zur Beratung beim Schulamt. Ohne die Registrierung bei der Ausl&auml;nderbeh&ouml;rde kann Ihr Kind nicht f&uuml;r die Schule angemeldet werden. Bitte bringen Sie alle Ihre Papiere und Dokumente und alle auf dem Ankunftsnachweis eingetragenen Personen zur Ausl&auml;nderbeh&ouml;rde mit.</p>\n<p>Weitere Informationen finden Sie unter :&nbsp;<strong>H&auml;ufige Fragen (FAQ)</strong>&nbsp;auf der Webseite der&nbsp;<a href=\"https://www.augsburg.de/buergerservice-rathaus/buergerservice/aemter-behoerden/staedtische-dienststellen/b/buergeramt/auslaenderbehoerde\" rel=\"noopener\">Stadt Augsburg/ Ausl&auml;nderbeh&ouml;rde</a></p>\n<div><strong>Die Kosten und die notwendigen Unterlagen erfahren Sie bei der zust&auml;ndigen Sachstelle!!! Rufen Sie dort an oder schicken Sie eine E-Mail.</strong></div>\n<div>&nbsp;</div>\n</div>

So especially when the calculation difference causes MTs to be allowed via one way and forbidden via the other, this could be a very big problem.

@timobrembeck timobrembeck added 🆘 prio: urgent Needs to be resolved now(?) and removed ‼️ prio: high Needs to be resolved ASAP. labels May 8, 2023
@PeterNerlich
Copy link
Contributor

PeterNerlich commented May 15, 2023

Looks to me as if the proper solution is TextLab fixing their whatever it is, and the next best thing we could do is to insert a normalizing step before sending anything off.

@osmers
Copy link

osmers commented May 24, 2023

I received an email on May 11th regarding this, I think:
The comment, among others is:
Wenn Sie folgende Beobachtung reproduzieren können, dann liegt ein echter Bug vor: Ich habe eine Seite im Editor geöffnet, der Index-Wert zeigt 14,49 an. Ich markiere einen Satz im Editor und kopiere ihn, daraufhin wird angezeigt "Der HIX-Wert ist veraltet." Offenbar reagiert das Tool auf das Kopieren so wie auf eine echte Änderungen, obwohl sich am Text nichts geändert hat. Dann lasse ich den HIX-Wert aktualisieren und der Wert fällt auf 11,23 - obwohl sich im Text exakt nichts geändert hat. Das darf nicht sein. Ein identischer Text muss zu einem identischen Index-Wert führen.

I assume that the value goes down bcs of this issue here, but the copying leading to an outdated value is another problem, right? Should I open an issue for that? And how far along are we with this issue?

@timobrembeck
Copy link
Member Author

I assume that the value goes down bcs of this issue here, but the copying leading to an outdated value is another problem, right? Should I open an issue for that?

Yes, you can open an issue for this, but I think the prio is quite low if the issue with the differing values is fixed.

And how far along are we with this issue?

Nobody is assigned to this issue, which means that nobody started working on it.

@osmers
Copy link

osmers commented May 24, 2023

I assume that the value goes down bcs of this issue here, but the copying leading to an outdated value is another problem, right? Should I open an issue for that?

Yes, you can open an issue for this, but I think the prio is quite low if the issue with the differing values is fixed.

Yes, prio low works for me once this issue is fixed.

@osmers
Copy link

osmers commented May 24, 2023

And how far along are we with this issue?

Nobody is assigned to this issue, which means that nobody started working on it.

Ok - is there anybody who could take the issue up? Bcs I think it is quite frustrating for our Kommunen.

@timobrembeck
Copy link
Member Author

Looks to me as if the proper solution is TextLab fixing their whatever it is, and the next best thing we could do is to insert a normalizing step before sending anything off.

@PeterNerlich We don't have insight into Textlab's algorithm, but what we do know is that little differences in the input text can cause differences in the HIX value, independently of whether the different text is really harder/easier to understand.
So yes, Textlab should question whether a different encoding of line breaks really impacts understandability, but independently from this problem we should make sure that we deterministically send the same input text to Textlab in the first place.

@timobrembeck
Copy link
Member Author

Apparently, this is not fully resolved yet:
https://chat.tuerantuer.org/digitalfabrik/pl/d5rkbgzep7yy3cjgwy1x9zro7o

@timobrembeck timobrembeck reopened this Jul 17, 2023
@MizukiTemma MizukiTemma removed their assignment Jul 24, 2023
@david-venhoff david-venhoff modified the milestones: 23Q2, 23Q3 Aug 10, 2023
@seluianova
Copy link
Contributor

seluianova commented Aug 28, 2023

@osmers Hi! Could you please clarify if the problem is reproduced now on all pages, or only on some pages?
If it only happens for some pages - could you please attach the text? (best with tags, as a source text)

I couldn't reproduce the issue :(

@osmers
Copy link

osmers commented Aug 29, 2023

@seluianova it does not seem to be a problem with every page - at least the fact that the HIX value changes. That it is outdated after pressing ctrl A is the issue with every page (I think it should not show outdated when I haven't really done anything).
The changing HIX value can be tried here: https://admin.integreat-app.de/albdonaukreis/pages/de/5091/edit/
Possibly on other pages as well...
Check this page for a big jump in HIX value: https://admin.integreat-app.de/albdonaukreis/pages/de/5094/edit/

@PeterNerlich
Copy link
Contributor

@ulliholtgrave @svenseeberg Can one of you get us the content from the database so that we can try it locally?

@seluianova seluianova self-assigned this Aug 29, 2023
@seluianova
Copy link
Contributor

seluianova commented Aug 29, 2023

I've found the steps to reproduce the issue.
It's only reproduced if the text has more than 1 paragraph.

  1. Create new page with the following content:
<p>Eine sehr schwierige Zeile</p>
<p>Eine weitere sehr schwierige Zeile</p>
  1. Save or Publish it --> HIX is 19.17

  2. Press "Publish" again with no changes --> HIX is 19.86

The behavior will be the same if step 3 is "Press Ctrl+A and reload the HIX value".

The problem is that the first time a page is saved, its content is saved like this:

<div><p>Eine sehr schwierige Zeile</p>\r\n<p>Eine weitere sehr schwierige Zeile</p></div>

If I just publish it a second time with no changes, the content becomes:

<div>\r\n<p>Eine sehr schwierige Zeile</p>\r\n<p>Eine weitere sehr schwierige Zeile</p>\r\n</div>

It's not reproduced with 1 paragraph, because div-tag is not added then. No div - no problem.

We should make sure that we deterministically send the same input text to Textlab in the first place

So we don't just send different content to TextLab, but also change it in our database.
I don't know yet why it happens...
Something connected to tinyMCE maybe?

@PeterNerlich
Copy link
Contributor

PeterNerlich commented Aug 30, 2023

David said:

I think that happens there:

content = fromstring(self.cleaned_data["content"])

Probably because lxml requires a single root element

This seems plausible, in that case the place inside the library should be here:
https://github.com/lxml/lxml/blob/762f62c5a1ab62ce37397aeeab2c27fdcc14ca66/src/lxml/html/__init__.py#L916-L922

Then the next step seems trivial to me. We should make sure we always create a surrounding div element.

@PeterNerlich
Copy link
Contributor

PeterNerlich commented Aug 30, 2023

Looking at the source code of that function, we should be able to switch to something like this:

         try:
-            content = fromstring(self.cleaned_data["content"])
+            doc = document_fromstring(self.cleaned_data["content"])
+            content = doc.body
+            children = content.getchildren()
+            # Ensure that this is stable and we don't add another `div` on every form save
+            if len(children) == 1 and children[0].tag == 'div':
+                content = children[0]
+            else:
+                content.tag = 'div'
         except LxmlError:
             # The content is not guaranteed to be valid html, for example it may be empty
             return self.cleaned_data["content"]

This basically emulates what fromstring() does currently in the case of multiple elements, without the stuff required to handle full documents (I'm just assuming we'll never have <html>/<head>/<body> in our forms)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 bug Something isn't working 🆘 prio: urgent Needs to be resolved now(?)
Projects
None yet
8 participants