Problem/Motivation

In the job item form, after reviewing an item the messages display the link to the reviewed item wrong. This is due to a missing markup.

EDIT: Also due to the latest core changes, the popup message is also broken. This is breaking the current tests.

Proposed resolution

add the markup and test coverage.

Remaining tasks

create patch, Review and commit.

User interface changes

Image

Data model changes

Original report by [username]

Comments

juanse254 created an issue. See original summary.

juanse254’s picture

Status: Active » Needs review
StatusFileSize
new1.25 KB
new1.6 KB

And this should fix it.

juanse254’s picture

StatusFileSize
new875 bytes

Hum sorry, this is the right TEST-Only.

The last submitted patch, 2: Ajax_error-2540696-21-TEST_ONLY.patch, failed testing.

The last submitted patch, 2: Hmtl_tags-2560951-2.patch, failed testing.

juanse254’s picture

Issue summary: View changes
StatusFileSize
new82.11 KB

Status: Needs review » Needs work

The last submitted patch, 3: Hmtl_tags-2560951-2-TEST-ONLY.patch, failed testing.

The last submitted patch, 2: Hmtl_tags-2560951-2.patch, failed testing.

The last submitted patch, 3: Hmtl_tags-2560951-2-TEST-ONLY.patch, failed testing.

juanse254’s picture

Status: Needs work » Needs review
StatusFileSize
new3.86 KB
new2.86 KB

That was not working, this is a better aproach.

The last submitted patch, 12: Html_tags-2560951-12.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 12: Html_tags-2560951-12-TEST-ONLY.patch, failed testing.

The last submitted patch, 12: Html_tags-2560951-12.patch, failed testing.

juanse254’s picture

Status: Needs work » Needs review
StatusFileSize
new4.22 KB
new1.16 KB

The test remains the same as in patch #12, as you can see in the interdiff.

Status: Needs review » Needs work

The last submitted patch, 17: Html_tags-2560951-17.patch, failed testing.

juanse254’s picture

Status: Needs work » Needs review
edurenye’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new60.11 KB

The code seems fine and it works fine.
I'm providing an snapshot of the result:

berdir’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/src/Entity/JobItem.php
@@ -715,12 +716,23 @@ class JobItem extends ContentEntityBase implements JobItemInterface {
+          if (!$this->getSourceUrl()) {
+            $source_url = array(
+              '@source_url' => (string) $job_url,
+            );
+          }
+          else {
+            $source_url = array(
+              '@source_url' => $this->getSourceUrl()->toString(),
+            );
+          }
           $variables = array(
-            '@source' => $this->link($this->getSourceLabel()),
+            '@source' => $this->getSourceLabel(),
             '@language' => $this->getJob()->getTargetLanguage()->getName(),
-            '@review_url' => $this->url('canonical', array('query' => array('destination' => $job_url))),
+            '@review_url' => Url::fromUserInput($job_url)->toString(),
           );
-          $this->needsReview('The translation of @source to @language is finished and can now be <a href="@review_url">reviewed</a>.', $variables);
+          $variables = array_merge($source_url, $variables);

This seems more complicated than it has to be. Instead of creating two arrays and merging them, why don't you just move the if/else below and just set the additional key in there? At that point, it might be simple enough to do it as a ternary?

juanse254’s picture

Status: Needs work » Needs review
StatusFileSize
new3.94 KB
new1.24 KB

You are right, this should work and save some lines.

giancarlosotelo’s picture

Status: Needs review » Needs work

The patch looks good but I tested locally and some links are not working.
You should also fix the link in.

 $message = 'The translation for @source has been accepted.';
 $variables = array('@source' => $url ? \Drupal::l($this->getSourceLabel(), $url) : $this->getSourceLabel());

and add some test for that.

juanse254’s picture

Status: Needs work » Needs review
StatusFileSize
new6.79 KB
new2.76 KB

This might solve it, some tests needed for the changes.

giancarlosotelo’s picture

Issue tags: +Needs tests

Now looks good! but we need tests.

giancarlosotelo’s picture

Status: Needs review » Reviewed & tested by the community
edurenye’s picture

Status: Reviewed & tested by the community » Needs work

If it needs tests, should go to needs work.

juanse254’s picture

Status: Needs work » Needs review
StatusFileSize
new7.49 KB
new586 bytes

This is the only way to test that, couldn't test the 'being processed' because it doesn't seem to be used anywhere.

juanse254’s picture

Priority: Normal » Major
StatusFileSize
new8.08 KB
new3.71 KB

This will fix both breaking tests and this issue, had to put them together since they are strictly related as the fail in the same message. Im also promoting to major since tests area failing until this gets fixed.

juanse254’s picture

Issue summary: View changes

Status: Needs review » Needs work

The last submitted patch, 30: Html_tags-2560951-30.patch, failed testing.

The last submitted patch, 30: Html_tags-2560951-30.patch, failed testing.

juanse254’s picture

Status: Needs work » Needs review
StatusFileSize
new12.91 KB
new10.57 KB
edurenye’s picture

Status: Needs review » Reviewed & tested by the community

There is a tabulation that should not be there, but this issue fixes the head, and as it passes the tests and the code seems fine.
I set it to RTBC, as should be commited asap and maybe delete the tabulation on commiting.

juanse254’s picture

Status: Reviewed & tested by the community » Needs review
StatusFileSize
new9.64 KB
new4.39 KB

Im sorry that patch contained an unrelated interdiff. This is the one.

juanse254’s picture

StatusFileSize
new8.97 KB
new482 bytes

Omg, im sorry a new patch got committed meanwhile and i forgot to rebase.

giancarlosotelo’s picture

Status: Needs review » Reviewed & tested by the community

Ok it seems that the head is fixed.

berdir’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: -Needs tests

Great, committed.

  • Berdir committed 10b445c on 8.x-1.x authored by juanse254
    Issue #2560951 by juanse254: Fixed Html tags showing instead of link in...

The last submitted patch, 29: Html_tags-2560951-29.patch, failed testing.

The last submitted patch, 30: Html_tags-2560951-30.patch, failed testing.

The last submitted patch, 29: Html_tags-2560951-29.patch, failed testing.

The last submitted patch, 36: Html_tags-2560951-36.patch, failed testing.

Status: Fixed » Closed (fixed)

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