Never allow duplicate submission of an item while in process

Assigned to me.

Initial patch. Not sure is this the right direction..

We have an API for this? tmgmt_job_item_load_latest()

Without test..

Work in progress..

You mean the bug that's fixed here: #2685479: Active items of continuous jobs not showing up on the sources overview?

Work in progress..

..and screenshot.

Without solution/part for 'Cart' and without test..

Fixed failing test..

Fixed bugs from #22, #23, covered 'Cart' but without test..

Improved code, but I'm not sure how to fix tests.

+++ b/src/Form/JobForm.php
@@ -525,6 +545,17 @@ class JobForm extends TmgmtFormBase {
+        drupal_set_message(\Drupal::translation()->formatPlural($num_of_items, '1 conflicting item have been dropped.', '@count conflicting items have been dropped.'));

I think here it should be '1 conflicting item has been dropped'.

Also the problem with the tests is that we were submitting twice an item while it was in progress, it had to fail but I wonder why when we dropped the conflicting items we allow to create empty jobs? maybe would be better to discuss that too.

1. +++ b/src/Form/JobForm.php
   @@ -208,6 +212,21 @@ class JobForm extends TmgmtFormBase {
   +        '#prefix' => '<div class="messages existing_items messages--warning hidden">',
   ...
   +        $form['message']['#prefix'] = '<div class="messages existing_items messages--warning">';
   

   Does "existing_items" needs to be "existing-items"?

2. +++ b/src/Form/JobForm.php
   @@ -416,6 +435,7 @@ class JobForm extends TmgmtFormBase {
   +
   

   Extra line, unrelated.

3. +++ b/src/Form/JobForm.php
   @@ -525,6 +545,17 @@ class JobForm extends TmgmtFormBase {
   +      $ids_of_existing_items = $this->checkoutExistingItems($form_state->get('check_target_language'));
   

   What about $existing_items_ids?

4. +++ b/tmgmt.module
   @@ -177,6 +177,50 @@ function tmgmt_job_item_load_latest($plugin, $item_type, $item_id, $source_langu
   +    $return = array();
   ...
   +    return $return;
   

   Maybe $job_items instead of $return for the variable name.

Regarding failing tests,
1. Line 228 (TmgmtUiTest), looks like "Save" on empty translation will not actually save a translation which results that "Save as completed" does not appear.
2. Line 351 (TmgmtUiTest) - there is a wrong label. This happens when there are no job items in the job... Since this issue prevents adding duplicated items, the job label should be either updated or tests changed.

Btw, with dropping job items, we can have a situation where we create "empty" jobs (jobs with no job items)

The message looks a bit contradictory in that case... Not sure if we want to prevent job creation in case there is only one (duplicated) job item. Could be a followup.

Creating an empty jobs, leads to something like:

Fixed mistakes and bugs from #28 and #29.

The test fail is caused by an old test, maybe we should delete those lines since the progress bar is already being tested somewhere else.

// Test that progress bar is being displayed.
$this->assertRaw('class="tmgmt-progress-pending" style="width: 50%"');
$this->assertRaw('class="tmgmt-progress-translated" style="width: 100%"');

In TMGMTUiReviewTest.

Added tests for other parts of new code and fixed bug in existing test.

I knew this was surprisingly easy on tests. The patch did remove conflicting fine enough, but it also ended up happily submitting completely empty jobs.

I refactored the check so that we can fail validation if that happens. And unsurprisingly, tests explode all over the place. Started fixing some, lets see how many fails are left.

Fixed that failing test too. Added another node so that we are testing the case where one item is deleted.

Not done yet, don't really understand \Drupal\tmgmt\Entity\Job::getConflictingItemIds() yet.

@Berdir, do I need to check this or you will continue with it?

I'm on it.

I think I understand what you were trying to do with the second last query. But that assumed that the one you're checking is the one that was changed last. That's not guaranteed.

What I didn't understand is all the logic with check target language and why you did two different checks. There is *always* a target language.

So I tried to simplify this. Only did manual testing now, lets see about tests.

Fixed the tests. The previous code also matched on inactive, not yet submitted job items, the cart test relied on that incorrect behavior. Fixed the test.

Will commit this when green I think. Did quite a lot of testing and this feels solid now.

Committed.