Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

variables and mentions #590

Closed
wants to merge 11 commits into from
Closed
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ function ($attribute, $value, $fail) {
}
},
],
'confirmation_reply_to' => 'email|nullable',
'confirmation_reply_to' => 'nullable',
'notification_sender' => 'required',
'notification_subject' => 'required',
'notification_body' => 'required',
Expand Down Expand Up @@ -107,6 +107,7 @@ public static function validateEmail($email): bool
public static function formatData(array $data): array
{
return array_merge(parent::formatData($data), [
'notification_subject' => Purify::clean($data['notification_subject'] ?? ''),
'notification_body' => Purify::clean($data['notification_body'] ?? ''),
]);
}
Expand Down
38 changes: 30 additions & 8 deletions api/app/Mail/Forms/SubmissionConfirmationMail.php
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

use App\Events\Forms\FormSubmitted;
use App\Mail\OpenFormMail;
use App\Open\MentionParser;
use App\Service\Forms\FormSubmissionFormatter;
use Illuminate\Bus\Queueable;
use Illuminate\Contracts\Queue\ShouldQueue;
Expand All @@ -16,13 +17,21 @@ class SubmissionConfirmationMail extends OpenFormMail implements ShouldQueue
use Queueable;
use SerializesModels;

private $formattedData;

/**
* Create a new message instance.
*
* @return void
*/
public function __construct(private FormSubmitted $event, private $integrationData)
{
$formatter = (new FormSubmissionFormatter($event->form, $event->data))
->createLinks()
->outputStringsOnly()
->useSignedUrlForFiles();

$this->formattedData = $formatter->getFieldsWithValue();
Comment on lines +29 to +34
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Handle Potential Exceptions from FormSubmissionFormatter

While constructing the FormSubmissionFormatter, exceptions could be thrown if the formatting fails. Consider adding exception handling to manage potential errors gracefully and provide fallback mechanisms if necessary.

Example:

try {
    $formatter = (new FormSubmissionFormatter($event->form, $event->data))
        ->createLinks()
        ->outputStringsOnly()
        ->useSignedUrlForFiles();

    $this->formattedData = $formatter->getFieldsWithValue();
} catch (\Exception $e) {
    // Handle the exception, log it, or set a default value
    // For example:
    $this->formattedData = [];
    \Log::error('FormSubmissionFormatter failed: ' . $e->getMessage());
}

}

/**
Expand All @@ -34,17 +43,14 @@ public function build()
{
$form = $this->event->form;

$formatter = (new FormSubmissionFormatter($form, $this->event->data))
->createLinks()
->outputStringsOnly()
->useSignedUrlForFiles();

return $this
->replyTo($this->getReplyToEmail($form->creator->email))
->from($this->getFromEmail(), $this->integrationData->notification_sender)
->subject($this->integrationData->notification_subject)
->subject($this->getSubject())
->markdown('mail.form.confirmation-submission-notification', [
'fields' => $formatter->getFieldsWithValue(),
'notificationBody' => $this->getNotificationBody(),
'fields' => $this->formattedData,
'form' => $form,
'integrationData' => $this->integrationData,
'noBranding' => $form->no_branding,
Expand All @@ -67,9 +73,25 @@ private function getReplyToEmail($default)
{
$replyTo = $this->integrationData->confirmation_reply_to ?? null;

if ($replyTo && filter_var($replyTo, FILTER_VALIDATE_EMAIL)) {
return $replyTo;
if ($replyTo) {
$parser = new MentionParser($replyTo, $this->formattedData);
$parsedReplyTo = $parser->parse();
if ($parsedReplyTo && filter_var($parsedReplyTo, FILTER_VALIDATE_EMAIL)) {
return $parsedReplyTo;
}
Comment on lines +76 to +81
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider Refactoring Repetitive Parsing Logic

The parsing logic using MentionParser in getReplyToEmail(), getSubject(), and getNotificationBody() is repetitive. To enhance maintainability and adhere to the DRY (Don't Repeat Yourself) principle, consider refactoring this code into a single private method.

Here's how you might refactor the parsing logic:

private function parseIntegrationDataField($field)
{
    $value = $this->integrationData->$field ?? null;
    if ($value) {
        $parser = new MentionParser($value, $this->formattedData);
        return $parser->parse();
    }
    return null;
}

Update the methods accordingly:

private function getReplyToEmail($default)
{
    $parsedReplyTo = $this->parseIntegrationDataField('confirmation_reply_to');

    if ($parsedReplyTo && filter_var($parsedReplyTo, FILTER_VALIDATE_EMAIL)) {
        return $parsedReplyTo;
    }
    return $default;
}

private function getSubject()
{
-   $parser = new MentionParser($this->integrationData->notification_subject, $this->formattedData);
-   return $parser->parse();
+   return $this->parseIntegrationDataField('notification_subject') ?? '';
}

private function getNotificationBody()
{
-   $parser = new MentionParser($this->integrationData->notification_body, $this->formattedData);
-   return $parser->parse();
+   return $this->parseIntegrationDataField('notification_body') ?? '';
}

}
return $default;
}

private function getSubject()
{
$parser = new MentionParser($this->integrationData->notification_subject, $this->formattedData);
return $parser->parse();
}

private function getNotificationBody()
{
$parser = new MentionParser($this->integrationData->notification_body, $this->formattedData);
return $parser->parse();
}
Comment on lines +92 to +96
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Review the Usage of getNotificationBody()

The getNotificationBody() method is defined but not used within this class. Unused methods can introduce unnecessary complexity. Consider implementing it where appropriate or removing it if it's not needed.

}
97 changes: 97 additions & 0 deletions api/app/Open/MentionParser.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,97 @@
<?php

namespace App\Open;

use DOMDocument;
use DOMXPath;

class MentionParser
{
private $content;
private $data;

public function __construct($content, $data)
{
$this->content = $content;
$this->data = $data;
}
Comment on lines +13 to +17
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add type hints and return types for improved code clarity

Adding parameter and return type hints enhances readability and helps catch type-related bugs. Consider updating your methods with type declarations:

-public function __construct($content, $data)
+public function __construct(string $content, array $data)

-public function parse()
+public function parse(): string

-private function replaceMentions()
+private function replaceMentions(): string

-private function getData($fieldId)
+private function getData(string $fieldId)

Also applies to: 16-20, 21-41, 43-54


public function parse()
{
$doc = new DOMDocument();
// Disable libxml errors and use internal errors
$internalErrors = libxml_use_internal_errors(true);

// Wrap the content in a root element to ensure it's valid XML
$wrappedContent = '<root>' . $this->content . '</root>';

// Load HTML, using UTF-8 encoding
$doc->loadHTML(mb_convert_encoding($wrappedContent, 'HTML-ENTITIES', 'UTF-8'), LIBXML_HTML_NOIMPLIED | LIBXML_HTML_NODEFDTD);

// Restore libxml error handling
libxml_use_internal_errors($internalErrors);

$xpath = new DOMXPath($doc);
$mentionElements = $xpath->query("//span[@mention]");

foreach ($mentionElements as $element) {
$fieldId = $element->getAttribute('mention-field-id');
$fallback = $element->getAttribute('mention-fallback');
$value = $this->getData($fieldId);

if ($value !== null) {
$textNode = $doc->createTextNode(is_array($value) ? implode(', ', $value) : $value);
$element->parentNode->replaceChild($textNode, $element);
} elseif ($fallback) {
$textNode = $doc->createTextNode($fallback);
$element->parentNode->replaceChild($textNode, $element);
} else {
$element->parentNode->removeChild($element);
}
}

// Extract and return the processed HTML content
$result = $doc->saveHTML($doc->getElementsByTagName('root')->item(0));

// Remove the root tags we added
$result = preg_replace('/<\/?root>/', '', $result);

// Trim whitespace and convert HTML entities back to UTF-8 characters
$result = trim(html_entity_decode($result, ENT_QUOTES | ENT_HTML5, 'UTF-8'));

return $result;
}
Comment on lines +19 to +63
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider refactoring and adding error handling.

The parse() method is well-structured but quite long. Consider breaking it down into smaller, more focused methods for better readability and maintainability. Additionally, it would be beneficial to add error handling for potential DOM manipulation failures.

For example:

  1. Create a method for initializing the DOMDocument.
  2. Create a method for processing mention elements.
  3. Create a method for extracting the final HTML content.

Also, consider wrapping the DOM operations in a try-catch block to handle any potential exceptions.


private function replaceMentions()
{
$pattern = '/<span[^>]*mention-field-id="([^"]*)"[^>]*mention-fallback="([^"]*)"[^>]*>.*?<\/span>/';
return preg_replace_callback($pattern, function ($matches) {
$fieldId = $matches[1];
$fallback = $matches[2];
$value = $this->getData($fieldId);

if ($value !== null) {
if (is_array($value)) {
return implode(' ', array_map(function ($v) {
return $v;
}, $value));
}
return $value;
} elseif ($fallback) {
return $fallback;
}
return '';
}, $this->content);
}
Comment on lines +65 to +85
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Clarify the purpose of the unused replaceMentions() method.

The replaceMentions() method appears to be an alternative implementation using regex for parsing mentions. However, it's not currently used in the class. Consider either:

  1. Removing this method if it's no longer needed.
  2. Documenting its purpose if it's intended for future use or as an alternative implementation.
  3. Refactoring the class to use this method if it's meant to be the primary implementation.

Keeping unused methods can lead to confusion and maintenance issues in the future.


private function getData($fieldId)
{
$value = collect($this->data)->firstWhere('nf_id', $fieldId)['value'] ?? null;

if (is_object($value)) {
return (array) $value;
}

return $value;
}
}
21 changes: 21 additions & 0 deletions api/app/Service/HtmlPurifier/OpenFormsHtmlDefinition.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
<?php

namespace App\Service\HtmlPurifier;

use HTMLPurifier_HTMLDefinition;
use Stevebauman\Purify\Definitions\Definition;
use Stevebauman\Purify\Definitions\Html5Definition;

class OpenFormsHtmlDefinition implements Definition
{
public static function apply(HTMLPurifier_HTMLDefinition $definition)
{
Html5Definition::apply($definition);

$definition->addAttribute('span', 'mention-field-id', 'Text');
$definition->addAttribute('span', 'mention-field-name', 'Text');
$definition->addAttribute('span', 'mention-fallback', 'Text');
$definition->addAttribute('span', 'mention', 'Bool');
$definition->addAttribute('span', 'contenteditable', 'Bool');
}
}
6 changes: 3 additions & 3 deletions api/config/purify.php
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
<?php

use Stevebauman\Purify\Definitions\Html5Definition;
use App\Service\HtmlPurifier\OpenFormsHtmlDefinition;

return [

Expand Down Expand Up @@ -40,7 +40,7 @@
'configs' => [

'default' => [
'HTML.Allowed' => 'h1,h2,b,strong,i,em,a[href|title],ul,ol,li,p,br,span,*[style]',
'HTML.Allowed' => 'h1,h2,b,strong,i,em,a[href|title],ul,ol,li,p,br,span[mention|mention-field-id|mention-field-name|mention-fallback],*[style]',
'HTML.ForbiddenElements' => '',
'CSS.AllowedProperties' => 'font,font-size,font-weight,font-style,text-decoration,color,text-align',

Expand Down Expand Up @@ -86,7 +86,7 @@
|
*/

'definitions' => Html5Definition::class,
'definitions' => OpenFormsHtmlDefinition::class,

/*
|--------------------------------------------------------------------------
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
@component('mail::message', ['noBranding' => $noBranding])

{!! $integrationData->notification_body !!}
{!! $notificationBody !!}

@if($form->editable_submissions)
@component('mail::button', ['url' => $form->share_url.'?submission_id='.$submission_id])
Expand Down
Loading
Loading