Skip to content

Fio 9618 render email templates using a core processor #57

Merged
brendanbond merged 1 commit intomainfrom
FIO-9618-core-processors
Apr 23, 2025
Merged

Fio 9618 render email templates using a core processor #57
brendanbond merged 1 commit intomainfrom
FIO-9618-core-processors

Conversation

@blakekrammes
Copy link
Copy Markdown
Contributor

@blakekrammes blakekrammes commented Mar 28, 2025

Link to Jira Ticket

https://formio.atlassian.net/browse/FIO-9618

Description

What changed?

Moved email template rendering from using the renderer's getView() functionality to rendering in a core processor using only the form and submission json.

Why have you chosen this solution?

It isn't necessary to use the component class instances in the renderer to generate html for components. We were rerendering forms on the server in order to create html for them (which was error-prone and limited–– for instance, we couldn't evaluate insecure JS on the server), whereas now we just generate html based on the form json and submission data directly.

––––––––––––

  1. Datetime component renders timezone in the email template: (this may have been added in a version since 9.3.2). Screenshot 2025-04-08 at 11 49 20 AM

  2. Tagpad rendering is fixed and behaves differently from how it did (notice how column headers are duplicated in 9.3.2 and how the 2nd tagpad dot is nested in the first row instead of rendered in the second): Screenshot 2025-04-08 at 11 55 08 AM

  3. Render multiple values for components that support it (this could be improved I think, especially for text area and address where mere comma-separation is hard to read–– probably a new table with each value as a row would be ideal):Screenshot 2025-04-08 at 12 04 00 PM

Breaking Changes / Backwards Compatibility

n/a

Dependencies

formio/core#235

How has this PR been tested?

Automated tests (and lots of manual testing):

Checklist:

  • I have completed the above PR template
  • I have commented my code, particularly in hard-to-understand areas (in this PR, yes)
  • I have made corresponding changes to the documentation (if applicable)
  • My changes generate no new warnings
  • My changes include tests that prove my fix is effective (or that my feature works as intended)
  • New and existing unit/integration tests pass locally with my changes
  • Any dependent changes have corresponding PRs that are listed above

@blakekrammes
Copy link
Copy Markdown
Contributor Author

There's a problem with rendering wizard forms. I'm working on it.

@blakekrammes blakekrammes marked this pull request as draft April 4, 2025 18:10
@blakekrammes blakekrammes force-pushed the FIO-9618-core-processors branch from af426b9 to abf7b16 Compare April 4, 2025 22:06
Comment thread src/core/renderEmail.ts Outdated
`);
}
const document = scopeRef.emailDom.window.document;
if (
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

do we need to handle any other conditional cases? custom logic?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@blakekrammes almost certainly, but IIRC logic uses the same scope as simple conditionals. You could also consider using the conditionallyHidden and intentionallyHidden ephemeral state that we attach to the component.scope property, which might be a better fit here

Copy link
Copy Markdown
Contributor Author

@blakekrammes blakekrammes Apr 9, 2025

Choose a reason for hiding this comment

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

@brendanbond

Interesting, from the bit of testing I've been doing, conditionally hidden components don't show up on context.data even if you provide them values in the submission object. I'm wondering if we even need to check for conditionallyHidden here.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@blakekrammes They wouldn't show up in context.data if they were hidden, they'd show up in context.components[i].scope

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@brendanbond Ok, so if we're grabbing the component values from context.data, then we don't need to worry about checking conditionals here, right?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@blakekrammes blakekrammes marked this pull request as ready for review April 7, 2025 22:30
@blakekrammes blakekrammes changed the title Fio 9618 core processors Fio 9618 render email templates using a core processor Apr 8, 2025
Comment thread src/core/renderEmail.ts Outdated
`);
}
const document = scopeRef.emailDom.window.document;
if (
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@blakekrammes almost certainly, but IIRC logic uses the same scope as simple conditionals. You could also consider using the conditionallyHidden and intentionallyHidden ephemeral state that we attach to the component.scope property, which might be a better fit here

Comment thread src/core/renderEmailUtils.ts Outdated
Comment thread src/core/renderEmail.ts
);
const intentionallyHidden = component.hidden;

if (conditionallyHidden || intentionallyHidden) return;
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

handling of conditionally and intentionally hidden components

return i18n.t(text, params, ...args);
};

export const isLayoutComponent = (component: Component) => {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

now detecting layout type via model types

return modelType === 'none' || modelType === 'content';
};

export const isGridBasedComponent = (component?: Component | null) => {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

detecting via model type now

@blakekrammes blakekrammes force-pushed the FIO-9618-core-processors branch 2 times, most recently from 0394a3b to bc29f79 Compare April 14, 2025 16:39
Comment thread src/core/renderEmail.ts
// the address component stores all the data needed for rendering on the component itself
// the children (if in manual mode) do not need to be iterated over since their data is redundant
// the children are the individual manual mode address fields (address1, city, etc)
if (parent?.type === 'address') return;
Copy link
Copy Markdown
Contributor Author

@blakekrammes blakekrammes Apr 14, 2025

Choose a reason for hiding this comment

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

added this code since address components in manual mode don't need to iterate over their children. context.data at the address component's data path contains everything needed for rendering. iterating over the individual manual-mode address field components is unnecessary.

address component with children:
Screenshot 2025-04-14 at 11 43 49 AM

rowValue (a.k.a. context.data{addressComponentDataPath}):
Screenshot 2025-04-14 at 11 50 24 AM

this investigation stemmed from this ticket. and, while this new email rendering returned the expected output, it was only because the code was not written to properly attach the address children to the email template. I thought it best to avoid iterating over those children entirely.

https://formio.atlassian.net/browse/FIO-10000

* FIO-9618: implement email rendering with jsdom

* FIO-9618: Add email rendering for edit grid tagpad
  - move all dom insertion to renderEmailUtils
  - todo: nested edit grids, etc

* FIO-9618: Fix pathing for nested grid-table components
  - Clean up other logic

* FIO-9618: Add typing for email rendering via core processor
  - Use core Evaluator string interpolation for address formatting

* FIO-9618: Fix datetime timezone handling
  - Handle empty values corectly for various components

* FIO-9618: Use updated core version

* FIO-9618: Handle wizards and doubly+ nested forms correctly

* FIO-9618: Fix problems with nested grids and forms

** The initial commit was authored by Brendan, the rest and majority of code by Blake

Co-authored-by: Brendan Bond <brendanbond@gmail.com>
@blakekrammes blakekrammes force-pushed the FIO-9618-core-processors branch from bc29f79 to 5c79f75 Compare April 14, 2025 16:47
@brendanbond brendanbond merged commit 3763846 into main Apr 23, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants