My first code contribution : approvals

RT Developers
1

Hi,

After one day of docs reading, I am proud to send to the list my first diff
:-))

As I don’t fully understand everything I did, I welcome all reviews and
comments about what I’ve done wrong here and how it should have been done
instead. I am not at all an HTML, Perl, Mason or RT guru.

The goal :
On the dependencies side of the ticket display page, links are all to
http://your.rt.com/Ticket/Display.html?id=xxxx, wether the ticket is a
standard ticket or an approval. When the ticket is an approval, you are then
not redirected to the proper page to approve or reject the ticket, the url
should read http://your.rt.com/Approvals/Display.html?id=xxxx.

The patch to fsck_com_rt.pm attached corrects this.

I noticed that the ticket “type” is either “ticket” or “Approval” (note the
upercase “A” and lowercase “t”)
Also, the URL is “/Ticket/” or “/Approvals/” (singular and plural).
These are really minor details.

The risks :

I have no idea what else I may have broken with my code… my RT instance
seems ok
I suspect some performance issues : I feel I access the database to load
tickets when they had already been loaded earlier in the process of
converting Ids to urls. If this is the case, I am wasting resources.

Blaise

fsck_com_rt.pm.diff (943 Bytes)

2

This won’t make it into 3.0.0, but I can see a patch like it going in
later in the 3.0.x series:

sub HREF {
my $self = shift;

  • my $ticket;
    if ($self->IsLocal) {
  •    return ( $RT::WebURL .  "Ticket/Display.html?id=".$self->Object->Id);

  •    $ticket = RT::Ticket->new($self->CurrentUser);

  •    $ticket->Load($self->Object->Id);

I believe all that can be replaced with

my $ticket = $self->Object

  •    if ($ticket->Type =~ /pproval/) {

      if ($ticket->Type =~ /^approval$/i) { 

is the case-insensitive version of all that.

  •        return ( $RT::WebURL .  "Approvals/Display.html?id=".$self->Object->Id);

  •    }

  •    else {

  •        return ( $RT::WebURL .  "Ticket/Display.html?id=".$self->Object->Id);

  •    }
    }

I suspect some performance issues : I feel I access the database to load
tickets when they had already been loaded earlier in the process of
converting Ids to urls. If this is the case, I am wasting resources.

There’s a caching layer in place that should take care of that

Request Tracker... So much more than a help desk — Best Practical Solutions – Trouble Ticketing. Free.

3 
  •    if ($ticket->Type =~ /pproval/) {

    if ($ticket->Type =~ /^approval$/i) {

is the case-insensitive version of all that.

Not to nitpick, but I’ll prolly write it as

      if (lc($ticket->Type) eq 'approval') {

but really, I don’t see the point of case insensitiveness…

Thanks,
/Autrijus/

4

if (lc($ticket->Type) eq ‘approval’) {

but really, I don’t see the point of case insensitiveness…

It’s only because the database record is “Approval” and not “approval” as
expected. If it ever changes, the code would break.

Blaise

5

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

  • THAUVIN Blaise (Dir. Informatique) [2003-03-24 02:31]:
    if (lc($ticket->Type) eq 'approval') {

          ^^

but really, I don’t see the point of case insensitiveness…

It’s only because the database record is “Approval” and not “approval” as
expected. If it ever changes, the code would break.

No, it wouldn’t – he’s using lc.

(darren)

We must respect the other fellow’s religion, but only in the sense and
to the extent that we respect his theory that his wife is beautiful
and his children smart.
– H.L.Mencken
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.1 (SunOS)

iD8DBQE+fxuyzsinjrVhZaoRAlW+AKCcPoSNljNYK4vNBcSCAHx0mI0NGgCgpb0O
xcKBNRheDIyi6vRIl7tCkLQ=
=0syb
-----END PGP SIGNATURE-----