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

feat(NotificationMailling): add 'References' header for mail grouping (Gmail) #14296

Conversation

stonebuzz
Copy link
Contributor

@stonebuzz stonebuzz commented Mar 14, 2023

From Gmail, mail grouping does not work.

To work GLPI notification need :

  • Same subject ( 🆗 )
  • Reference headers that reference IDs seen earlier in the thread, or have references headers that consistently refer to the same message ID ( ❌ )

https://workspaceupdates.googleblog.com/2019/03/threading-changes-in-gmail-conversation-view.html

this PR add custom header References with <GLPI-%uuid-%itemtype-%items_id>

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets !27170

trasher
trasher previously approved these changes Mar 14, 2023
@stonebuzz stonebuzz self-assigned this Mar 14, 2023
@stonebuzz stonebuzz added this to the 10.0.7 milestone Mar 14, 2023
@cedric-anne
Copy link
Member

Waiting for customer validation.

@cedric-anne cedric-anne marked this pull request as draft March 14, 2023 08:14
@stonebuzz stonebuzz force-pushed the feat_notificationeventmailling_header branch from bfed8d9 to 02d2d31 Compare March 14, 2023 13:59
@stonebuzz stonebuzz marked this pull request as ready for review March 16, 2023 09:47
install/mysql/glpi-empty.sql Outdated Show resolved Hide resolved
src/MailCollector.php Outdated Show resolved Hide resolved
src/NotificationEventMailing.php Outdated Show resolved Hide resolved
tests/bootstrap.php Outdated Show resolved Hide resolved
@cedric-anne cedric-anne self-requested a review March 20, 2023 13:37
@cedric-anne cedric-anne force-pushed the feat_notificationeventmailling_header branch 2 times, most recently from 04df93c to 5bbb79b Compare March 21, 2023 20:37
cedric-anne
cedric-anne previously approved these changes Mar 21, 2023
@cedric-anne cedric-anne dismissed stale reviews from trasher and themself March 21, 2023 20:50

Bug detected.

Copy link
Member

@cedric-anne cedric-anne left a comment

Choose a reason for hiding this comment

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

I made some tests, and grouping notifications for a same item in Gmail requires additional changes. Indeed, mails are grouped only if subject is identical. I was able to achieve it on my GLPI instance by changing subject in Ticket notification template translation this way:

-##ticket.action## ##ticket.title##
+##ticket.title##

We should so probably consider changing default subjects of our notifications to make them identical across events. I guess such change could be made in another PR.

Anyway current PR will also prevent grouping of some notifications that are currently grouped. For instance:

  • passwordexpires and passwordforget for a same User will not be grouped anymore, and that may be considered as an expected behaviour;
  • plugin_formcreator_form_created, plugin_formcreator_need_validation, plugin_formcreator_accepted and plugin_formcreator_refused for a same PluginFormcreatorFormAnswer will not be grouped anymore, and it will probably be considered as a bug.

So, as it will change grouping behaviour on plugin notification, that could be considered as a BC-break, we should probably made such a change in main branch only, and produce a documentation to explain how to override the CommonDBTM::getMessageReferenceEvent() to customize grouping.

Also, we should review all core notifications to ensure that we are correctly define grouping rules where it is necessary.

I put it as a draft, as we need to decide what should and what should not be done to finalize this PR.

@cedric-anne cedric-anne marked this pull request as draft March 21, 2023 21:04
@cedric-anne cedric-anne modified the milestones: 10.0.7, 10.1.0 Mar 21, 2023
@cedric-anne cedric-anne self-assigned this Mar 21, 2023
@cedric-anne cedric-anne self-requested a review April 20, 2023 15:33
@cedric-anne cedric-anne modified the milestones: 10.1.0, 10.0.8 Apr 20, 2023
@cedric-anne cedric-anne force-pushed the feat_notificationeventmailling_header branch 4 times, most recently from 50796fb to f10b946 Compare April 24, 2023 07:21
@cedric-anne cedric-anne force-pushed the feat_notificationeventmailling_header branch from f10b946 to c630c9b Compare April 24, 2023 08:28
@cedric-anne
Copy link
Member

As discussed IRL with @orthagh, we can merge this PR in 10.0/bugfixes branch.

Until now, all notifications related to the same object, depending on email client, were either always separated (in Gmail for instance), either always grouped (in Thunderbird for instance). With proposed changes, grouping instructions should premit to harmonize behaviour between email clients. This will change the way messages are grouped a bit, but it would not prevent end-users from receiving notifications, so the change is therefore acceptable in 10.0/bugfixes branch.

Note: As far as I understand, according to RFC2822, Gmail should group messages into a same thread even if subjects are different, but it is actually not the case. So, we still have to review default notification subjects, but it could be done in another PR.

@cedric-anne cedric-anne marked this pull request as ready for review April 24, 2023 08:34
@cedric-anne cedric-anne merged commit 7f94e39 into glpi-project:10.0/bugfixes May 10, 2023
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants