Store input format for message templates #140

jhedstrom · 2016-10-18T18:41:43Z

Fixes Input format isn't stored with message templates #128

Related #139

jhedstrom · 2016-10-19T20:57:21Z

@amitaibu ready for review!

amitaibu

Thank you so much for providing such awesome PRs!

I've added a few questions and comments.

I have tried to create a message-template in Hebrew. It worked, however I didn't find how to export the Hebrew version. What am I'm missing?

amitaibu · 2016-10-20T18:29:10Z

src/Entity/MessageTemplate.php

@@ -253,9 +262,23 @@ public function getText($langcode = Language::LANGCODE_NOT_SPECIFIED, $delta = N
      $text = $translated_text ?: [];
    }

+    // Process text format.
+    foreach ($text as $key => $item) {
+      // Call the renderer directly instead of adding a dependency on the Filter


is this to avoid the a hard dependency on filter module?

Yeah, although I've added the text module as a dependency, which in turn depends on filter, but I'm inclined to leave this as-is for easier unit testing (otherwise, for any unit test, we'd need to redeclare that function in the test).

amitaibu · 2016-10-20T18:30:58Z

src/Entity/MessageTemplate.php

@@ -282,7 +305,9 @@ public static function create(array $values = []) {
   * {@inheritdoc}
   */
  public function preSave(EntityStorageInterface $storage) {
-    $this->text = array_filter($this->text);
+    $this->text = array_filter($this->text, function ($a) {


I'm not totaly clear what we do here. also I think $a should get a more descriptive name. maybe $delta, or $partial?

btw, would that be a good time to rename the word partial to delta? 😄

I've fixed this (not yet pushed). I think partial makes more sense than delta, which is fairly generic.

amitaibu · 2016-10-20T18:31:58Z

src/Form/MessageTemplateForm.php

+    });
+    // Do not store weight, as these are now sorted.
+    $text = array_map(function ($a) {
+      unset($a['_weight']); return $a;


should be in separate lines

is this a common practice? What's the down side of keeping the weight

(just asking, sorting before saving looks fine)

Initially during testing, I was having problems storing weight (without running into schema exceptions). Since the text_format property is a compound one (consisting of value and format), I wasn't immediately clear on how to store a third property side-by-side with those in terms of schema declaration. I could add a @todo here to eventually store weight? However, since they are sorted prior to storage, I'm not sure storing weight buys us anything.

Oh, and also, using _weight added some magic to the form dragging--when I renamed it to just weight, the dragging broke, and I didn't dive into why that was the case.

amitaibu · 2016-10-20T18:34:35Z

src/FormElement/MessageTemplateMultipleTextField.php

+   * @param int $delta
+   *   Delta for the element.
+   * @param array $text
+   *   Array containing 'value' and optionally 'format' for a text_format


When is the case we won't a format?

When adding a new template, there won't be an existing format set.

// Increase number of elements if requested, or none exist. $trigger_element = $form_state->getTriggeringElement(); if (!empty($trigger_element['#add_more']) || !$start_key) { $form['text'][] = $this->singleElement($start_key + 1, ['value' => '']); }

The call to filter_default_format() could be in this code instead of in the singleElement method...

amitaibu · 2016-10-20T18:34:56Z

src/FormElement/MessageTemplateMultipleTextField.php

+   *   element.
+   *
+   * @return array
+   *   The single form element.


nitpick: A Single ..

amitaibu · 2016-10-20T18:36:54Z

tests/src/Kernel/MessageTemplateCreateTrait.php

@@ -35,6 +35,20 @@ protected function createMessageTemplate($template, $label, $description, array
        'clear' => FALSE,
      ],
    ];
+
+    // If the $text array is simple text values, transform to text + format.


not clear why $detail['format'] can be empty

In this case, we could require all callers to specify a format, but since this is a trait for making testing easier, I initially decided to support the legacy input of just an array of text strings:

$text = [ 'text string one', 'text string two', ];

as opposed to updating all callers to use the now more verbose input:

$text = [ [ 'value' => 'text string one', 'format' => 'some_format', ], ... ];

amitaibu · 2016-10-20T18:40:17Z

tests/src/Unit/Entity/MessageTemplateTest.php

+    // Language without translation should return empty array.
+    // @todo Is this correct? Why not return untranslated text?
+    $default_language = $this->prophesize(Language::class);
+    $mock_template = $this->prophesize(MessageTemplate::class);


$mock_template => $message_template

amitaibu · 2016-10-20T18:42:22Z

tests/src/Unit/Entity/MessageTemplateTest.php

+    $this->assertEquals($expected, $this->messageTemplate->getText());
+
+    // Language without translation should return empty array.
+    // @todo Is this correct? Why not return untranslated text?


The behavior is probably an edge case, but I think it right. If for example you have activity stream in English and Hebrew, and the dev didn't translate a template from Hebrew (default language) to English - you, as a non-Heberw speaker, will prefer not to see anything rather than the hebrew.

On the other hand, I can teach you some Hebrew 😜

amitaibu · 2016-10-20T18:44:16Z

tests/src/Unit/Entity/MessageTemplateTest.php

+   *
+   * @covers ::getText
+   */
+  public function testGetText() {


wow, it's really nice to see this as unit test!

I think it's worth spliting it to smaller methods, as right now i kind of checks everything 😄

jhedstrom · 2016-10-20T21:45:42Z

I have tried to create a message-template in Hebrew. It worked, however I didn't find how to export the Hebrew version. What am I'm missing?

I didn't know either, but asked Alex Pott and he showed me that if you run, for instance drush config-export, the translations appear in a subdirectory called language. If you look at a translated message, you'll see that only the translatable parts are exported:

gP6UkhH6Auo_sNDWtBHqxnTRmoFo2K85SXIivKS3QinnkEg8hg/sync/language/he/message.template.bb.yml
description: translated text here
text:
  -
    value: "<p>TRANSLATION 1</p>\r\n"
    format: basic_html
  -
    value: "<p>TRANSLATION 2</p>\r\n"
    format: restricted_html

- Fixes #128

- Fixes #139

jhedstrom · 2016-10-24T17:10:31Z

@amitaibu should be ready for another review!

amitaibu · 2016-10-27T17:28:22Z

I haven't forgotten about this, just missing the time. I'll pickup in a couple of days.

jhedstrom · 2016-10-31T16:44:56Z

Bump =)

amitaibu · 2016-10-31T16:57:37Z

Sorry! Just hectic days. So in order for me not to block - go ahead and
merge all you awesome PRs. I can revisit if needed post-mortem.

On Oct 31, 2016 6:45 PM, "Jonathan Hedstrom" notifications@github.com
wrote:

Bump =)

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#140 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAHrC9wFykh89E9UbKCyRfxZgkzOVlOpks5q5hsIgaJpZM4KaJWO
.

jhedstrom · 2016-10-31T17:49:19Z

Sounds good. File follow-ups as needed :)

jhedstrom added the bug label Oct 18, 2016

jhedstrom added this to the 8.x-1.0 beta milestone Oct 18, 2016

jhedstrom mentioned this pull request Oct 18, 2016

Remove customized config translation handling #139

Closed

jhedstrom force-pushed the 128-text-format branch from 03b66a1 to e963c05 Compare October 19, 2016 18:48

jhedstrom added Ready for review Release blocker labels Oct 19, 2016

jhedstrom mentioned this pull request Oct 19, 2016

Add kernel tests to Message::getText #120

Closed

jhedstrom force-pushed the 128-text-format branch from b8a4404 to a4cc1a3 Compare October 19, 2016 23:50

jhedstrom mentioned this pull request Oct 19, 2016

Kernel tests for Message entity #141

Closed

amitaibu reviewed Oct 20, 2016

View reviewed changes

jhedstrom removed the Ready for review label Oct 20, 2016

jhedstrom added 2 commits October 24, 2016 09:29

Stores message template text with format.

65925ef

- Fixes #128

Removes custom config translation handling.

9a15cfb

- Fixes #139

jhedstrom force-pushed the 128-text-format branch from a4cc1a3 to f6b8d93 Compare October 24, 2016 17:04

Unit tests for MessageTemplate::getText()

1d29686

jhedstrom force-pushed the 128-text-format branch from f6b8d93 to 1d29686 Compare October 24, 2016 17:09

jhedstrom added the Ready for review label Oct 24, 2016

This was referenced Oct 27, 2016

Keep up with Message changes Gizra/message_notify#40

Closed

Keep up with Message changes. Gizra/message_notify#41

Merged

Keep up with Message changes Gizra/message_subscribe#42

Closed

Fixed flag schema errors. Gizra/message_subscribe#43

Merged

jhedstrom merged commit d36d044 into 8.x-1.x Oct 31, 2016

jhedstrom mentioned this pull request Oct 31, 2016

Revert "Store input format for message templates" #147

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Store input format for message templates #140

Store input format for message templates #140

jhedstrom commented Oct 18, 2016 •

edited

jhedstrom commented Oct 19, 2016

amitaibu left a comment

amitaibu Oct 20, 2016

jhedstrom Oct 24, 2016

amitaibu Oct 20, 2016

amitaibu Oct 20, 2016

jhedstrom Oct 24, 2016

amitaibu Oct 20, 2016

amitaibu Oct 20, 2016

jhedstrom Oct 24, 2016

jhedstrom Oct 24, 2016

amitaibu Oct 20, 2016

jhedstrom Oct 24, 2016

amitaibu Oct 20, 2016

amitaibu Oct 20, 2016

jhedstrom Oct 24, 2016

amitaibu Oct 20, 2016

amitaibu Oct 20, 2016

amitaibu Oct 20, 2016

jhedstrom commented Oct 20, 2016

jhedstrom commented Oct 24, 2016

amitaibu commented Oct 27, 2016

jhedstrom commented Oct 31, 2016

amitaibu commented Oct 31, 2016

jhedstrom commented Oct 31, 2016

Store input format for message templates #140

Store input format for message templates #140

Conversation

jhedstrom commented Oct 18, 2016 • edited

jhedstrom commented Oct 19, 2016

amitaibu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhedstrom commented Oct 20, 2016

jhedstrom commented Oct 24, 2016

amitaibu commented Oct 27, 2016

jhedstrom commented Oct 31, 2016

amitaibu commented Oct 31, 2016

jhedstrom commented Oct 31, 2016

jhedstrom commented Oct 18, 2016 •

edited