Would it make sense to delete the job item that (for example) represents a node when that node is deleted? We won't be able to store the translation after that anymore anyways. Or do we want to inform the user that the source object is gone when he/she tries to save that translated job item? Maybe it would be cool to have a drupal_set_message whenever a source object is deleted that represents a job item. Or a watchdog entry.. Dunno. That way the user would have the chance to try and cancel the job that is working on that job item to save money.

Comments

berdir’s picture

I don't think we should delete automatically. Maybe a way to notify the user of running jobs when trying to delete a source through the UI (kinda hard to detect for e.g. i18n_string stuff) or just some kind of availability check for sources when listing items.. we already discussed this before, for various reasons.

vkosenko’s picture

Component: Code » Core
StatusFileSize
new5.37 KB

The patch adds some validation to the node deletion process and checks if the node to be deleted has any translation job items.
If the node has active translation job items, then an error message will appear saying that it is not possible to delete the node because it has some active translation jobs. A list of the active jobs (with links to the jobs) will be displayed as well.
In case if the node has no active job items, but still has some jobs with other status types (Needs Review, Accepted), then a warning message will appear informing the user about the fact and offering an option (a checkbox) to delete the items as well.
If the node has no job items of any kind, the usual node deletion process will take place.

berdir’s picture

Status: Active » Needs review
berdir’s picture

+++ b/sources/node/tmgmt_node.moduleundefined
@@ -1,4 +1,4 @@
-<?php
+  <?php

Looks like you added a space here.

+++ b/sources/node/tmgmt_node.moduleundefined
@@ -24,3 +24,155 @@ function tmgmt_node_tmgmt_source_plugin_info() {
+  while ($record = $result->fetchAssoc()) {
+    $str_return .= '<li><a href="/admin/config/regional/tmgmt/jobs/' . $record['tjid'] . '">' . $record['label'] . '</a></li>';
+  }
+  if ($str_return != '') {
+    $str_return = '<ol>' . $str_return . '</ol>';

This should return an array and then we should probably use a list theme function to display it.

+++ b/sources/node/tmgmt_node.moduleundefined
@@ -24,3 +24,155 @@ function tmgmt_node_tmgmt_source_plugin_info() {
+  if (count($arr_job_items) != 0) {
+    $str_return = implode(', ', $arr_job_items);

Same here, no need to implode, API functions should return an array.

vkosenko’s picture

All issues have been fixed. Please have a look at the patch now.

miro_dietiker’s picture

A job that left the client system, has some transactions with real payments pending.
Thus, the items should no more be removed... Instead we should mark them as abandoned.

vkosenko’s picture

I'd like to point out once again that if a node has active jobs, it will not be possible to delete that node at all (as well as the jobs). A node (and the jobs that refer to the node) can only be deleted if the node has no active jobs.

miro_dietiker’s picture

I think that resulting behavior (banning deletes of nodes) is wrong. While warning with required confirmation seems right.
I'm fine to allow people to delete nodes anytime.

vkosenko’s picture

Would it be ok, if I (instead of blocking the deletion of a nodes) add a new job item state named "Abandoned" (the way you recommend), so that all active job items get the Abandoned state and the node can be deleted without any problem, displaying just a warning message that the state change will be performed?

berdir’s picture

We have the job status "Cancelled" already, we could add the same for job items?

Cancelled in theory also works with active jobs that are being translated, but it depends on the translation provider if they support it and nobody does that yet AFAIK.

vkosenko’s picture

So, I guess we have to select one of the two options:

Option A: We disallow to delete a node that has active job items (the way it is now in the patch). The problem lies in the fact that if we do allow a node to be deleted having some active job items, then it will not be possible to finish the job - ever!

Option B: We add the "Cancelled" job item status, so that all active job items must be set cancelled. And only after all active items have been cancelled successfully, the node can be deleted. In case if any of the active items fails to be cancelled, the node is not deleted. We then could either roll back the cancelled items to the previous state or leave them cancelled.

miro_dietiker’s picture

I would expect that, deleting a node with active job items just leads to "cancelled" job items.
Regarding the completion of a job: Cancelled items should be excluded from the overall stats and for sure, if a job contains 10 items, 8 are accepted and 2 cancelled, it is perfectly complete.

vkosenko’s picture

So, now the patch works this way:

if the node we are trying to delete has active job items, then a warning message will be shown saying that the node has some active items and that their status will be set to 'Cancelled' if the node gets deleted. A list of the corresponding job items will be displayed as well. A checkbox with the information on what items (including their quantity) belong to the node will also be displayed on the screen. If the user checks it, all items will be deleted (the active items will be marked as 'Cancelled' and then deleted).

if the node has no active job items, then a warning message will tell the user that the node has some job items (non-active). The same checkbox allowing to delete the items, will be here as well.

berdir’s picture

+++ b/includes/tmgmt.plugin.incundefined
@@ -321,6 +321,17 @@ interface TMGMTTranslatorPluginControllerInterface extends TMGMTPluginBaseInterf
+  public function cancelTranslationItem(TMGMTJobItem $job_item);

@@ -441,6 +452,22 @@ abstract class TMGMTDefaultTranslatorPluginController extends TMGMTPluginBase im
   public function cancelTranslation(TMGMTJob $job) {
     // Assume that we can cancel a translation job at any time.
     $job->setState(TMGMT_JOB_STATE_CANCELLED);
+
+    // Setting the state of all job items to 'Cancelled'.
+    $job_items = $job->getItems();
+    foreach ($job_items as $job_item) {
+      $job_item->cancelTranslationItem();
...
+  public function cancelTranslationItem(TMGMTJobItem $job_item) {
+    // Assume that we can cancel a translation job item at any time.
+    $job_item->setState(TMGMT_JOB_ITEM_STATE_CANCELLED);
     return TRUE;

That is a bit tricky. Having two methods could be complicated to implement for translators. Especially given that cancelTranslationItem() is called on all items if a job is cancelled so there are two different situations. Not sure what we should do with that.

Maybe not call within cancelTranslation() and only set the job item status directly?

+++ b/sources/node/tmgmt_node.moduleundefined
@@ -24,3 +24,220 @@ function tmgmt_node_tmgmt_source_plugin_info() {
+    'tmgmt_node_list' => array(
+      'variables' => array('jobs'),
+      'function' => 'theme_tmgmt_node_list',

Not sure we need those theme functions.

The chance that someone really wants to customize those seems to be close to zero to me :) And even if, we could do something like theme('item_list__tmgmt_node') which should work out of the box. But even that I'm not convinced we need :)

+++ b/sources/node/tmgmt_node.moduleundefined
@@ -24,3 +24,220 @@ function tmgmt_node_tmgmt_source_plugin_info() {
+    $items[] = l($job_item->data['node_title']['#text'], 'admin/config/regional/tmgmt/items/' . $job_item->tjiid);

You can use $job_item->getSourceLabel() and $job_item->getSourceUri() (returns $uri array of which you need $uri['path'])

vkosenko’s picture

@Berdir: thanks a lot for your review and valuable comments!

1) my idea was that there could be some translators that could use the cancelTranslationItem()

2) removed custom theming for the items output - using the standart theme_item_list instead

3) updated the way the links to the items are formed (using defaultUri() because I need the path to the job item, getSourceUri() would give me the path to the node itself):
$uri = $job_item->defaultUri();
l($job_item->getSourceLabel(), $uri['path']);

miro_dietiker’s picture

Deleting is not OK. We should be able to mark it as deleted - at least after submission.
A job is similar to a commerce order. Even if the product disappears, the order still happened. Including the product in it.

miro_dietiker’s picture

Issue summary: View changes
Status: Needs review » Needs work