This task is for a new student in MD Systems, don't do it if it was not assigned to you.

Problem/Motivation

Since this issue was fixed #2646592: Fix duplicated task when saving local task is not possible any more to change the status from this form, when you do so, the task disappears.

So we want to make the status of the local task read-only.

To reproduce that, follow this steps:

  1. Install the tmgmt_demo module, it will install tmgmt_local also
  2. Create a new TMGMT Job submitting it to the Local Translator
  3. Go to Translate in the top-right of the site main page
  4. Click View to enter to the TMGMT local Task

Here you can see that the status is a selector. This is wrong, we want it to be a item, as an item is read-only.

Proposed resolution

Make the status read-only like the rest of items inside the info container.

Remaining tasks

Make the status read-only.

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#18 interdiff-make_status_read_only-2650042-9-18.txt3.33 KBedurenye
#18 make_status_read_only-2650042-18.patch4.44 KBedurenye
#15 2650042-15.png199.93 KBBambell
#15 interdiff-2650042-9-15.txt899 bytesBambell
#9 tmgmt-status_read_only_one_line-2650042-9.patch1.69 KBBambell
#9 tmgmt-status_read_only-2650042-9.patch1.39 KBBambell
#4 tmgmt-status_read_only-2650042-4.patch1.7 KBBambell

Comments

edurenye created an issue. See original summary.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

As discussed, I'm not sure if we should show that field at all. You can't change status in jobs either, it's done automatically based on actions. We should check if there is a valid use case for doing it here and if not, show it as read only.

edurenye’s picture

edurenye commented
Title: Change status in localTaskForm is not working » Make status read only in localTaskForm
Category: Bug report » Task
Issue summary: View changes
Parent issue: » #2646592: Fix duplicated task when saving local task

I agree that there's not valid use cases for doing that and this is also happening in D7, so let's make status read only.
As this is simple, let's save this issue for a new student in MD Systems.

Bambell’s picture

Bambell commented
StatusFileSize
new tmgmt-status_read_only-2650042-4.patch1.7 KB

I changed the "status" from a select element to plain text and moved it into the edit-info container. I had to reduce margin-right of .tmgmt-ui-info-item so that it would all fit on a single line. The patch should be attached to this comment. First contribution to Drupal, hurray :).

Bambell’s picture

Bambell commented
Assigned: Unassigned » Bambell
Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 4: tmgmt-status_read_only-2650042-4.patch, failed testing.

Bambell’s picture

Bambell commented

Just noticed that some unit tests have failed. The first failure apparently has to do with the fact that the original selector could only be accessed by a tmgmt administrator (being read only, I removed the access rights verification for the new element). The second failure apparently has to do with the fact that the tests attempt to set the value property of #edit-status to 0 (unassigned) and POST the form, but the new #edit-status element (a div) obviously has no such property. I'm not entirely sure about the third failure, but it's from the same line as the second. I could try to edit LocalTranslatorTest.php, but I can't seem to find how to run the tests on my local machine to... test the tests.

edurenye’s picture

edurenye commented

Awesome, Good job!
Don't worry now with the tests now, first fix some details:

+++ b/css/tmgmt.admin.css
@@ -5,7 +5,7 @@
 .tmgmt-ui-info-item {
   float: left;
-  margin-right: 80px;
+  margin-right: 70px;
 }

1. This change affects to all tmgmt, so maybe will break other views and it a change not really related to the issue, the items will be allocated depending the screen, it's not a problem if the items are split in two lines.

+++ b/translators/tmgmt_local/src/Form/LocalTaskForm.php
@@ -114,6 +106,14 @@ class LocalTaskForm extends ContentEntityForm {
+	
+	$form['info']['status'] = array(
+	  '#type' => 'item',
+      '#title' => t('Status'),
+      '#markup' => $states[$local_task->getStatus()],
+      '#prefix' => '<div class="tmgmt-ui-info-item">',
+      '#suffix' => '</div>',
+    );

This is correct, just is not well formatted, you should never use tab to do indentation, and the elements inside the array has to be 2 spaces in. Take a look to the Coding standards in Drupal https://www.drupal.org/coding-standards#indenting

Also if you have PhpStorm well configured it can auto-format the code, to fix those errors.

By the way, to test, you need to install simpletest module, and the you can go to 'admin/config/development/testing', but don't fix the test yet.

Bambell’s picture

Bambell commented
StatusFileSize
new tmgmt-status_read_only-2650042-9.patch1.39 KB
new tmgmt-status_read_only_one_line-2650042-9.patch1.69 KB

Thanks for the tips and and suggestions ! I'll go ahead and download PhpStorm. Regarding the "splitting" issue, the content of edit-info exceeds the container width (it will display on two "lines"), for a >1280px screen resolution, only for longer status messages ("Completed" and "Unassigned") and not for shorter ones ("Pending"). That looks a bit aesthetically odd, in my humble opinion, and I felt like I had to address this issue. Leaving the status outside of edit-info container looks a bit odd too. I realized that the class could be used throughout tmgmt; I could find only one other view using .tmgmt-ui-info-item and the change wouldn't break it. Anyway, I have re-uploaded the patch without correction to the styling (tmgmt-status_read_only-2650042-9.patch). I also uploaded a second patch (tmgmt-status_read_only_one_line-2650042-9.patch) with an alternative correction to the styling, specifically for the last element of that type of container. Also, the indenting issue should be fixed now. Sorry about that ! Thanks !

Bambell’s picture

Bambell commented
Status: Needs work » Needs review

The last submitted patch, 9: tmgmt-status_read_only-2650042-9.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 9: tmgmt-status_read_only_one_line-2650042-9.patch, failed testing.

edurenye’s picture

edurenye commented

Remember that when you upload a patch that is based in a patch before, you have to provide an interdiff, that the difference between those patches.
Also when you make a change in the UI you have to provide also an screenshot of the change.

edurenye’s picture

edurenye commented 
+++ b/css/tmgmt.admin.css
@@ -8,6 +8,10 @@
+.tmgmt-ui-info-item-last {
+  float: left;

We already had the "float: left" it in the class tmgmt-ui-info-item, so I'll go for the first patch.

Now the indentation is fine.
Anyway, please provide the interdiff for the first patch and the screenshot.

Bambell’s picture

Bambell commented
StatusFileSize
new interdiff-2650042-9-15.txt899 bytes
new 2650042-15.png199.93 KB

Interdiff between tmgmt-status_read_only-2650042-9.patch and tmgmt-status_read_only_one_line-2650042-9.patch is attached to this message, as well as an explicative screenshot of the difference between those two patches and the current state of the branch. Thanks.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Lets not make any css changes here, we can still improve that. Also note that we want to switch to admin theme, that will change a lot how this looks. No point in spending time on this now.

That said, there's still a test that is broken. We possibly need to simply remove that and replace with a test that checks that the status is displayed correctly.

edurenye’s picture

edurenye commented

This is to much complex for the test we wanted to do, so @Bambell you did it very well you work here is done.
Now I'll provide a patch with the test fixed and also fixing the Unassign action as without the selector we have no way to unassign right now there.

edurenye’s picture

edurenye commented
Status: Needs work » Needs review
StatusFileSize
new make_status_read_only-2650042-18.patch4.44 KB
new interdiff-make_status_read_only-2650042-9-18.txt3.33 KB

Fixed the tests and now you can unassign selecting Unassigned in the tuid selector.
And now the assigned user is the one that shows the selector by default.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Fixed

Looks good to me now.

  • Berdir committed 9f2697f on 8.x-1.x authored by edurenye 
    Issue #2650042 by Bambell, edurenye: Make status read only in...

Status: Fixed » Closed (fixed)

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