digitalfabrik / integreat-cms

Simplified content management back end for the Integreat App - a multilingual information platform for newcomers
https://digitalfabrik.github.io/integreat-cms/
Apache License 2.0
55 stars 33 forks source link

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

Closed timobrembeck closed 10 months ago

timobrembeck commented 1 year ago

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.

charludo commented 1 year ago

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

timobrembeck commented 1 year ago

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.

PeterNerlich commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

seluianova commented 12 months ago

@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 commented 12 months ago

@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 commented 12 months ago

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

seluianova commented 12 months ago

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 commented 11 months ago

David said:

I think that happens there: https://github.com/digitalfabrik/integreat-cms/blob/353ceda9abe39efe3cb5bc9dacff823d21473fe4/integreat_cms/cms/forms/custom_content_model_form.py#L103

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 commented 11 months ago

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)