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

Fixed the endless Re: Re: loop #1380

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fixed the endless Re: Re: loop #1380

wants to merge 1 commit into from

Conversation

mikenowak
Copy link
Contributor

Currently when responding to messages via helpdesk the 'Re:' is appended by the following piece of code

$message['subject'] = 'Re: '.$reply['subject'];

However, it the message has been exchanged between client and helpdesk a number of times the subject line would contain Re: multiple times - i.e. 'Re: Re: Re: test'

@mikenowak
Copy link
Contributor Author

looks like a problem with travis rather than the code.

@chilek
Copy link
Owner

chilek commented Sep 13, 2018

looks like a problem with travis rather than the code.

Yeap. Not first time. Every day they generate at least on check fail.

@chilek
Copy link
Owner

chilek commented Sep 13, 2018

Currently when responding to messages via helpdesk the 'Re:' is appended by the following piece of code
However, it the message has been exchanged between client and helpdesk a number of times the subject line would contain Re: multiple times - i.e. 'Re: Re: Re: test'

It depends on what you click "Reply/Answer" or "New message/note". The latter doesn't prepend subject with 'Re:'.

@chilek
Copy link
Owner

chilek commented Sep 13, 2018

Perhaps we should limit 'Re: ' phrase count?

@mikenowak
Copy link
Contributor Author

mikenowak commented Sep 13, 2018

OK I've tested the behaviour as per your previous note, and it is indeed as you describe.

Looking through modules/rtnoteadd.php, I came across this piece of code that has been added recently:

if(ConfigHelper::checkConfig('rt.note_send_re_in_subject'))
    $params['subject'] = 'Re: '.$ticketdata['subject'];

I also believe that note notifications are only sent to users, not clients.

On the subject of New Messages, I confirm that they do not contain Re:.

Also while looking at this I noticed that while message replies utilise sprint('%06d', $id), which results in 6 digit %tid, the notifications don't use this. So, there is a discrepancy between them being 000042 and 42 respectively. I will be submitting a separate PR for this - if this is something that needs fixing, please confirm. This has been implemented at #1383.

Anyway, going back to the original subject - what would be the right course of action here?

@mikenowak
Copy link
Contributor Author

@chilek, additionally the ticket number isn't included in the subject of notes or messages and I think this would be quite useful.

I am thinking of something among the lines of:

$ticketid = sprintf("%06d", $message['ticketid']);
$ticket_prefix = ConfigHelper::getConfig('rt.ticket_prefix', 'RT#');

Then

Reply to an existing message:

if (preg_match('/^Re: \[' . $ticket_prefix . $ticketid . '\] /i', $reply['subject']) === 1) {
	$message['subject'] = $reply['subject'];
} else {
	$message['subject'] = 'Re: [' . $ticket_prefix . $ticketid . '] ' . $reply['subject'];
}
	

New message:

$message['subject'] = 'Re: [' .$ticket_prefix . $ticketid . '] ' . $reply['subject'];

And a note

$params['subject'] = 'Re: [' .$ticket_prefix . $ticketid . '] ' . $ticketdata['subject'];

Thoughts before I go and implement this?

@interduo
Copy link
Collaborator

Maybe note should have other prefix?

@mikenowak
Copy link
Contributor Author

@interduo, ok sure - I am open for suggestions - just let me know what you think the note prefix should be.

@chilek
Copy link
Owner

chilek commented Sep 14, 2018

additionally the ticket number isn't included in the subject of notes or messages and I think this would be quite useful.

@mikenowak imo better to use helpdesk notification format configuration variable settings such as:
phpui.helpdesk_customerinfo_mail_body
phpui.helpdesk_customerinfo_sms_body
phpui.helpdesk_notification_mail_subject
phpui.helpdesk_notification_mail_body
phpui.helpdesk_notification_sms_body

Have u tried to use them?

@chilek
Copy link
Owner

chilek commented Sep 14, 2018

Would be also nice to allow user define ticketid format by use something like printf-like format:
%02tid. What do you think about this?

@mikenowak
Copy link
Contributor Author

OK all good feedback, thanks for that.

additionally the ticket number isn't included in the subject of notes or messages and I think this would be quite useful.

It turns out google apps (also most likely also gmail) group the email with the same reference, and I wasn't getting constant subjects during my tests even that phpui.helpdesk_notification_mail_subject was set.

So lets ignore that comment and focus on the below:

Perhaps we should limit 'Re: ' phrase count?

and

Would be also nice to allow user define ticketid format by use something like printf-like format:
%02tid. What do you think about this?

I will see what can be done about these and report back.

@mikenowak
Copy link
Contributor Author

@chilek,

I also noticed that the ticket number is appended to the front of every user notification as per phpui.helpdesk_notification_subject.

So I am thinking of cleaning up the subject a little before we do various str_replace on it.

So while sending new note/message that is fine because the subject would look like [RT#123456] test ticket, however moving on responding to an existing message would produce [RT#123456] Re: [RT#123456] test ticket

So far I came up with this

        public function ReplaceNotificationSymbols($text, array $params) {
+               // strip everything up to the ticket number 
+               if (preg_match('/^(?:[^\]]+)\]\s(.*)/i', $params['subject'], $match))
+                      $params['subject'] = $match[1];
                $text = str_replace('%tid', sprintf("%06d", $params['id']), $text);

This would strip everything up to the first ] inclusive, plus a space.

i.e. Re: [RT#123456] test ticket would become test ticket.

This however depends on the %tid in the phpui.helpdesk_notification_subject being wrapped in [].

Can you point at a better way of doing this if you can think of one?

@chilek
Copy link
Owner

chilek commented Sep 19, 2018

@mikenowak very good question hard to answer for now. Maybe we should introduce some new configuration setting which would describe format of ticket prefix.
i.e. if phpui.helpdesk_notification_ticketid_format (temporary name) has value of [RT#%06tid] and already existing phpui.helpdesk_notification_subject has value of %tid %subject substitution process would produce the following result:
[RT#000345] some ticket subject
The one challenge which it would trigger, we'll have to find a way to convert phpui.helpdesk_notification_ticketid_format to corresponding regular expression.

So we would have 2-level %tid substitution:

  1. phpui.helpdesk_notification_ticketid_format (replaces [RT#%06tid] by [RT#000345]).
  2. phpui.helpdesk_notification_subject (replaces %tid %subject by [RT#000345] some ticket subject).

By default phpui.helpdesk_notification_ticketid_format would have %tid value so it wouldn't break existing configurations.

@chilek
Copy link
Owner

chilek commented Sep 19, 2018

I've fixed a liitle bit my above comment.

@chilek
Copy link
Owner

chilek commented Sep 19, 2018

The one challenge which it would trigger, we'll have to find a way to convert phpui.helpdesk_notification_ticketid_format to corresponding regular expression.

Maybe regular expression generation would not be so difficult, because %tid the most complex format could be [RT#%06tid]
so regular expression will always be \[RT#[0-9]*\] when prefix have format [RT#%06tid]. How I calculated this regexp? By escaping special symbols and replacing %06tid by [0-9]*.

What do you think of this?

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.

3 participants