Hi!
I have some quibbles with some of the styling:
-
In the full-page edit UI, we should be using a more RTish key:value layout
rather than the multicolumn table
-
I’m not sure “Reminder” is the right header for the table, maybe “Summary”? not sure.
-
The table looks a little weird in context
Additionally, we should work toward killing the inline mason components. (I know they’re not your fault. They were mine 8 years ago)
It looks like you may not have run tests after applying these patches, as they generated some warnings.
All that being said, this is much better than what we had before. Thanks! Applied.
Best,
JesseOn Thu, Dec 16, 2010 at 04:33:26PM +0100, Christian Loos wrote:
Hi Jesse,
attached 7 patches against 3.9-trunk.
-Chris
Am 13.12.2010 22:42, schrieb Jesse Vincent:
Patches would certainly be easier to deal with than “here’s a thing on github”
If you have the cycles to do that, I’d find time to take a look.
From 11034a1e490c426fc55957072ea4fc9a607bad9e Mon Sep 17 00:00:00 2001
From: Christian Loos cloos@netcologne.de
Date: Thu, 16 Dec 2010 13:05:08 +0100
Subject: [PATCH 1/7] reminder code cleanups
lib/RT/Reminders.pm | 70 +++++++++++++++++++++++±-------------------------
1 files changed, 34 insertions(+), 36 deletions(-)
diff --git a/lib/RT/Reminders.pm b/lib/RT/Reminders.pm
index 13dd12a…70cf67c 100644
— a/lib/RT/Reminders.pm
+++ b/lib/RT/Reminders.pm
@@ -52,7 +52,6 @@ use base qw/RT::Base/;
our $REMINDER_QUEUE = ‘General’;
sub new {
my $class = shift;
my $self = {};
@@ -61,7 +60,6 @@ sub new {
return($self);
}
sub Ticket {
my $self = shift;
$self->{‘ticket’} = shift if (@);
@@ -74,10 +72,9 @@ sub TicketObj {
$self->{’_ticketobj’} = RT::Ticket->new($self->CurrentUser);
$self->{‘_ticketobj’}->Load($self->Ticket);
}
- return $self->{‘_ticketobj’};
}
=head2 Collection
Returns an RT::Tickets object containing reminders for this object’s “Ticket”
@@ -88,8 +85,8 @@ sub Collection {
my $self = shift;
my $col = RT::Tickets->new($self->CurrentUser);
-
my $query = ‘Type = “reminder” AND RefersTo = "’.$self->Ticket.‘"’;
-
$col->FromSQL($query);
$col->OrderBy( FIELD => ‘Due’ );
@@ -107,55 +104,56 @@ Takes
Owner
Due
=cut
sub Add {
my $self = shift;
- my %args = ( Subject => undef,
-
Owner => undef,
-
Due => undef,
-
@_);
- $reminder->Create( Subject => $args{‘Subject’},
-
Owner => $args{'Owner'},
-
Due => $args{'Due'},
-
RefersTo => $self->Ticket,
-
Type => 'reminder',
-
Queue => $self->TicketObj->Queue,
-
);
- $self->TicketObj->_NewTransaction(Type => ‘AddReminder’,
-
Field => 'RT::Ticket',
-
NewValue => $reminder->id);
- $reminder->Create(
-
Subject => $args{'Subject'},
-
Owner => $args{'Owner'},
-
Due => $args{'Due'},
-
RefersTo => $self->Ticket,
-
Type => 'reminder',
-
Queue => $self->TicketObj->Queue,
- );
- $self->TicketObj->_NewTransaction(
-
Type => 'AddReminder',
-
Field => 'RT::Ticket',
-
NewValue => $reminder->id
- );
}
sub Open {
my $self = shift;
sub Resolve {
my $self = shift;
my $reminder = shift;
$reminder->SetStatus(‘resolved’);
- RT::Base->_ImportOverlays();
+RT::Base->_ImportOverlays();
1;
1.7.1
From 4b33ccc72a9a3b8199990b5439834afe999d73a2 Mon Sep 17 00:00:00 2001
From: Christian Loos cloos@netcologne.de
Date: Thu, 16 Dec 2010 13:17:15 +0100
Subject: [PATCH 2/7] only record transaction on success
lib/RT/Reminders.pm | 15 ++++++++±-----
1 files changed, 9 insertions(+), 6 deletions(-)
diff --git a/lib/RT/Reminders.pm b/lib/RT/Reminders.pm
index 70cf67c…49d8183 100644
— a/lib/RT/Reminders.pm
+++ b/lib/RT/Reminders.pm
@@ -116,7 +116,7 @@ sub Add {
);
my $reminder = RT::Ticket->new($self->CurrentUser);
- my ( $status, $msg ) = $reminder->Create(
Subject => $args{‘Subject’},
Owner => $args{‘Owner’},
Due => $args{‘Due’},
@@ -128,30 +128,33 @@ sub Add {
Type => ‘AddReminder’,
Field => ‘RT::Ticket’,
NewValue => $reminder->id
- ) if $status;
- return ( $status, $msg );
}
sub Open {
my $self = shift;
my $reminder = shift;
- $reminder->SetStatus(‘open’);
- my ( $status, $msg ) = $reminder->SetStatus(‘open’);
$self->TicketObj->_NewTransaction(
Type => ‘OpenReminder’,
Field => ‘RT::Ticket’,
NewValue => $reminder->id
- ) if $status;
- return ( $status, $msg );
}
sub Resolve {
my $self = shift;
my $reminder = shift;
- $reminder->SetStatus(‘resolved’);
- my ( $status, $msg ) = $reminder->SetStatus(‘resolved’);
$self->TicketObj->_NewTransaction(
Type => ‘ResolveReminder’,
Field => ‘RT::Ticket’,
NewValue => $reminder->id
- ) if $status;
- return ( $status, $msg );
}
RT::Base->_ImportOverlays();
1.7.1
From 6349338cfbe0d7da3d24dd50257d442a9477ceb0 Mon Sep 17 00:00:00 2001
From: Christian Loos cloos@netcologne.de
Date: Thu, 16 Dec 2010 13:36:21 +0100
Subject: [PATCH 3/7] don’t record link transaction on reminder create
Don’t record an link transaction on reminder create because the reminder
themself records an transaction of type ‘AddReminder’
lib/RT/Ticket_Overlay.pm | 2 ±
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/lib/RT/Ticket_Overlay.pm b/lib/RT/Ticket_Overlay.pm
index e95481f…22dbf20 100755
— a/lib/RT/Ticket_Overlay.pm
+++ b/lib/RT/Ticket_Overlay.pm
@@ -636,7 +636,7 @@ sub Create {
my ( $wval, $wmsg ) = $self->_AddLink(
Type => $LINKTYPEMAP{$type}->{‘Type’},
$LINKTYPEMAP{$type}->{‘Mode’} => $link,
–
1.7.1
From b27ccc7508a047aeff42a093cce8521be843e787 Mon Sep 17 00:00:00 2001
From: Christian Loos cloos@netcologne.de
Date: Thu, 16 Dec 2010 14:46:15 +0100
Subject: [PATCH 4/7] create ShowReminders element and use it in MyReminders
share/html/Elements/MyReminders | 31 ±----------
share/html/Elements/ShowReminders | 101 +++++++++++++++++++++++++++++++++++++
2 files changed, 102 insertions(+), 30 deletions(-)
create mode 100644 share/html/Elements/ShowReminders
diff --git a/share/html/Elements/MyReminders b/share/html/Elements/MyReminders
index 00ab113…8b20fbe 100755
— a/share/html/Elements/MyReminders
+++ b/share/html/Elements/MyReminders
@@ -49,40 +49,11 @@
<&|/Widgets/TitleBox,
class => ‘reminders’,
title => loc(“Reminders”) &>
-
-<%perl>
-my $i =0;
-while (my $reminder = $reminders->Next) {
-$i++;
-my $targets = RT::Tickets->new($session{‘CurrentUser’});
-$targets->FromSQL("ReferredToBy = ".$reminder->id);-if (my $ticket= $targets->First) {
+<& /Elements/ShowReminders, OnlyOverdue => 1 &>
-</%perl>
-
-
-
-% }
-% else {-<&|/l, $reminder->id &>Couldn’t find a ticket for reminder [_1].</&>
-<&|/l&>Please contact your administrator.</&>
-% }}
-
<%$reminder->Subject%>
-
-#<%$ticket->id%>: <%$ticket->Subject%>
-<& /Elements/ShowUser, User => $reminder->OwnerObj &>
-<%$reminder->DueObj->Unix >0 ? '• '.$reminder->DueObj->AgeAsString : ‘’ |n %>
-
- |
</&>
<%init>
return unless RT->Config->Get(‘EnableReminders’);
-my $reminders = RT::Tickets->new($session{‘CurrentUser’});
-$reminders->FromSQL(‘(Owner = “Nobody” OR Owner = "’.$session{‘CurrentUser’}->id.‘")’ .
From 7f4c4e01fa7fec82b485a4ac69c0f27a449230c2 Mon Sep 17 00:00:00 2001
From: Christian Loos cloos@netcologne.de
Date: Thu, 16 Dec 2010 15:19:26 +0100
Subject: [PATCH 5/7] create reminder view tool and link it to MyReminders
share/html/Elements/MyReminders | 3 ±
share/html/Elements/Tabs | 6 ++++
share/html/Tools/MyReminders.html | 55 +++++++++++++++++++++++++++++++++++++
3 files changed, 63 insertions(+), 1 deletions(-)
create mode 100644 share/html/Tools/MyReminders.html
diff --git a/share/html/Elements/MyReminders b/share/html/Elements/MyReminders
index 8b20fbe…c3e4450 100755
— a/share/html/Elements/MyReminders
+++ b/share/html/Elements/MyReminders
@@ -48,7 +48,8 @@
%# DEPRECATED
<&|/Widgets/TitleBox,
class => ‘reminders’,
- title => loc(“Reminders”) &>
- title => loc(“Reminders”),
- title_href => RT->Config->Get(‘WebPath’) . ‘/Tools/MyReminders.html’ &>
<& /Elements/ShowReminders, OnlyOverdue => 1 &>
diff --git a/share/html/Elements/Tabs b/share/html/Elements/Tabs
index f9b7b86…7613fd1 100755
— a/share/html/Elements/Tabs
+++ b/share/html/Elements/Tabs
@@ -111,6 +111,12 @@ if ( $request_path !~ m{^/SelfService/} ) {
$tools->child( my_day => title => loc(‘My Day’), path => ‘/Tools/MyDay.html’, sort_order => 5,
description => loc(‘Easy updating of your open tickets’) );
- if ( RT->Config->Get(‘EnableReminders’) )
- {
-
$tools->child( my_reminders => title => loc('My Reminders'), path => '/Tools/MyReminders.html', sort_order => 6,
-
description => loc('Easy viewing of your reminders') );
- }
- $tools->child( offline => title => loc(‘Offline’), path => ‘/Tools/Offline.html’, sort_order => 10,
description => loc(‘Create tickets offline’) );
diff --git a/share/html/Tools/MyReminders.html b/share/html/Tools/MyReminders.html
new file mode 100644
index 0000000…5415cdf
— /dev/null
+++ b/share/html/Tools/MyReminders.html
@@ -0,0 +1,55 @@
+%# BEGIN BPS TAGGED BLOCK {{{
+%#
+%# COPYRIGHT:
+%#
+%# This software is Copyright (c) 1996-2010 Best Practical Solutions, LLC
+%# jesse@bestpractical.com
+%#
+%# (Except where explicitly superseded by other copyright notices)
+%#
+%#
+%# LICENSE:
+%#
+%# This work is made available to you under the terms of Version 2 of
+%# the GNU General Public License. A copy of that license should have
+%# been provided with this software, but in any event can be snarfed
+%# from www.gnu.org.
+%#
+%# This work is distributed in the hope that it will be useful, but
+%# WITHOUT ANY WARRANTY; without even the implied warranty of
+%# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+%# General Public License for more details.
+%#
+%# You should have received a copy of the GNU General Public License
+%# along with this program; if not, write to the Free Software
+%# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
+%# 02110-1301 or visit their web page on the internet at
+%# GNU General Public License v2.0 - GNU Project - Free Software Foundation.
+%#
+%#
+%# CONTRIBUTION SUBMISSION POLICY:
+%#
+%# (The following paragraph is not intended to limit the rights granted
+%# to you to modify and distribute this software under the terms of
+%# the GNU General Public License and is only of importance to you if
+%# you choose to contribute your changes and enhancements to the
+%# community by submitting them to Best Practical Solutions, LLC.)
+%#
+%# By intentionally submitting any modifications, corrections or
+%# derivatives to this work, or any other work intended for use with
+%# Request Tracker, to Best Practical Solutions, LLC, you confirm that
+%# you are the copyright holder for those contributions and you grant
+%# Best Practical Solutions, LLC a nonexclusive, worldwide, irrevocable,
+%# royalty-free, perpetual, license to use, copy, create derivative
+%# works based on those contributions, and sublicense and distribute
+%# those contributions and any derivatives thereof.
+%#
+%# END BPS TAGGED BLOCK }}}
+<& /Elements/Header, Title => loc(‘My reminders’) &>
+<& /Elements/Tabs &>
+
+<& /Elements/ShowReminders, OnlyOverdue => 0 &>
+
+<%INIT>
+return unless RT->Config->Get(‘EnableReminders’);
+</%INIT>
1.7.1
From e3bff5646a328360358d7897323851b60d2e6735 Mon Sep 17 00:00:00 2001
From: Christian Loos cloos@netcologne.de
Date: Thu, 16 Dec 2010 15:26:56 +0100
Subject: [PATCH 6/7] more constistent naming of save button
share/html/Ticket/Reminders.html | 4 ++±
1 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/share/html/Ticket/Reminders.html b/share/html/Ticket/Reminders.html
index 64fef40…fae8105 100755
— a/share/html/Ticket/Reminders.html
+++ b/share/html/Ticket/Reminders.html
@@ -57,7 +57,9 @@
<& /Ticket/Elements/Reminders, Ticket => $Ticket, ShowCompleted => 1, Edit => 1 &>
</&>
-<& /Elements/Submit, Label => loc(‘Save’) &>
+<& /Elements/Submit,
- Label => loc(‘Save Changes’),
- Caption => loc(“If you’ve updated anything above, be sure to”) &>
–
1.7.1
From 1a52b4ab3efa11ccc9cbd7f8708a2afc55a09875 Mon Sep 17 00:00:00 2001
From: Christian Loos cloos@netcologne.de
Date: Thu, 16 Dec 2010 16:25:18 +0100
Subject: [PATCH 7/7] redesign ticket reminder element
share/html/Ticket/Elements/Reminders | 115 +++++++++++++++++++±-------------
1 files changed, 69 insertions(+), 46 deletions(-)
diff --git a/share/html/Ticket/Elements/Reminders b/share/html/Ticket/Elements/Reminders
index 43da890…33aa480 100644
— a/share/html/Ticket/Elements/Reminders
+++ b/share/html/Ticket/Elements/Reminders
@@ -76,23 +76,26 @@ if ( $request_args->{‘update-reminders’} ) {
$reminder->SetOwner( $request_args->{ ‘Reminder-Owner-’ . $reminder->id } , “Force” ) ;
}
-
if ( exists( $request_args->{ 'Reminder-Due-' . $reminder->id } ) && ( $reminder->DueObj->Date ne $request_args->{ 'Reminder-Due-' . $reminder->id } )) {
-
$reminder->SetDue( $request_args->{ 'Reminder-Due-' . $reminder->id } ) ;
-
if ( exists( $request_args->{ 'Reminder-Due-' . $reminder->id } ) && $request_args->{ 'Reminder-Due-' . $reminder->id } ne '' ) {
-
my $DateObj = RT::Date->new( $session{'CurrentUser'} );
-
$DateObj->Set(
-
Format => 'unknown',
-
Value => $request_args->{ 'Reminder-Due-' . $reminder->id }
-
);
-
if ( defined $DateObj->Unix && $DateObj->Unix != $reminder->DueObj->Unix ) {
-
$reminder->SetDue( $DateObj->ISO );
-
}
}
}
}
if ( $request_args->{‘NewReminder-Subject’} ) {
my $due_obj = RT::Date->new( $session{‘CurrentUser’} );
- my $date = Time::ParseDate::parsedate(
-
$request_args->{'NewReminder-Due'},
-
UK => RT->Config->Get('DateDayBeforeMonth'),
-
PREFER_PAST => 0,
-
PREFER_FUTURE => 1
@@ -105,42 +108,51 @@ $reminder_collection = $Ticket->Reminders->Collection;
</%init>
-
-% while (my $reminder = $reminder_collection->Next) {
-% if ($reminder->Status eq ‘resolved’ && !$ShowCompleted) {
-
-% } elsif ($Edit) {
-<& SELF:EditEntry, Reminder => $reminder, Ticket => $Ticket &>
-% } else {
-<& SELF:ShowEntry, Reminder => $reminder, Ticket => $Ticket &>
-% }
+<table border=“0” cellpadding=“1” cellspacing=“0” class=“collection-as-table”<% $Edit ? ’ style=“width: auto;”’ : ‘’ |n %>>
+
+% if ( $Edit ) {
+<&|/l&>Reminders</&>
+% } else {
+
+<&|/l&>Reminder</&>
+<&|/l&>Due</&>
+<&|/l&>Owner</&>
+% }
+
+% my ( $i, $visible ) = 0;
+% while ( my $reminder = $reminder_collection->Next ) {
+% $i++;
+% if ( $reminder->Status eq ‘resolved’ && !$ShowCompleted ) {
+
+% $i++;
+% } elsif ($Edit) {
+<& SELF:EditEntry, Reminder => $reminder, Ticket => $Ticket, Index => $i &>
+% $visible++;
+% } else {
+<& SELF:ShowEntry, Reminder => $reminder, Ticket => $Ticket, Index => $i &>
+% $visible++;
% }
-% if ($reminder_collection->Count) {
-<&|/l&>(Check box to delete)</&>
% }
-
-
+
+% if ( $visible > 0 ) {
+<&|/l&>(Check box to complete)</&>
+% }
<&|/l&>New reminder:</&>
<& SELF:NewReminder, Ticket => $Ticket &>
-
<%method NewReminder>
<%args>
$Ticket
</%args>
-
-
-
+
+
+
-
-
-
-
-
-
+
+
+
+
+<&|/l&>Subject: | |
<&|/l&>Subject: | |
-<&|/l&>Owner: |
-<& /Elements/SelectOwner, Name => 'NewReminder-Owner', QueueObj => $Ticket->QueueObj, Default=>$session{'CurrentUser'}->id, DefaultValue => 0 &>
- |
<&|/l&>Due: | <& /Elements/SelectDate, Name => "NewReminder-Due", Default => "" &> |
<&|/l&>Owner: | <& /Elements/SelectOwner, Name => 'NewReminder-Owner', QueueObj => $Ticket->QueueObj, Default=>$session{'CurrentUser'}->id, DefaultValue => 0 &> | <&|/l&>Due: | <& /Elements/SelectDate, Name => "NewReminder-Due", Default => "" &> |
@@ -148,22 +160,33 @@ $Ticket
<%args>
$Reminder
$Ticket
+$Index
-Status eq 'resolved' ? 'checked="checked"' : '' %> />
- •
- <& /Elements/SelectOwner, Name => 'Reminder-Owner-'.$Reminder->id, Queue => $Ticket->QueueObj, Default => $Reminder->Owner, DefaultValue => 0 &>
- <& /Elements/SelectDate, Name => 'Reminder-Due-'.$Reminder->id, Default => $Reminder->DueObj->Date &>
- (<%$Reminder->DueObj->Unix>0 ? $Reminder->DueObj->AgeAsString : '' %>)
+
+Status eq 'resolved' ? 'checked="checked"' : '' |n %> />
+<&|/l&>Subject:
+
+
+
+
+<&|/l&>Owner:
+<& /Elements/SelectOwner, Name => 'Reminder-Owner-'.$Reminder->id, QueueObj => $Ticket->QueueObj, Default => $Reminder->Owner, DefaultValue => 0 &>
+<&|/l&>Due:
+<& /Elements/SelectDate, Name => 'Reminder-Due-'.$Reminder->id &> (<% $Reminder->DueObj->AsString %>)
+
<%method ShowEntry>
<%args>
$Reminder
$Ticket
+$Index
-Status eq 'resolved' ? 'checked="checked"' : '' %> />
- <%$Reminder->Subject%> • \
- <& /Elements/ShowUser, User => $Reminder->OwnerObj &> \
- <%$Reminder->DueObj->Unix>0 ? "• ". $Reminder->DueObj->AgeAsString : '' |n%>
+% my $dueobj = $Reminder->DueObj;
+% my $overdue = $dueobj->Unix > 0 && $dueobj->Diff < 0 ? 1 : 0;
+
+Status eq 'resolved' ? 'checked="checked"' : '' |n %> />
+<% $Reminder->Subject %>
+<% $overdue ? '' : '' |n %><% $dueobj->AgeAsString || loc('Not set') %><% $overdue ? '' : '' |n %>
+<& /Elements/ShowUser, User => $Reminder->OwnerObj &>
+
--
1.7.1