Introduce functionality to also get only the new part for HTML mails.

[Hipska]

This is a second try for #1 but now to the correct branch (master instead of develop)

[Hipska]

Hi @piRGoif, could you give any feedback?

[jbostoen]

What will happen in cases where a user forwards an email?

For instance, our users forward emails to the helpdesk which they suspect to be malware/phishing/...

[Hipska]

Good question, but then it’s a new ticket and I don’t think the method getNewPartHTML Is called in that case.

Anyway, maybe better to educate users to send the spam messages as attachments so the original headers can be investigated as well…

[jbostoen]

True, but you know how users are.

Sometimes they forward the email as a reply to the original request.

[Hipska]

What do you mean exactly?

If this feature introduces unexpected behavior, then it is easy to disable by providing an empty array in the config.

[jbostoen]

True, but if there's a possible change in behavior, it might either be best to leave it empty in the config by default; or warn in an upgrade note?

[piRGoif]

Hello,
I tend to agree with Jeffrey, this seems to be quite a change of the default behavior !

To answer this PR, I would say the same as for #1 :

Could you provide some examples to see this in action ? There are lots of existing one in the log directory, you can add yours in there.

Also we are not very confident to change the default and historic behavior... You should add a parameter to activate your new behavior, this parameter would disabled it by default.

[Hipska]

The current behaviour is that this functionality is being done when the email is plain text, my addition only adds that this is now also correctly done when the email is in html format. Why would anyone ever want different behaviour based on the format of the email?

In my opinion, this is more a fix than adding new functionality. (In the worst case you could say that this is a completion of not fully implemented feature.)

Maybe we can schedule a call to discuss face to face? I think that will be easier to explain both sides.

[jbostoen]

Just to be clear: I actually agree that this might be a fix/fully implementing a feature.

Just warning that it may need an upgrade note which really draws some attention about the implication.

[Hipska]

Okay, I made a configuration parameter delimit_html_message to be set true when you want to enable this functionality.

Default behaviour will be the same in case of upgrade. New users/installations will have the new functionality.

[jbostoen]

Something I just noticed related to this: settings seem to mostly have underscores; but for some reason the delimiter settings have hyphens. Isn't this something which could also be tackled?

And if so: is this something which could/should automatically be updated for existing users?

[Hipska]

Yes should be tackled, Yes I guess update class can do this migration, and no, this is not in the scope for this PR. You can/should make a separate issue/PR for that.

[jbostoen]

Yes should be tackled, Yes I guess update class can do this migration, and no, this is not in the scope for this PR. You can/should make a separate issue/PR for that.

I'll wait for feedback from Combodo if this would be accepted, before making a separate PR. :)

[piRGoif]

Thanks for the modification Thomas ! Sorry I don't have much time right now :/ Can this be tested in any way so we can see with/without results ? Maybe using the existing eml test files ?

About the configuration options syntax, in the Config class it seems that there are only underscore used. I think it was a mistake to use hyphens in some of combodo-email-synchro parameters (like undesired-purge-delay or html-tags-to-remove), actually there are more config parameter in this modules that are using underscores...
So yes, it's better to use underscore I think.

Changing existing parameters names to use underscore for all of them seems like a challenge for me (updating existing config files), with a benefit that can be questioned...

[piRGoif]

About the configuration options syntax

Jeffrey did a PR on this : #9. Continue the discussion on this topic in this new PR :)

[piRGoif]

Hello,

Can this be tested in any way so we can see with/without results ? Maybe using the existing eml test files ?

Do you think you can manage to code such a test @Hipska ?

[Hipska]

I would need to see how, never did any and I also don't have much time for this now.

Also, it doesn't seem the tests are being run by GitHub CI?

[piRGoif]

Hello,

Also, it doesn't seem the tests are being run by GitHub CI?

We are running on own private instances, and not planning to change this.
PHPUnit tests can be ran easily using Composer though.

[jbostoen]

I was thinking about this earlier today after considering if it's doable to add some form of loop-protection (where ticket system B sends an e-mail to iTop instance A, each time without reference, resulting in a new ticket in system A, notification to user/system B, triggering a new e-mail to iTop A, ...).

Anyhow: I like the idea of this pull request very much, but I'm also wondering if it can be done in a better way to avoid that messages are being stripped too easily. You're currently tackling it the same way Combodo does. But perhaps the default patterns should be more strict or it should be possible to make them more strict. Do we really want to remove every message that's simply forwarded to the help desk? (for instance: a phishing mail. Either as a new ticket or as a reply to an existing one). Should the "From:" be more strict and should it be recommended in notes to change the pattern to match something like **From:** blabla <servicedesk@itopinstance.com> ? And if so... should we make it possible to use a variable here, as this is a global setting and users might have multiple email inboxes? So a $current_mailbox_email_address$ or something? Otherwise admins need to configure this in the iTop configuration for each mailbox too.

If this becomes more strict and only skips the part where the From: contains the mailbox's e-mail address, at least it would work with this kind of case (or where people just forward other emails). If @Hipska is correct (I didn't verify) that the GetNewPart method NOT called for new tickets, it has the additional benefit that also users forwarding a notification from a now closed ticket would still get processed correctly.

Also talking about patterns: when I forward a message in Outlook (desktop client), the "From:" in the message body is in bold (actually a strong tag). So we might also want to cover that in the default regex patterns as a new line or by changing the existing one?

And one for Combodo: is it the plan to only natively supply the settings for English/French, or can we add patterns for other languages too in the default settings?

[jbostoen]

Another concern I just realized: some people write a response such as "I've answered inline in the (original) message below".

There needs to be a bit more logic to compare it to the previous log entries.
Which makes another case for revising log entries structure ...

[Hipska]

Okay, that makes sense, but that is also the case for when they are plaintext..