From 3c4c2c8fe24a2aa47d7fa219b4aad0ad08889d18 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Samuli=20Sillanp=C3=A4=C3=A4?=
 <samuli.sillanpaa@helsinki.fi>
Date: Mon, 2 Mar 2020 17:19:51 +0200
Subject: [PATCH] Feedbackform improvements. (#1565)

- Add support for configuring help texts to be displayed before
  and/or after the form element.
- Move translation of help texts from template to Form handler
  in order to ease extending.
- Add form group specific CSS-classes.
---
 config/vufind/FeedbackForms.yaml              | 12 +++++-
 module/VuFind/src/VuFind/Form/Form.php        | 38 +++++++++++++++++--
 module/VuFind/src/VuFind/Form/FormFactory.php |  3 +-
 .../src/VuFindTest/Form/FormTest.php          | 24 ++++++++++--
 .../bootstrap3/templates/feedback/form.phtml  | 24 +++++++++---
 5 files changed, 85 insertions(+), 16 deletions(-)

diff --git a/config/vufind/FeedbackForms.yaml b/config/vufind/FeedbackForms.yaml
index 445644fb956..6f5861b91be 100644
--- a/config/vufind/FeedbackForms.yaml
+++ b/config/vufind/FeedbackForms.yaml
@@ -50,10 +50,18 @@
 #     settings (array)   HTML attributes as key-value pairs, for example:
 #       - [class, "custom-css-class another-class"]
 #     type (string)      Element type (text|textarea|email|url|select|radio|checkbox)
-#     help (string)      Element help text (translation key).
+#
+#     help (string)      Element help text (translation key) that is displayed before the element.
 #                        To include HTML formatting, use a translation key ending
 #                        in '_html' here, and define markup in the language files.
-#     
+#
+#       or
+#
+#     help (array)
+#      pre (string)      Like above.
+#      post (string)     Like above but the help text is displayed after the element.
+#
+#
 #     And for select and radio elements:
 # 
 #     options (array) List of select values (translation keys)
diff --git a/module/VuFind/src/VuFind/Form/Form.php b/module/VuFind/src/VuFind/Form/Form.php
index 867b7d83fa4..e12d02aaa1b 100644
--- a/module/VuFind/src/VuFind/Form/Form.php
+++ b/module/VuFind/src/VuFind/Form/Form.php
@@ -31,6 +31,7 @@ use VuFind\Config\YamlReader;
 use Zend\InputFilter\InputFilter;
 use Zend\Validator\EmailAddress;
 use Zend\Validator\NotEmpty;
+use Zend\View\HelperPluginManager;
 
 /**
  * Configurable form.
@@ -91,20 +92,32 @@ class Form extends \Zend\Form\Form implements
      */
     protected $yamlReader;
 
+    /**
+     * View helper manager.
+     *
+     * @var HelperPluginManager
+     */
+    protected $viewHelperManager;
+
     /**
      * Constructor
      *
-     * @param YamlReader $yamlReader    YAML reader
-     * @param array      $defaultConfig Default Feedback configuration (optional)
+     * @param YamlReader          $yamlReader        YAML reader
+     * @param HelperPluginManager $viewHelperManager View helper manager
+     * @param array               $defaultConfig     Default Feedback configuration
+     * (optional)
      *
      * @throws \Exception
      */
-    public function __construct(YamlReader $yamlReader, array $defaultConfig = null)
-    {
+    public function __construct(
+        YamlReader $yamlReader, HelperPluginManager $viewHelperManager,
+        array $defaultConfig = null
+    ) {
         parent::__construct();
 
         $this->defaultFormConfig = $defaultConfig;
         $this->yamlReader = $yamlReader;
+        $this->viewHelperManager = $viewHelperManager;
     }
 
     /**
@@ -127,6 +140,23 @@ class Form extends \Zend\Form\Form implements
         $this->buildForm($this->formElementConfig);
     }
 
+    /**
+     * Get display string.
+     *
+     * @param string $translationKey Translation key
+     * @param bool   $escape         Whether to escape the output.
+     * Default behaviour is to escape when the translation key does
+     * not end with '_html'.
+     *
+     * @return string
+     */
+    public function getDisplayString($translationKey, $escape = null)
+    {
+        $escape = $escape ?? substr($translationKey, -5) !== '_html';
+        return $this->viewHelperManager->get($escape ? 'transEsc' : 'translate')
+            ->__invoke($translationKey);
+    }
+
     /**
      * Get form configuration
      *
diff --git a/module/VuFind/src/VuFind/Form/FormFactory.php b/module/VuFind/src/VuFind/Form/FormFactory.php
index 4849f589dd0..22884a4a657 100644
--- a/module/VuFind/src/VuFind/Form/FormFactory.php
+++ b/module/VuFind/src/VuFind/Form/FormFactory.php
@@ -65,9 +65,10 @@ class FormFactory implements FactoryInterface
         $config = $container->get(\VuFind\Config\PluginManager::class)
             ->get('config')->toArray();
         $yamlReader = $container->get(\VuFind\Config\YamlReader::class);
+        $viewHelperManager = $container->get('ViewHelperManager');
 
         return new $requestedName(
-            $yamlReader, $config['Feedback'] ?? null
+            $yamlReader, $viewHelperManager, $config['Feedback'] ?? null
         );
     }
 }
diff --git a/module/VuFind/tests/unit-tests/src/VuFindTest/Form/FormTest.php b/module/VuFind/tests/unit-tests/src/VuFindTest/Form/FormTest.php
index 357934a9c25..3f2f0ed64ed 100644
--- a/module/VuFind/tests/unit-tests/src/VuFindTest/Form/FormTest.php
+++ b/module/VuFind/tests/unit-tests/src/VuFindTest/Form/FormTest.php
@@ -48,7 +48,10 @@ class FormTest extends \VuFindTest\Unit\TestCase
      */
     public function testDefaultsWithoutConfiguration()
     {
-        $form = new Form(new YamlReader());
+        $form = new Form(
+            new YamlReader(),
+            $this->createMock(\Zend\View\HelperPluginManager::class)
+        );
         $this->assertTrue($form->isEnabled());
         $this->assertTrue($form->useCaptcha());
         $this->assertFalse($form->showOnlyForLoggedUsers());
@@ -80,7 +83,11 @@ class FormTest extends \VuFindTest\Unit\TestCase
             'recipient_name' => 'me',
             'email_subject' => 'subject',
         ];
-        $form = new Form(new YamlReader(), $defaults);
+        $form = new Form(
+            new YamlReader(),
+            $this->createMock(\Zend\View\HelperPluginManager::class),
+            $defaults
+        );
         $this->assertEquals(
             [['name' => 'me', 'email' => 'me@example.com']], $form->getRecipient()
         );
@@ -97,7 +104,10 @@ class FormTest extends \VuFindTest\Unit\TestCase
         $this->expectException(\VuFind\Exception\RecordMissing::class);
         $this->expectExceptionMessage('Form \'foo\' not found');
 
-        $form = new Form(new YamlReader());
+        $form = new Form(
+            new YamlReader(),
+            $this->createMock(\Zend\View\HelperPluginManager::class)
+        );
         $form->setFormId('foo');
     }
 
@@ -108,8 +118,12 @@ class FormTest extends \VuFindTest\Unit\TestCase
      */
     public function testDefaultsWithFormSet()
     {
-        $form = new Form(new YamlReader());
+        $form = new Form(
+            new YamlReader(),
+            $this->createMock(\Zend\View\HelperPluginManager::class)
+        );
         $form->setFormId('FeedbackSite');
+
         $this->assertTrue($form->isEnabled());
         $this->assertTrue($form->useCaptcha());
         $this->assertFalse($form->showOnlyForLoggedUsers());
@@ -144,9 +158,11 @@ class FormTest extends \VuFindTest\Unit\TestCase
             ],
             $form->getElements()
         );
+
         $this->assertEquals(
             [['email' => null, 'name' => null]], $form->getRecipient()
         );
+
         $this->assertEquals('Send us your feedback!', $form->getTitle());
         $this->assertNull($form->getHelp());
         $this->assertEquals('VuFind Feedback', $form->getEmailSubject([]));
diff --git a/themes/bootstrap3/templates/feedback/form.phtml b/themes/bootstrap3/templates/feedback/form.phtml
index 2878cc3ffd2..78c3eb774dc 100644
--- a/themes/bootstrap3/templates/feedback/form.phtml
+++ b/themes/bootstrap3/templates/feedback/form.phtml
@@ -13,8 +13,8 @@
 
   $help = $form->getHelp();
   $helpPre = $helpPost = null;
-  $helpPre = isset($help['pre']) ? $this->translate($help['pre']) : null;
-  $helpPost = isset($help['post']) ? $this->translate($help['post']) : null;
+  $helpPre = isset($help['pre']) ? $form->getDisplayString($help['pre'], false) : null;
+  $helpPost = isset($help['post']) ? $form->getDisplayString($help['post'], false) : null;
 ?>
 <?php if (!$this->inLightbox): ?><div class="feedback-content"><?php endif; ?>
   <?php if ($title): ?>
@@ -53,6 +53,17 @@
         $handleGroup = 'openAndClose';
         $currentGroup = $group;
     }
+    $elementHelpPre = $elementHelpPost = null;
+    if ($elementHelp = $el['help'] ?? null) {
+      if (is_string($elementHelp)) {
+        $elementHelpPre = $elementHelp;
+      } else {
+        $elementHelpPre = $elementHelp['pre'] ?? null;
+        $elementHelpPost = $elementHelp['post'] ?? null;
+      }
+      $elementHelpPre = $form->getDisplayString($elementHelpPre);
+      $elementHelpPost = $form->getDisplayString($elementHelpPost);
+    }
     ?>
 
     <?php if (in_array($handleGroup, ['close', 'openAndClose'])): ?>
@@ -66,9 +77,9 @@
       <?php endif ?>
     <?php endif ?>
 
-    <div class="form-group <?= $el['type'] ?>">
-    <?php if (!empty($el['help'])): ?>
-      <p class="info"><?= substr($el['help'], -5) === '_html' ? $this->translate($el['help']) : $this->transEsc($el['help']) ?></p>
+    <div class="form-group <?= $el['type'] ?> group-<?=$this->escapeHtmlAttr($el['name'])?>">
+    <?php if (!empty($elementHelpPre)): ?>
+      <p class="info pre"><?=$elementHelpPre?></p>
     <?php endif ?>
     <?php if ($el['type'] !== 'submit'): ?>
       <?php if ($el['label']): ?>
@@ -87,6 +98,9 @@
       <?=$this->recaptcha()->html($this->useRecaptcha) ?>
     <?php endif ?>
     <?= $this->formRow($formElement) ?>
+    <?php if (!empty($elementHelpPost)): ?>
+      <p class="info post"><?=$elementHelpPost?></p>
+    <?php endif ?>
     </div>
   <?php endforeach ?>
   <?= $this->form()->closeTag() ?>
-- 
GitLab