salesagility / SuiteCRM

SuiteCRM - Open source CRM for the world
https://www.suitecrm.com
GNU Affero General Public License v3.0
4.53k stars 2.09k forks source link

AOS_PDF_Templates change column type to allow more data #8967

Open tsmgeek opened 4 years ago

tsmgeek commented 4 years ago

Can we change the pdfheader/pdffooter fields to LONGTEXT inline with the description column so we can store b64 image data embedded in the template.

https://github.com/salesagility/SuiteCRM/blob/master/modules/AOS_PDF_Templates/vardefs.php#L150 https://github.com/salesagility/SuiteCRM/blob/master/modules/AOS_PDF_Templates/vardefs.php#L165

pgorod commented 3 years ago

🤔 wouldn't it be better to just reference the image, instead of storing it with the template? Even if then it would eventually get encoded into the PDF file when generating....

Another concern, if this actually gets implemented, is to not change the SugarField type carelessly - it has several implications on the screens where TinyMCE is used. Probably better to just change dbtype in the vardefs.

tsmgeek commented 3 years ago

@pgorod

  1. email templates use LONGTEXT
  2. pdfheader/pdffooter use LONGTEXT

Yes I meant only change the type in vardefs, nothing on the UI itself and standardizing all the field to be the same type.

We do not use TinyMCE anyway to generate templates, we have them all built via another system which uses templating engine and then just updates the DB directly. Base64 images just makes it far easier to deploy these templates for us and means we do not have to redeploy our docker container with new theme standard logos etc.

But as I mentioned above this is more about just making the type field consistent in DB.

pgorod commented 3 years ago

There are two different things in the vardefs:

The SugarField selection, in turn, might affect the editor used. It depends on which screen you're on and how it's coded. Even if you don't use TinyMCE, changing this without care for the editor HTML that gets produced will result in bugs.

I'm working on changes around this. The problem I'm trying to solve is the fact that the Editor choice made in the user-profile (between TinyMCE, DirectHTML, Mozaik) currently only affects Email templates module. It does not affect the Email compose window, or the PDF templates, or any of the other places where we edit HTML (Cases, etc). So my solution (which I think aligns with the architecture much better) is a new SugarField called editor, which is able to supply HTML to use whichever Editor the user prefers, independently of which screen it's used on.

That said, I don't necessarily object to the changes you're proposing, I'm just saying that this needs to be carefully considered because it's definitely a problematic area of the app. Too much code repetition, too many changes without understanding the full implications of them --> technical debt all over the place.

tsmgeek commented 3 years ago

When I look on my CRM its using the same editor for all three description/header/footer fields, its just the size on the DB that is the issue.

pgorod commented 3 years ago

Yes, I am talking about the future :-)

Currently almost every screen is quite insensitive to the user-profile option for HTML editor. This can be quite confusing for users ("I changed the editor and nothing happened").

One of the advantages I aim to achieve by making this more consistent across all modules and views, is to allow more TinyMCE configuration to happen (in an easy way, with files from custom dir). Choosing toolbars, adding dictionaries, even JS hooks.

About the types/dbtypes:

We currently have the following relatively similar widget types: text, longtext, varchar, email. Some of them just inherit from the others (that's why the longtext widget behaves just like the text widget: see).

My point is just to avoid multiplying SugarFields and widget types, and change just the dbtype when that's all that needs to change.

tsmgeek commented 3 years ago

https://github.com/salesagility/SuiteCRM/pull/9044

Can we get this merged in as it illogical to leave it as text, our headers/footers have quite a bit of data in them.