andrewhancox / moodle-atto_translations

1 stars 4 forks source link

Extra options in quiz and lessons when using this plugin #1

Closed rjnl closed 2 years ago

rjnl commented 2 years ago

This plugin seems to add tags everywhere there is an Atto editor. So when creating/editing quiz questions or lesson pages, we've noticed that extra/empty options are displayed when previewing questions/pages. See screenshot below. Only 3 options were provided in the quiz question but 5 options (2 empty) are displayed after saving the question.

image

Span tags should only be added to Atto editor when it has text in it. Currently, we have to manually remove the span tags from the editor so that the quiz and lesson behave correctly.

andrewhancox commented 2 years ago

There is no easy solution here... There is not (so far as I can see) a way for a toolbar plugin to process the contents of the editor prior to the form being submitted. Even if there was it's not clear how we would handle this case: The editor does not know if it is saving edits to an existing object or it's initial creation so when it sees an editor with nothing but the language span tags should it remove them? If someone added text, saved, the text got translated, they then edited again, saved it blank, then immediately saved it with text, then the link with the translations would be irrevocably broken. I'm also worried there are going to be plugins/bits of core that will have issues with the span tags being present (even though they are hidden).

I'm increasingly thinking that automatically adding this tag is a bad idea... I would suggest we change it as follows: It is never inserted automatically It is visible within the editor (but nowhere else) so can be deleted It is added by clicking the toolbar button at which point you also specify the language of the original text

jgramp commented 2 years ago

Hi Andrew.

We cannot manually specify all the places we want text translated. That would not be scalable. We will have hundreds of courses, each with hundreds of pieces of content to translate. One of our requirements is to automatically run through all our existing courses and add spans to the content, so it is translation ready. We also need to do this for restored courses. Doing this manually defeats the purpose of having this plugin.

The language of the original text should be automatically set to the site's default language. It may be useful to override this in future, but we shouldn't need to define this manually.

"If someone added text, saved, the text got translated, they then edited again, saved it blank, then immediately saved it with text, then the link with the translations would be irrevocably broken." > That is fine. If someone deletes text and saves, they can't expect it to not have consequences. We need this to work for our usual process, rather than trying to foresee all the edge cases that might occur, but are unlikely.

In any case, if there's no span, then the text will match through it having the same content hash anyway, won't it? I thought the process was:

  1. check for a span id and
  2. if this doesn't exist, match it to the hash of the text.
  3. So every translation contains the span id number (if it exists) and the hash of the original text and if the id doesn't match, it falls back to the hash. And this is how we match all of the text in plain (non HTML/Atto editor) text fields too. So if someone was to remove a short answer to a question in a quiz and this then removed the span, as there was no other content in the text field, it would no longer match the span id, but it would match to the translated text based on the hash of the original text right?
jgramp commented 2 years ago

Perhaps a way forward if it is indeed not possible to remove spans from empty sections, is to do the opposite and manually remove/not allow the span in certain places (such as the extra unused quiz answers).

andrewhancox commented 2 years ago

Forgot about this issue as it's no on the main repo...

Extending out the editor plugin to include a drop down menu and additional options is out-side the current scope of work and a reasonable amount of work so will need to wait for now.

The immediate fixes I can see are:

Anywhere text gets saved, if the only contents is the translation span we remove it. There is a problem with an edge case in this approach:

  1. Text gets created with a translation span
  2. The text is translated and the translation is linked to the span tag
  3. The original text is edited, first by emptying the editor entirely and saving (so removing the span tag) then by putting more text in (so adding a new span tag)
  4. The link between the text and it's stale translation has now been irrevocably broken

The other option is to create an exclude list of fields. This is dramatically harder as the atto editor has very limited information about the element it has been created for - it knows the context, page url and form element id - I'm not sure you could reliably exclude these form elements...

I really can't see a good way to handle this.

andrewhancox commented 2 years ago

I'm reluctant to build the tinymce plugin until we have this issue either resolved or de-scoped from the current package of work.

jgramp commented 2 years ago

In terms of the edge case above. I can't see someone removing all the text for a translation, clicking save. Then expecting the translation to still be there somehow. If someone was to save an empty field as a course editor, then yes, their translation is gone. I am comfortable accepting that this edge case exists and if the only contents is the translation span we remove it.

Otherwise perhaps we could manually mark the field as not requiring a translation? It's something that we would do as course editors or during our QA process. Could we wrap the content in tags or something? But this is clunky and I prefer the above approach - if the only contents is the translation span we remove it. The above approach makes things easier overall and I don't think the risk is high that someone will be removing all the content for some reason and expecting to retain translations.

andrewhancox commented 2 years ago

Ok, I've pushed a fix for this that gets it as good as I think it can be. @jgramp @rjnl can you take a look and confirm you're ok with this so we can close this issue down, at that point I'll get the tinymce editor built.

jgramp commented 2 years ago

Hi Andrew.

The issue seems half resolved. I added 3 new answers to a multiple choice quiz question. I filled in one of these with text "the fifth option". I left the other 2 blank.

I saved and "the fifth option" displayed and the 2 blank options did not. So it's resolved the issue with the span tag being added to blank options and these blank options then showing when the quiz is viewed. However, all the other options, that were added with the previous version of the plugin have span tags containing translationids, however the new "a fifth option" does not have a span tag with a translation id. Is it possible to add a span with a translation id if text exists in the answer field, but remove it there is no text in the field - i.e. it's a blank extra question?

image

jgramp commented 2 years ago

Could we use strip tags to check if there's any real content in a field and if this is empty then not save the span or any of the content, otherwise keep the span with the translation id around the content?

https://www.php.net/manual/en/function.strip-tags.php

andrewhancox commented 2 years ago

Annoyingly there is no way for an atto plugin to post-process content on the back-end so we can only use javascript.

I've looked at the problem a bit more closely and there is something odd about the way the atto editors are initialised for these fields, they are rich text but do not consistently have rich text editors rendered. I can see that the span tags were added by the insert_spans CLI tool and that may have caused some unexpected behaviour. I think the best way to handle this would probably to remove the question answers table from the list of tables that you process as described in the readme file.

Since this issue is now not in the atto editor I think we can close this issue.