Attempting to make Subject: tag matching a little more permissive, diff enclosed

Hi. I’m trying to make RT play nice with a certain large telecom vendor’s ticketing system. They’re trying to play nice too, but they’re too dumb to get it right. :frowning: In particular, they’re turning our “[tag #12345]” into “[tag#12345]” on subject lines. So I want to relax the requirement for whitespace between the tag and the ticket number.

I was a bit lazy for a cold source-dive, but fortunately google turned up a commit a few years back that looked like it was touching exactly the code I wanted, lib/RT/Interface/Email.pm: ParseTicketId().

It looks like the changes I need to make are:

Email.pm 2015-10-19 13:38:13.000000000 -0400
+++ Email.test.pm 2016-09-28 12:50:06.000000000 -0400
@@ -1139,11 +1139,11 @@
# We use @captures and pull out the last capture value to guard against
# someone using (…) instead of (?:…) in $EmailSubjectTagRegex.
my $id;

  • if ( my @captures = $Subject =~ /[$test_name\s+#(\d+)\s*]/i ) {
  • if ( my @captures = $Subject =~ /[$test_name\s*#(\d+)\s*]/i ) {
    $id = $captures[-1];
    } else {
    foreach my $tag ( RT->System->SubjectTag ) {
  •        next unless my @captures = $Subject =~ /\[\Q$tag\E\s+\#(\d+)\s*\]/i;
    
  •        next unless my @captures = $Subject =~ /\[\Q$tag\E\s*\#(\d+)\s*\]/i;
          $id = $captures[-1];
          last;
      }
    

In other words, just replace a “+” with a “*”, so it’ll match no whitespace as well as some.

So, questions:

  1. Does this make sense?
  2. Am I likely to screw anything up by doing this?
  3. Did I leave out something else that needs to be adjusted, besides ParseTicketId()?
  4. Do I really need the first change? (I’m not sure what “$test_name” is for)

And lastly, is it worth submitting this patch? In my estimation, it’s still a unique enough match that it shouldn’t scarf up things it shouldn’t, so it might be generally useful to others trying to work with foreign ticketing systems, while not bothering anyone who isn’t.

I can run this way in a test instance for a while, but my major concern is screwing up on real-world inbound mail, which I can’t easily test.

I’ll be monitoring this list on the web, but a direct reply would be appreciated anyway.

Thanks,
/a

Your approach could work (you don’t mention which version of RT, recent
versions changed incoming email handling).

Another approach to limit the scope of your change would be to search
for incoming mail only from that email address (or domain, if there are
many) and update the subject to put the space back in. The main
advantages here are: 1) you limit the scope of the change to just email
from that address and 2) you’re making it look like RT expects, which is
less likely to miss some other location that assumes subject will be in
the specified format.

You could do this with some tool before the email gets into RT or by
adding an email processing step somewhere before the code below.

Either way, if you make changes, do it in an override library in local,
not in the main code. That way it’s simple to revert if it doesn’t
behave as you expect.On 9/28/16 1:53 PM, Alexis Rosen wrote:

Hi. I’m trying to make RT play nice with a certain large telecom vendor’s ticketing system. They’re trying to play nice too, but they’re too dumb to get it right. :frowning: In particular, they’re turning our “[tag #12345]” into “[tag#12345]” on subject lines. So I want to relax the requirement for whitespace between the tag and the ticket number.

I was a bit lazy for a cold source-dive, but fortunately google turned up a commit a few years back that looked like it was touching exactly the code I wanted, lib/RT/Interface/Email.pm: ParseTicketId().

It looks like the changes I need to make are:

Email.pm 2015-10-19 13:38:13.000000000 -0400
+++ Email.test.pm 2016-09-28 12:50:06.000000000 -0400
@@ -1139,11 +1139,11 @@
# We use @captures and pull out the last capture value to guard against
# someone using (…) instead of (?:…) in $EmailSubjectTagRegex.
my $id;

  • if ( my @captures = $Subject =~ /[$test_name\s+#(\d+)\s*]/i ) {
  • if ( my @captures = $Subject =~ /[$test_name\s*#(\d+)\s*]/i ) {
    $id = $captures[-1];
    } else {
    foreach my $tag ( RT->System->SubjectTag ) {
  •        next unless my @captures = $Subject =~ /\[\Q$tag\E\s+\#(\d+)\s*\]/i;
    
  •        next unless my @captures = $Subject =~ /\[\Q$tag\E\s*\#(\d+)\s*\]/i;
          $id = $captures[-1];
          last;
      }
    

In other words, just replace a “+” with a “*”, so it’ll match no whitespace as well as some.

So, questions:

  1. Does this make sense?
  2. Am I likely to screw anything up by doing this?
  3. Did I leave out something else that needs to be adjusted, besides ParseTicketId()?
  4. Do I really need the first change? (I’m not sure what “$test_name” is for)

And lastly, is it worth submitting this patch? In my estimation, it’s still a unique enough match that it shouldn’t scarf up things it shouldn’t, so it might be generally useful to others trying to work with foreign ticketing systems, while not bothering anyone who isn’t.

I can run this way in a test instance for a while, but my major concern is screwing up on real-world inbound mail, which I can’t easily test.

I’ll be monitoring this list on the web, but a direct reply would be appreciated anyway.

Thanks,
/a

RT 4.4 and RTIR training sessions, and a new workshop day! https://bestpractical.com/training

  • Boston - October 24-26
  • Los Angeles - Q1 2017

Your approach could work (you don’t mention which version of RT, recent versions changed incoming email handling).

Thanks for the response. The version is 4.2.12.

Another approach to limit the scope of your change would be to search for incoming mail only from that email address (or domain, if there are many) and update the subject to put the space back in. The main advantages here are: 1) you limit the scope of the change to just email from that address and 2) you’re making it look like RT expects, which is less likely to miss some other location that assumes subject will be in the specified format.

You think some other part of the code duplicates the function of ParseTicketId()?

I’m not worried about mail from other addresses. If their Subject: is matched by that regex, I want it handled the same way.

You could do this with some tool before the email gets into RT or by adding an email processing step somewhere before the code below.

Either way, if you make changes, do it in an override library in local, not in the main code. That way it’s simple to revert if it doesn’t behave as you expect.

Can you point me to a doc describing “override libraries”? This is my first look at RT source and I’m not familiar with how it’s organized. Google is less than helpful here, so far.

I take it from this that you’re not interested in merging the patch?

Thanks,
/a

Can you point me to a doc describing “override libraries”? This is my first look at RT source and I’m not familiar with how it’s organized. Google is less than helpful here, so far.

It’s mentioned here briefly:

https://docs.bestpractical.com/rt/4.2.13/RT/StyleGuide.html#The-Overlay-mechanism

To be more specific, you can create a file
local/lib/RT/Interface/Email_Local.pm and create your own version of
ParseTicketId. RT will then use your version of that subroutine rather
than the main one.

I take it from this that you’re not interested in merging the patch?

I think this sort of change would be great in an extension. For example,
here’s another extension that provides a small bit of alternate
functionality in handling subjects:

Can you point me to a doc describing “override libraries”? This is my first look at RT source and I’m not familiar with how it’s organized. Google is less than helpful here, so far.

It’s mentioned here briefly:

https://docs.bestpractical.com/rt/4.2.13/RT/StyleGuide.html#The-Overlay-mechanism

To be more specific, you can create a file local/lib/RT/Interface/Email_Local.pm and create your own version of ParseTicketId. RT will then use your version of that subroutine rather than the main one.

Hm. I see the advantage in isolating the source from local patches. OTOH, if a new version has a changed version of function I’m altering, I’ll wind up with a hacked old version after the upgrade, not a pristine new version. Both options have disadvantages.

In any case, that brief doc is sufficient. Thanks.

I take it from this that you’re not interested in merging the patch?

I think this sort of change would be great in an extension. For example, here’s another extension that provides a small bit of alternate functionality in handling subjects:

https://metacpan.org/pod/RT::Extension::TrailingSubjectTag

Um. that looks a bit more involved that I’m ready to deal with. At least the patch is here in the mailing list if someone needs it.

Thanks again,
/a

Can you point me to a doc describing “override libraries”? This is my first look at RT source and I’m not familiar with how it’s organized. Google is less than helpful here, so far.

It’s mentioned here briefly:

https://docs.bestpractical.com/rt/4.2.13/RT/StyleGuide.html#The-Overlay-mechanism

To be more specific, you can create a file local/lib/RT/Interface/Email_Local.pm and create your own version of ParseTicketId. RT will then use your version of that subroutine rather than the main one.

Hm. I see the advantage in isolating the source from local patches. OTOH, if a new version has a changed version of function I’m altering, I’ll wind up with a hacked old version after the upgrade, not a pristine new version. Both options have disadvantages.

True, but if you modify in place and forget, then upgrade, your
modification will be overwritten when the new files are put in place.
Having the change isolated in a separate file in local/ keeps it safe
(we don’t modify local on upgrade) and makes it a little easier for you
or possibly someone else to discover when upgrading.

You’re absolutely correct that you will want to take a look at that
subroutine when you upgrade to see if it was modified and then re-port
your change into the new version if there are updates.