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

Allow 'function' fields in email and pdf templates #4266

Open
wants to merge 76 commits into
base: develop
Choose a base branch
from

Conversation

HVStechnik
Copy link

Issue

E-mails send by the Workflow module based on an Email template do not contain function fields (source = 'function' in vardef definition). Those fields are not automatically calculated, when the bean is initiated and the db row retrieved. For standard views the function fields are calculated during displaying. For email templates they however stayed blank.

Solution

Update the templateParser.php file within the AOS_PDF_Templates module in order to calculate function fields while creating the replacement array for the template markers.
The solution is similar to the calculation of those fields in /include/EditView/EditView2.php lines 1000 to 1040

How To Test This

Create an email template including a function field from a vardef and try to send it automatically.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Final checklist

  • My code follows the code style of this project found here.
  • My change requires a change to the documentation.
  • I have read the How to Contribute guidelines.

Thanks to pgorod for guiding me through my first git pull request!

@Dillon-Brown Dillon-Brown added the PR:Type:Enhancement Pull requests that provide more functionality. Associated Issues are called suggestions label Apr 24, 2018
@codecov-io
Copy link

codecov-io commented Aug 18, 2018

Codecov Report

❗ No coverage uploaded for pull request base (develop@f4a767a). Click here to learn what that means.
The diff coverage is 0%.

@@            Coverage Diff             @@
##             develop    #4266   +/-   ##
==========================================
  Coverage           ?      11%           
  Complexity         ?    41385           
==========================================
  Files              ?     3378           
  Lines              ?   245629           
  Branches           ?        0           
==========================================
  Hits               ?    27035           
  Misses             ?   218594           
  Partials           ?        0

@jack7anderson7 jack7anderson7 added the PR:Community Contribution These are contribution made by the community label Dec 3, 2018
Dillon-Brown and others added 21 commits January 2, 2019 10:02
* Update command to allow for cache directory specified in config
* Remove warnings/errors when no cached directory present
* Update to have namespace "cache" so we can add other cache commands later
  * Leave alias for current command
* Use provided toolsets required by robo for implementation
* Print cleaned directories
* Confirm before cleaning directory
  * Allow for force override
…-notes

# Conflicts:
#	modules/Campaigns/Campaign.php
#	modules/Campaigns/views/view.detail.php
#	modules/Notes/vardefs.php
…gn-notes

Add ability to have Notes on a Campaign
…e-command-config-cache-dir

Allow for clean cache command to use config'd cache_dir (plus clean up)
Dillon-Brown and others added 5 commits May 29, 2019 10:46
# Conflicts:
#	modules/AOS_PDF_Templates/PDF_Lib/classes/bmp.php
#	modules/AOS_PDF_Templates/PDF_Lib/classes/cssmgr.php
#	modules/AOS_PDF_Templates/PDF_Lib/classes/directw.php
#	modules/AOS_PDF_Templates/PDF_Lib/classes/form.php
#	modules/AOS_PDF_Templates/PDF_Lib/classes/gif.php
#	modules/AOS_PDF_Templates/PDF_Lib/classes/grad.php
#	modules/AOS_PDF_Templates/PDF_Lib/classes/indic.php
#	modules/AOS_PDF_Templates/PDF_Lib/classes/svg.php
#	modules/AOS_PDF_Templates/PDF_Lib/classes/tocontents.php
#	modules/AOS_PDF_Templates/PDF_Lib/classes/ttfontsuni.php
#	modules/AOS_PDF_Templates/PDF_Lib/classes/wmf.php
#	modules/AOS_PDF_Templates/PDF_Lib/mpdf.php
#	modules/AOS_PDF_Templates/PDF_Lib/mpdfi/fpdi_pdf_parser.php
#	modules/AOS_PDF_Templates/PDF_Lib/mpdfi/pdf_context.php
#	modules/AOS_PDF_Templates/PDF_Lib/mpdfi/pdf_parser.php
# Conflicts:
#	composer.lock
#	files.md5
#	include/HTMLPurifier/HTMLPurifier.standalone.php
#	include/HTMLPurifier/standalone/HTMLPurifier/Lexer/PH5P.php
#	include/SugarCharts/Jit/FlashCanvas/canvas2png.js
#	include/SugarCharts/Jit/FlashCanvas/flashcanvas.js
#	include/SugarCharts/Jit/js/Jit/jit.js
#	include/SugarCharts/Jit/js/sugarCharts.js
#	include/pclzip/pclzip.lib.php
#	include/utils/zip_utils.php
#	lib/Robo/Plugin/Commands/CleanCacheCommands.php
#	modules/AOS_PDF_Templates/PDF_Lib/classes/barcode.php
#	modules/AOS_PDF_Templates/PDF_Lib/classes/bmp.php
#	modules/AOS_PDF_Templates/PDF_Lib/classes/cssmgr.php
#	modules/AOS_PDF_Templates/PDF_Lib/classes/directw.php
#	modules/AOS_PDF_Templates/PDF_Lib/classes/form.php
#	modules/AOS_PDF_Templates/PDF_Lib/classes/gif.php
#	modules/AOS_PDF_Templates/PDF_Lib/classes/grad.php
#	modules/AOS_PDF_Templates/PDF_Lib/classes/indic.php
#	modules/AOS_PDF_Templates/PDF_Lib/classes/meter.php
#	modules/AOS_PDF_Templates/PDF_Lib/classes/svg.php
#	modules/AOS_PDF_Templates/PDF_Lib/classes/tocontents.php
#	modules/AOS_PDF_Templates/PDF_Lib/classes/ttfontsuni.php
#	modules/AOS_PDF_Templates/PDF_Lib/classes/ttfontsuni_analysis.php
#	modules/AOS_PDF_Templates/PDF_Lib/classes/wmf.php
#	modules/AOS_PDF_Templates/PDF_Lib/compress.php
#	modules/AOS_PDF_Templates/PDF_Lib/config.php
#	modules/AOS_PDF_Templates/PDF_Lib/config_cp.php
#	modules/AOS_PDF_Templates/PDF_Lib/config_fonts.php
#	modules/AOS_PDF_Templates/PDF_Lib/font/ccourier.php
#	modules/AOS_PDF_Templates/PDF_Lib/font/ccourierb.php
#	modules/AOS_PDF_Templates/PDF_Lib/font/ccourierbi.php
#	modules/AOS_PDF_Templates/PDF_Lib/font/ccourieri.php
#	modules/AOS_PDF_Templates/PDF_Lib/font/chelvetica.php
#	modules/AOS_PDF_Templates/PDF_Lib/font/chelveticab.php
#	modules/AOS_PDF_Templates/PDF_Lib/font/chelveticabi.php
#	modules/AOS_PDF_Templates/PDF_Lib/font/chelveticai.php
#	modules/AOS_PDF_Templates/PDF_Lib/font/csymbol.php
#	modules/AOS_PDF_Templates/PDF_Lib/font/ctimes.php
#	modules/AOS_PDF_Templates/PDF_Lib/font/ctimesb.php
#	modules/AOS_PDF_Templates/PDF_Lib/font/ctimesbi.php
#	modules/AOS_PDF_Templates/PDF_Lib/font/ctimesi.php
#	modules/AOS_PDF_Templates/PDF_Lib/font/czapfdingbats.php
#	modules/AOS_PDF_Templates/PDF_Lib/graph.php
#	modules/AOS_PDF_Templates/PDF_Lib/includes/CJKdata.php
#	modules/AOS_PDF_Templates/PDF_Lib/includes/functions.php
#	modules/AOS_PDF_Templates/PDF_Lib/includes/ind_bn_1_001.volt.php
#	modules/AOS_PDF_Templates/PDF_Lib/includes/ind_gu_1_001.volt.php
#	modules/AOS_PDF_Templates/PDF_Lib/includes/ind_hi_1_001.volt.php
#	modules/AOS_PDF_Templates/PDF_Lib/includes/ind_kn_1_001.volt.php
#	modules/AOS_PDF_Templates/PDF_Lib/includes/ind_ml_1_001.volt.php
#	modules/AOS_PDF_Templates/PDF_Lib/includes/ind_or_1_001.volt.php
#	modules/AOS_PDF_Templates/PDF_Lib/includes/ind_pa_1_001.volt.php
#	modules/AOS_PDF_Templates/PDF_Lib/includes/ind_ta_1_001.volt.php
#	modules/AOS_PDF_Templates/PDF_Lib/includes/ind_te_1_001.volt.php
#	modules/AOS_PDF_Templates/PDF_Lib/includes/out.php
#	modules/AOS_PDF_Templates/PDF_Lib/includes/subs_core.php
#	modules/AOS_PDF_Templates/PDF_Lib/includes/subs_win-1252.php
#	modules/AOS_PDF_Templates/PDF_Lib/includes/upperCase.php
#	modules/AOS_PDF_Templates/PDF_Lib/mpdf.php
#	modules/AOS_PDF_Templates/PDF_Lib/mpdfi/filters/FilterASCII85.php
#	modules/AOS_PDF_Templates/PDF_Lib/mpdfi/filters/FilterLZW.php
#	modules/AOS_PDF_Templates/PDF_Lib/mpdfi/fpdi_pdf_parser.php
#	modules/AOS_PDF_Templates/PDF_Lib/mpdfi/pdf_context.php
#	modules/AOS_PDF_Templates/PDF_Lib/mpdfi/pdf_parser.php
#	modules/AOS_PDF_Templates/PDF_Lib/patterns/de.php
#	modules/AOS_PDF_Templates/PDF_Lib/patterns/en.php
#	modules/AOS_PDF_Templates/PDF_Lib/patterns/es.php
#	modules/AOS_PDF_Templates/PDF_Lib/patterns/fi.php
#	modules/AOS_PDF_Templates/PDF_Lib/patterns/fr.php
#	modules/AOS_PDF_Templates/PDF_Lib/patterns/it.php
#	modules/AOS_PDF_Templates/PDF_Lib/patterns/nl.php
#	modules/AOS_PDF_Templates/PDF_Lib/patterns/pl.php
#	modules/AOS_PDF_Templates/PDF_Lib/patterns/sv.php
#	modules/AOS_PDF_Templates/PDF_Lib/utils/UnicodeRanges.php
#	modules/AOS_PDF_Templates/PDF_Lib/utils/font_collections.php
#	modules/AOS_PDF_Templates/PDF_Lib/utils/font_coverage.php
#	modules/AOS_PDF_Templates/PDF_Lib/utils/font_dump.php
#	modules/AOS_PDF_Templates/PDF_Lib/utils/font_names.php
#	modules/Emails/include/ListView/DeleteEmailAction.js
#	modules/UpgradeWizard/silentUpgrade_step1.php
@Mac-Rae
Copy link
Contributor

Mac-Rae commented Aug 5, 2019

Please resolve the conflicts on this pull request bringing it up to date.

@Mac-Rae Mac-Rae added the Status:Requires Updates Issues & PRs which requires input or update from the author label Aug 5, 2019
@salesagility salesagility deleted a comment Sep 7, 2019
@HVStechnik
Copy link
Author

How do I check the clahub agreement? I have signed the agreement myself already. How can the "commit's parents" not have signed the CLA if I base on the SA SuiteCRM develop branch?

@Dillon-Brown
Copy link
Contributor

Hi @HVStechnik, the issue with the CLAHub is that your commit is coming from what Git sees as two different authors, "HVStechnik" and "Max Wilckens". I would suggest either editing the author of the last commit and force pushing back up or doing a rebase (This may be of some assistance: https://www.git-tower.com/learn/git/faq/change-author-name-email)

Let me know if you run into any difficulties, I can rebase this for you with just the "HVStechnik" account as the author if that makes it easier. Thanks!

@HVStechnik
Copy link
Author

Thanks @Dillon-Brown, I managed to change the author, but now travis fails. I think that the php5.6 build currently fails for all new pull requests....

samus-aran and others added 4 commits September 13, 2019 15:53
…html-purifier-file

Remove HTMLPurifier ConfigForm.js file.
E-mails send by the Workflow module based on an Email template do not contain function fields (source = 'function' in vardef definition). Those fields are not automatically calculated, when the bean is initiated and the db row retrieved. For standard views the function fields are calculated during displaying. For email templates they however stayed blank.

Update the templateParser.php file within the AOS_PDF_Templates module in order to calculate function fields while creating the replacement array for the template markers.
The solution is similar to the calculation of those fields in /include/EditView/EditView2.php lines 1000 to 1040
@Mac-Rae Mac-Rae removed the Status:Requires Updates Issues & PRs which requires input or update from the author label Oct 31, 2019
@pgorod
Copy link
Contributor

pgorod commented Nov 28, 2019

@Dillon-Brown it would be nice to merge this one, I think it's low risk and useful.

You don't have to label it "Enhancement", in my opinion, since it's technically a bug to have fields that conform to a spec, and render well in some views but fail when sent via email. This doesn't require the extra "Enhancement" review, I believe...

@HVStechnik
Copy link
Author

HVStechnik commented Nov 28, 2019

@pgorod I reviewed the custom function options once again and got a bit confused:

Within the vardefs there are three different versions of how to include a custom function to calculate a field value:

  1. $vardefs[$field]['function']
    The function vardef key may be an array that stores a file to be included (key: include), the function to be called (key: name), and an optional return mode (key: returns: if set to 'html' the function's return value replaces the field's value, otherwise the function's return value is set as the options key of the vardefs array and defines e.g., the options in an enum field).
    The code is implemented in EditView2, InlineEditing, MassUpdate, SearchForm2, and in SugarFieldBase
    DetailViews2 extends the EditView class (file: EditView2.php) and thus also implements this code. But it does not seem to be called in List views (?) With 'returns' => 'html' it should be similar to the option 3:

  2. $vardefs[$field]['source'] == 'function'
    This is, what I was adressing in this PR. If ['source'] == 'function', SugarBean looks for function_class, function_name, and function_params in the vardef array.
    This is implemented in SugarBean ony. It is implemented in the process_union_list_query function, which calls the function in ListViews.

  3. $vardefs[$field]['type'] == 'function'
    Last, but not least... This has the same parameters as ['source'] == 'function' (function_class, function_name, and function_params)
    This is implemented in SearchForm2 and EditView2. DetailViews2 extends the EditView class (file: EditView2.php) and thus also implements this code. But I think this version has the disadvantage, that the field does not render via the approapriate SugarField (because no specific type is given). Instead the function field is render via the base field, so that phone numbers, currencies, etc. are not rendered properly.

I would very much recommend to simplify this a little bit:
I think the 'returns' => 'html' cases should better implement option (2). The other implementations of (1) could be simplified by a utils function that implements the redundant code. Option 2 should also render in Detail and Edit view (maybe as read-only in edit views?).

After having done this review, I think we might want to leverage the callFunction function in EditView that does exactly the same as implemented by this PR. Would it be an option to move that function to SugarBean.php and call it whenever a custom function field needs to be evaluated?
What do you think?

PS: This does not make this PR less usefull ;-)

@pgorod
Copy link
Contributor

pgorod commented Nov 28, 2019

@HVStechnik thanks for taking the time to study this. I guess some reorganization would be beneficial. We'd have to be careful not to make the changes break any existing customizations that people might have out there, relying on one of these mechanisms.

Another thing that would be quite useful would be to improve (or start?) some Documentation about this... I don't think there's any place really teaching how to use this.

@SuiteBot
Copy link

SuiteBot commented Aug 27, 2020

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
7 out of 9 committers have signed the CLA.

✅ isleshocky77
✅ samus-aran
✅ Dillon-Brown
✅ cherrador
✅ connorshea
✅ cameronblaikie
✅ lazka
❌ code-ph0y
❌ HamburgischerVereinSeefahrt
You have signed the CLA already but the status is still pending? Let us recheck it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR:Community Contribution These are contribution made by the community PR:Type:Enhancement Pull requests that provide more functionality. Associated Issues are called suggestions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet