Allow updates for needs review translations and introduce a #customized flag

+++ b/includes/tmgmt.entity.incundefined
@@ -845,7 +845,7 @@ class TMGMTJobItem extends Entity {
-      if (!$status || $status == TMGMT_DATA_ITEM_STATE_PENDING) {
+      if (!$status || $status == TMGMT_DATA_ITEM_STATE_PENDING || $status == TMGMT_DATA_ITEM_STATE_PENDING) {

Close but not quite ;)

We also need to look into this next week.
The valid update workflows should be defined and well covered by the different plugins.

Partial proof of concept patch that introduces simple revision handling for data item translations.

Let's trigger bot review.

Retrying.

There's still no #customized flag and no user interface.
But there's much more message explanation logging and persistence of data now.

Yay, retry ;-)

A field that is customized should be locked for further incoming.

An extra button "revert" (if customized) should drop the customization and go back to the lates incoming translation.

+++ b/entity/tmgmt.entity.job_item.incundefined
@@ -501,8 +508,40 @@ class TMGMTJobItem extends Entity {
+          // Load existing revisions.
+          if (!empty($data['#translation']['#text_revisions'])) {
+            $translation['#text_revisions'] = $data['#translation']['#text_revisions'];

This is a bit ugly but not sure if there's a different way.

Maybe by not overriding numerical keys below a #property?

+++ b/entity/tmgmt.entity.job_item.incundefined
@@ -501,8 +508,40 @@ class TMGMTJobItem extends Entity {
+            $this->addMessage('Translation for @key received. Revert your changes if you wish to use it.', array('@key' => tmgmt_ensure_keys_string($key)));

We need to extend the messages to be able to log messages for specific data items, then we can directly link to it and things like that. Not to be solved here.

+++ b/entity/tmgmt.entity.job_item.incundefined
@@ -501,8 +508,40 @@ class TMGMTJobItem extends Entity {
+            $diff = strlen($translation['#text']) - strlen($data['#translation']['#text']);

Should use drupal_strlen() so that UTF-8 characters are correctly counted.

+++ b/entity/tmgmt.entity.job_item.incundefined
@@ -501,8 +508,40 @@ class TMGMTJobItem extends Entity {
+        // If the key is among customized items set the flag and remove from
+        // the pool.
+        if (in_array($key, $this->data_items_customized)) {
+          $this->data_items_customized = array_diff($this->data_items_customized, array($key));
+          $translation['#customized'] = TRUE;

@@ -511,12 +550,53 @@ class TMGMTJobItem extends Entity {
+  public function dataItemLocallyCustomized(array $key) {
+    $this->data_items_customized[] = $key;

+++ b/ui/tmgmt_ui.moduleundefined
@@ -629,9 +665,15 @@ function tmgmt_ui_translation_review_form_submit($form, &$form_state) {
+      if (!isset($data['#translation']['#text']) || $data['#translation']['#text'] != $value['translation']) {
+        $item->dataItemLocallyCustomized(tmgmt_ensure_keys_array($key));

Not sure I understand what this part is doing.

Why not directly set #customized in this case?

I was also wondering if that's something that would be handled directly in the API but might not be necessary.

+++ b/ui/tmgmt_ui.moduleundefined
@@ -563,6 +563,22 @@ function _tmgmt_ui_review_form_element($data, TMGMTJobItem $job_item, &$zebra) {
+        if (!empty($data[$key]['#translation']['#customized'])) {
+          $form[$target_key]['actions']['revert'] = array(
+            '#type' => 'submit',
+            // Unicode character U+21B6 ANTICLOCKWISE TOP SEMICIRCLE ARROW
+            '#value' => '↶',
+            '#attributes' => array('title' => t('Revert')),
+            '#name' => 'revert-' . $target_key,
+            '#data_item_key' => $key,
+            '#submit' => array('tmgmt_ui_translation_review_form_revert'),
+            '#ajax' => array(
+              'callback' => 'tmgmt_ui_translation_review_form_ajax',
+              'wrapper' => $form[$target_key]['#ajaxid'],
+            ),
+          );

Right now we just display it for #customized and it's not possible to actually see the updated revision before reverting it, right?

That is related the issue where we want to have an official space below data items to display revisions, comments and so on. Then the revert action should probably be displayed on the list of revisions (together with a diff action if diff.module is installed :))

We don't need to be fancy in a first version but not showing anything at all makes this a rather useless feature :)

As a first step, just being to able to
- see yes it is cusomized
- drop customization (and thus revert to last incoming revision)
I thought is perfectly fine.

Extending the UI with interaction of the revisions i would consider a followup task.
I guess we first need a new round to improve the mockup for the revision part before coding anything.

Yes, we discussed it and disagreed :)

The button would allow you to revert, but without any chance to see to what you would revert to or to go back to your own customizations after you reverted it. Why would you want to use such a thing? :)

;-) Let's see at some cases then.

A) I get something from the origin .... i accept it right away.
If then revisions come in - oops anyway. Yes... diff please.

B) I got something, changed it and accepted it.
If then revisions come in - doubleoops. A diff between incoming revision is one thi ng to see what the translator improved.
A diff to my current version shows my improvements. We need a three way merge.

C) I'm not happy with what came in, requesting for revision.
(e.g. Gengo) provided a revision. It gets automerged as long as uncustomized.
NO UI needed.

D) I was not happy with the work, started improving it and still asked for revision.
Something comes in but is not merged.
Like with B, a three way merge is needed: Merge diff from translator (before to after) with my current version.

E) If i started improving and an update comes in witout any request.
Same as D.

Generally, agree with Berdir. We need that merge/diff UI to make a good job when customization happened.
The first important with it was to clearly stop overwriting customized text (or do overwriting if uncustomized).
And a first visual note that something is be outdated.

I'm looking forward to your full specification then. :-)

A) / B) isn't handled yet, as we still ignore that. I guess we shouldn't and instead handle it similar to customized data items, store it as a new revision but keep the custom one active.

C) Why not? Yes, it is automerged but might still want to see what was actually changed. Especially in a longer text, just like I like reviewing interdiff's of longer patches instead of the whole thing :) If we have a UI for the other cases, we get one for this for free anyway.

D) / E) Well three way merges and stuff like that are fancy ideas that we can implement when we're bored and no longer know what we can do with our free time :)

As discussed with @blueminds, we're just going to display a (by default collapsed) table with revisions (with date, text and possibly information if it was a customized/diffstat) if there are any and display the revert button within that list for.

Diff's, merges, ... are for follow-ups, all I want for a first step is being able see the text I'm reverting to before I'm doing it (and would then be unable to go back again, which is how the current patch works). We could just as well add a "shuffle the characters around" button, would be similarly useful ;)

#24: tmgmt-translation_revisions-1898686-13.patch queued for re-testing.

Nice work, like what I see. Don't worry about the somewhat long review, various things can be follow-up's and the other should be easy to change, just elaborate thinking on my side ;)

+++ b/entity/tmgmt.entity.job_item.incundefined
@@ -501,8 +501,47 @@ class TMGMTJobItem extends Entity {
+          // If current translation was created locally and the incoming one is
+          // remote, do not override the local, just create a new revision.
...
+            $this->addMessage('Translation for @key received. Revert your changes if you wish to use it.', array('@key' => tmgmt_ensure_keys_string($key)));

Hm. Apart from the message we actually don't have a way of knowing that we have a data item revision that is newer than the current one. In case we store the date for the current translation as well, that's actually something that we can then calculate from that.

So if the latest revision of a data item is newer than the timestamp of the translation itself, then we can (later, doesn't need to be now) highlight it easly.

+++ b/entity/tmgmt.entity.job_item.incundefined
@@ -501,8 +501,47 @@ class TMGMTJobItem extends Entity {
+              '#timestamp' => $this->changed,
...
+              '#timestamp' => $this->changed,

Hm, that's actually not necessarly correct. That will set the timestamp to whenever the job item was last saved but that doesn't mean that every translation was updated/added then. I think we should set that just like you've set origin above. We can still fall back to $this->changed if we don't have anything else but that should only happen initially for existing jobs.

+++ b/tests/tmgmt.crud.testundefined
@@ -189,6 +189,67 @@ class TMGMTCRUDTestCase extends TMGMTBaseTestCase {
 
+  function testDataItems() {

Test method should have a short one line docblock. Nice test coverage. The only thing that's hard to really test is #timestamp as everything's going to be the same be should be able to add a check that timestamp isn't empty in both revisions and the translation.

And we could possibly use updateData() to fake an older timestamp before adding translated 3, so that we could see that the revision timestamp is newer than the translation timestamp?

+++ b/ui/tmgmt_ui.moduleundefined
@@ -563,6 +563,42 @@ function _tmgmt_ui_review_form_element($data, TMGMTJobItem $job_item, &$zebra) {
+        if (!empty($data[$key]['#translation']['#text_revisions'])) {
+          $form[$target_key]['actions']['revert'] = array(
+            '#type' => 'submit',
+            // Unicode character U+21B6 ANTICLOCKWISE TOP SEMICIRCLE ARROW
+            '#value' => '↶',

The current UI and API isn't going to allow you to revert to the second-last revision. As mentioned before, should we move the revision button to each revision? I think that would also make the icon way more obvious as to what it does. For the API, possibly optionally add the revision key to which you want to revert and default to the latest? Probably another thing for later as we then need to solve the problem of being able to have links in there.. or make it a link. So fine for now if you create a follow-up issue :)

+++ b/ui/tmgmt_ui.moduleundefined
@@ -563,6 +563,42 @@ function _tmgmt_ui_review_form_element($data, TMGMTJobItem $job_item, &$zebra) {
+              '%origin' => $revision['#origin'],

So we have remote/local as implicitly allowed values but they're not translated. Should we add constants for those (we can still use 'remote'/'local' as strings for the actual storage) and add a label function like we have for e.g. statuses so that remote/local has a translatable label? We can also do this is a follow-up. Your choice, create a follow-up issue or implement it ;)

+++ b/ui/tmgmt_ui.moduleundefined
@@ -563,6 +563,42 @@ function _tmgmt_ui_review_form_element($data, TMGMTJobItem $job_item, &$zebra) {
+            '#type' => 'markup',
+            '#markup' => theme('item_list', array('items' => $revisions)),

#type => 'item_list', '#items' => $revisions is actually easier in this case :)

+++ b/ui/tmgmt_ui.moduleundefined
@@ -629,9 +685,12 @@ function tmgmt_ui_translation_review_form_submit($form, &$form_state) {
       $data['#translation'] = array(
         '#text' => $value['translation'],
+        '#origin' => 'local',
       );
+
       $item->updateData($key, $data);

Hm. So once you save the review form, this is going to set everything to local, even if you haven't changed it. I think if we just make two small changes to addTranslatedData(), then we could easily use it here.

- Only apply the don't overwrite logic if $translation origin is remote and $data['#translation'] origin is local, so that a local
- Don't do anything if a translation exists and hasn't been changed.

With that, we a) don't have to check for the second condition here manually and we get revisions for our local changes as well, basically for free :)

I'll finish this.

Turned out there were a few more changes necessary to fix some interesting issues :)

- Translation updates for already reviewed translations are now also accepted and handled the same way as updates for customized translations, as a new revision and a message.
- Tracking #timestamp for translations as suggested above.
- Improved job messages
- The diff message is now only created for remote updates, as saving on the review page now uses the same API
- When addTranslatedData() is called with the same text, no change is done.
- Added a default message within revertDataItem()
- Lots of new tests for timestamp, job messages, not updating when data is the same and so on.
- The revision table broke the ajax handling. Refactored and updated that quite a bit, It replaces the whole table now and there's a wrapper around it. That means a lot less custom stuff necessary, e.g. status messages now just work.
- Also display translation revisions for accepted job items, that wasn't happening because it was within a !isAccepted() check before.
- Unsetting $form_state['input'] for the reverted data item so that the new default value is shown
-

Nice work. Reviewed the interdiff and looks like a great cleanup.

+++ b/ui/tmgmt_ui.moduleundefined
@@ -569,37 +571,38 @@ function _tmgmt_ui_review_form_element($data, TMGMTJobItem $job_item, &$zebra) {
             '#value' => '↶',
-            '#attributes' => array('title' => t('Revert')),
+            '#attributes' => array('title' => t('Revert to previous revision')),

Is this really the "previous version" or is it a specific/this/last version? Or is it always "Drop customization"?

From the full review: the test looks great.

It always reverts to the previous/last revision. As said above, I think it makes more sense to move it to a link on each listed revision (which should possibly be sorted newest/last first?)

Showing all revisions but only being able to switch to the latest feels almost similarly limited to just having the button and not knowing to what you revert ;-P
Finally i consider the only reallly helpful thing a diff, but that's for followups.

So yes, button per version is required and tests to cover switching to old stuff.

I'm just a bit worried about ux in case of big body fields.

:-)

#29: tmgmt-translation_revisions-1898686-29.patch queued for re-testing.

Committed and pushed, opened a follow-up for the UI, as discussed: #2010986: Improve the user interface for revisions and reverting revisions