Problem/Motivation

JobItemForm don't check if hasTranslator and fail when Save.
This happens if you are in "admin/structure/types/manage/article/translate" and you request translation and close without Submitting. Then you go again to "admin/structure/types/manage/article/translate" click "In progress" and try to Save. You will get an Exception.

Proposed resolution

Add validation in JobItemForm to check if hasTranslator when saving.
And maybe redirect to the JobForm if the job do not have translator when building the form.

Remaining tasks

create a patch, review and commit

User interface changes

None

API changes

None

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

edurenye created an issue. See original summary.

dragos-dumi’s picture

Status: Active » Needs review
FileSize
840 bytes
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 2,433 pass(es), 1 fail(s), and 0 exception(s). View

Added validation on job item form submit to check if the job has a translator.

Regarding the redirect to the job form, if wanted, do you have any suggestion how to do that in the job item form method?

Status: Needs review » Needs work
Berdir’s picture

+++ b/src/Form/JobItemForm.php
@@ -182,6 +182,9 @@ class JobItemForm extends TmgmtFormBase {
+    if (!$item->hasTranslator()) {
+      $form_state->setErrorByName('accept', $this->t('The job item could not be saved because the job does not have a translator assigned.'));
+    }
     if($item->getTranslator()){

I don't think we need to have a validation error. We just need to avoid calling the translator, which means we just need to switch the getTranslator() call below to hasTranslator().

And we will need tests, since they currently don't fail.

Status: Needs work » Needs review

Status: Needs review » Needs work
sasanikolic’s picture

This core issue probably broke configration translation. We need the core issue fixed before making a patch for this issue.

sasanikolic’s picture

I get this error.

tduong’s picture

Uploaded:

  • patch including changes in JobItemForm and extended test coverage for this case
  • interdiff of the two version of JobItemForm file
giancarlosotelo’s picture

Status: Needs review » Needs work

Looks pretty good to me. Some minor stuff

  1. +++ b/sources/tmgmt_config/src/Tests/ConfigSourceUiTest.php
    @@ -117,6 +117,33 @@ class ConfigSourceUiTest extends EntityTestBase {
    +    // Request an italian translation.
    

    Maybe we can add a comment here about what we are testing.

  2. +++ b/sources/tmgmt_config/src/Tests/ConfigSourceUiTest.php
    @@ -117,6 +117,33 @@ class ConfigSourceUiTest extends EntityTestBase {
    +    $this->assertText(t('One job needs to be checked out.'));
    +    $this->assertText('Article content type (English to Italian, Unprocessed)');
    ...
    +    $this->assertText(t('Job item Article content type'));
    

    We can get rid of these asserts because we already do that before.

tduong’s picture

Status: Needs work » Needs review
FileSize
1.25 KB
2.31 KB

Uploaded patch:

  • added explanation comment lines
  • dropped unnecessary assertion lines
giancarlosotelo’s picture

Status: Needs review » Reviewed & tested by the community

Seems fine to me.

miro_dietiker’s picture

Status: Reviewed & tested by the community » Needs work

If there is no translator assigned, why do we allow editing and we are showing the save button?
Feels like a UX trap.

See also comment above from Berdir: "We just need to avoid calling the translator, which means we just need to switch the getTranslator() call below to hasTranslator()."

So the fact that we don't have a translator is definitively not an Error.
We should discuss to properly define the goal.

Berdir’s picture

Just remove the else. No error, just don't call the translator. This is just a left-over of the switch to hasTranslation().

Editing before submitting is currently not very useful, but we want to change that (e.g. skip data items from being translated/sent to the translator and stuff like that)

tduong’s picture

Uploaded patch:

  • dropped else block
  • dropped lines in the test about the error message
  • add hasTranslator() in save(), otherwise it gives an exception

miro_dietiker’s picture

Status: Needs review » Needs work

I don't agree with the save() modification.
The problem is in addTranslatedData() where the Translator is called.
Check there if a translator is present.

tduong’s picture

  • reset save() as the origin
  • added a statement to check whether the job has a translator in JobItem::addTranslatedData()
miro_dietiker’s picture

Status: Needs review » Fixed

Look good to me!

  • miro_dietiker committed 1f66a6f on 8.x-1.x authored by tduong
    Issue #2574877 by tduong, dragos.dumitrescu: JobItemForm don't check if...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.