Closed (fixed)
Project:
Translation Management Tool
Version:
8.x-1.x-dev
Component:
User interface
Priority:
Normal
Category:
Bug report
Assigned:
Unassigned
Reporter:
Created:
25 Sep 2015 at 14:30 UTC
Updated:
20 Nov 2015 at 16:14 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #2
edurenye commentedComment #3
dragos-dumi commentedAttached the patch for double encoding.
Comment #4
edurenye commentedThis is fine.
Remember to change the status to Needs review when you upload a patch to make the test run.
There's more work in this issue: Also add the two new columns: Translation and Status
And we need a test to ensure that this is not going to change again.
Also when finished remember to add a screenshot as this is a UI change.
Comment #5
miro_dietikerSounds like a mixed issue...
First it is a bug (this issue) to make the label work properly, with the patch proposed. No screenshot needed here.
But adding extra columns should be a separate feature issue.
Comment #6
edurenye commentedOk, you are right, those things are really different, I created a issue for adding the columns #2608070: Add Translation and Status columns in ConfigTranslateForm
But this still needs tests.
Comment #7
edurenye commentedComment #8
tduong commentedprovided test coverage for label encoding
Comment #10
edurenye commentedThats fine as a test only patch.
But when you add tests you have to add first a patch with the same name but ending with test_only.patch and then a patch with both solution and test in the same patch.
And both have to be in the same comment.
https://www.drupal.org/contributor-tasks/write-tests
Comment #11
sasanikolic commentedI don't think this test makes sense. You should assert if there is no Source inside strong HTML tags present in the table (specific: 1st row/3rd column) and that there is only "Source" now.
Comment #12
berdirthe assertRaw() is doing exactly that, it ensures that the
<strong>tags are not escaped.Yes, we could extend it to check the specific row where it is shown, but this would IMHO be enough for a regression test of this specific issue.
Comment #13
tduong commentedOk, thanks for the suggestions :)
Uploaded patches:
Comment #16
berdirthat patch won't apply, the test change is an interdiff against the previous patch. And I don't think that change is even necessary.
Comment #17
tduong commentedOk, now it should be correct ^^'
Updated patches.
Comment #19
tduong commentedchanged patches order
Comment #21
berdirThanks, committed.
Comment #23
berdir