From bbe1fdc1376dfa19d7d4e31014c12f06d32593be Mon Sep 17 00:00:00 2001 From: Damien Regad Date: Thu, 15 Jan 2015 16:11:56 +0100 Subject: [PATCH 1/3] Use defined constants instead of magic numbers --- core/timeline_inc.php | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/core/timeline_inc.php b/core/timeline_inc.php index 65a351a784..1071cbef40 100644 --- a/core/timeline_inc.php +++ b/core/timeline_inc.php @@ -17,11 +17,13 @@ require_once( 'core.php' ); require_api( 'timeline_api.php' ); +define( 'MAX_EVENTS', 50 ); + $f_days = gpc_get_int( 'days', 0 ); $f_all = gpc_get_int( 'all', 0 ); -$t_end_time = time() - ( $f_days * 24 * 60 * 60 ); -$t_start_time = $t_end_time - ( 7 * 24 * 60 * 60 ); +$t_end_time = time() - ( $f_days * SECONDS_PER_DAY ); +$t_start_time = $t_end_time - ( 7 * SECONDS_PER_DAY ); $t_events = timeline_events( $t_start_time, $t_end_time ); echo '
'; @@ -45,7 +47,7 @@ $t_events = timeline_sort_events( $t_events ); if ( $f_all == 0 ) { - $t_events = array_slice( $t_events, 0, 50 ); + $t_events = array_slice( $t_events, 0, MAX_EVENTS ); } if( count( $t_events ) > 0 ) { From 523bf91d6195cb73db59d2e0c8d7c858120e3b3e Mon Sep 17 00:00:00 2001 From: Damien Regad Date: Mon, 19 Jan 2015 19:37:48 +0100 Subject: [PATCH 2/3] Fix number of displayed events lower than 50 Depending on the types of events displayed, in some cases the timeline displayed fewer than 50 entries even though more are available. This is because the 'Status Change' timeline events are only printed for some status types (RESOLVED, CLOSED, REOPENED); prior to this, other types were printed as an empty string. This adds a new private method in IssueStatusChangeTimelineEvent class to determine the type of status change in the constructor and store it as a property, which is then used to reimplement the skip() method as well as to drive html generation. This allows the timeline_events() function to exclude unwanted entries with skip(). In addition, the logic for limiting the number of items to display is now implemented in timeline_print_events() instead of timeline_inc.php. Fixes #18034 --- .../IssueStatusChangeTimelineEvent.class.php | 72 ++++++++++++++++--- core/classes/TimelineEvent.class.php | 9 +++ core/timeline_api.php | 21 ++++-- core/timeline_inc.php | 10 +-- 4 files changed, 91 insertions(+), 21 deletions(-) diff --git a/core/classes/IssueStatusChangeTimelineEvent.class.php b/core/classes/IssueStatusChangeTimelineEvent.class.php index a7e13043b7..67dec64a97 100644 --- a/core/classes/IssueStatusChangeTimelineEvent.class.php +++ b/core/classes/IssueStatusChangeTimelineEvent.class.php @@ -31,6 +31,16 @@ class IssueStatusChangeTimelineEvent extends TimelineEvent { private $issue_id; private $old_status; private $new_status; + private $type; + + /** + * Status change types to be displayed in Timeline + * IGNORED = not displayed + */ + const IGNORED = 0; + const RESOLVED = 1; + const CLOSED = 2; + const REOPENED = 3; /** * @param integer $p_timestamp Timestamp representing the time the event occurred. @@ -45,24 +55,70 @@ public function __construct( $p_timestamp, $p_user_id, $p_issue_id, $p_old_statu $this->issue_id = $p_issue_id; $this->old_status = $p_old_status; $this->new_status = $p_new_status; + $this->type = $this->change_type(); } /** - * Returns html string to display - * @return string + * Return the type of status change + * @return int One of the status change constants defined above */ - public function html() { + private function change_type() { $t_resolved = config_get( 'bug_resolved_status_threshold' ); $t_closed = config_get( 'bug_closed_status_threshold' ); if( $this->old_status < $t_closed && $this->new_status >= $t_closed ) { - $t_string = sprintf( lang_get( 'timeline_issue_closed' ), user_get_name( $this->user_id ), string_get_bug_view_link( $this->issue_id ) ); + return IssueStatusChangeTimelineEvent::CLOSED; } else if( $this->old_status < $t_resolved && $this->new_status >= $t_resolved ) { - $t_string = sprintf( lang_get( 'timeline_issue_resolved' ), user_get_name( $this->user_id ), string_get_bug_view_link( $this->issue_id ) ); + return IssueStatusChangeTimelineEvent::RESOLVED; } else if( $this->old_status >= $t_resolved && $this->new_status < $t_resolved ) { - $t_string = sprintf( lang_get( 'timeline_issue_reopened' ), user_get_name( $this->user_id ), string_get_bug_view_link( $this->issue_id ) ); + return IssueStatusChangeTimelineEvent::REOPENED; } else { - return ''; + return IssueStatusChangeTimelineEvent::IGNORED; + } + } + + /** + * Whether to skip this timeline event. + * This normally implements access checks for the event. + * @return boolean + */ + public function skip() { + return $this->type == IssueStatusChangeTimelineEvent::IGNORED; + } + + /** + * Returns html string to display + * @return string + */ + public function html() { + switch( $this->type ) { + case IssueStatusChangeTimelineEvent::RESOLVED: + $t_string = sprintf( + lang_get( 'timeline_issue_resolved' ), + user_get_name( $this->user_id ), + string_get_bug_view_link( $this->issue_id ) + ); + break; + case IssueStatusChangeTimelineEvent::CLOSED: + $t_string = sprintf( + lang_get( 'timeline_issue_closed' ), + user_get_name( $this->user_id ), + string_get_bug_view_link( $this->issue_id ) + ); + break; + case IssueStatusChangeTimelineEvent::REOPENED: + $t_string = sprintf( + lang_get( 'timeline_issue_reopened' ), + user_get_name( $this->user_id ), + string_get_bug_view_link( $this->issue_id ) + ); + break; + case IssueStatusChangeTimelineEvent::IGNORED: + return ''; + default: + # Unknown status change type + trigger_error( ERROR_GENERIC, ERROR ); + return ''; } $t_html = $this->html_start(); @@ -71,4 +127,4 @@ public function html() { return $t_html; } -} \ No newline at end of file +} diff --git a/core/classes/TimelineEvent.class.php b/core/classes/TimelineEvent.class.php index 9a344d6e02..3c84b73374 100644 --- a/core/classes/TimelineEvent.class.php +++ b/core/classes/TimelineEvent.class.php @@ -69,6 +69,15 @@ public function compare( TimelineEvent $p_other ) { return 0; } + /** + * Whether to skip this timeline event. + * This normally implements access checks for the event. + * @return boolean + */ + public function skip() { + return false; + } + /** * Returns html string to display * @return string diff --git a/core/timeline_api.php b/core/timeline_api.php index c2d9b41d12..ae5376f8ba 100644 --- a/core/timeline_api.php +++ b/core/timeline_api.php @@ -68,6 +68,7 @@ function timeline_get_affected_issues( $p_start_time, $p_end_time ) { /** * Get an array of timeline events + * Events for which the skip() method returns true will be excluded * @param integer $p_start_time Timestamp representing start time of the period. * @param integer $p_end_time Timestamp representing end time of the period. * @return array @@ -127,7 +128,8 @@ function timeline_events( $p_start_time, $p_end_time ) { break; } - if( $t_event != null ) { + # Do not include skipped events + if( $t_event != null && !$t_event->skip() ) { $t_timeline_events[] = $t_event; } } @@ -163,11 +165,22 @@ function timeline_sort_events( array $p_events ) { /** * Print for display an array of events - * @param array $p_events Array of events to display. + * @param array $p_events Array of events to display + * @param int $p_max_num Maximum number of events to display, 0 = all * @return void */ -function timeline_print_events( array $p_events ) { - foreach ( $p_events as $t_event ) { +function timeline_print_events( array $p_events, $p_max_num = 0 ) { + if( empty( $p_events ) ) { + echo '

' . lang_get( 'timeline_no_activity' ) . '

'; + return; + } + + $i = 0; + foreach( $p_events as $t_event ) { + # Stop displaying events if we're reached the maximum + if( $p_max_num && $i++ >= $p_max_num ) { + break; + } echo $t_event->html(); } } diff --git a/core/timeline_inc.php b/core/timeline_inc.php index 1071cbef40..8f42568025 100644 --- a/core/timeline_inc.php +++ b/core/timeline_inc.php @@ -46,15 +46,7 @@ echo '
' . date( $t_short_date_format, $t_start_time ) . ' .. ' . date( $t_short_date_format, $t_end_time ) . $t_prev_link . $t_next_link . '
'; $t_events = timeline_sort_events( $t_events ); -if ( $f_all == 0 ) { - $t_events = array_slice( $t_events, 0, MAX_EVENTS ); -} - -if( count( $t_events ) > 0 ) { - timeline_print_events( $t_events ); -} else { - echo '

' . lang_get( 'timeline_no_activity' ) . '

'; -} +timeline_print_events( $t_events, ( $f_all ? 0 : MAX_EVENTS ) ); if( $f_all == 0 ) { echo '

' . $t_prev_link = ' [ ' . lang_get( 'timeline_more' ) . ' ]

'; From 8139fa85c39a09e6201fec7e448055702210431d Mon Sep 17 00:00:00 2001 From: Damien Regad Date: Mon, 19 Jan 2015 19:54:24 +0100 Subject: [PATCH 3/3] Only display "More events" link when needed If the timeline is showing all available events, the presence of the link is unnecessary and confusing to users. Fixes #18035 --- core/timeline_api.php | 5 +++-- core/timeline_inc.php | 7 +++++-- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/core/timeline_api.php b/core/timeline_api.php index ae5376f8ba..701b0227de 100644 --- a/core/timeline_api.php +++ b/core/timeline_api.php @@ -167,12 +167,12 @@ function timeline_sort_events( array $p_events ) { * Print for display an array of events * @param array $p_events Array of events to display * @param int $p_max_num Maximum number of events to display, 0 = all - * @return void + * @return int Number of displayed events */ function timeline_print_events( array $p_events, $p_max_num = 0 ) { if( empty( $p_events ) ) { echo '

' . lang_get( 'timeline_no_activity' ) . '

'; - return; + return 0; } $i = 0; @@ -183,5 +183,6 @@ function timeline_print_events( array $p_events, $p_max_num = 0 ) { } echo $t_event->html(); } + return min( $p_max_num, $i); } diff --git a/core/timeline_inc.php b/core/timeline_inc.php index 8f42568025..e51876e1bc 100644 --- a/core/timeline_inc.php +++ b/core/timeline_inc.php @@ -46,9 +46,12 @@ echo '
' . date( $t_short_date_format, $t_start_time ) . ' .. ' . date( $t_short_date_format, $t_end_time ) . $t_prev_link . $t_next_link . '
'; $t_events = timeline_sort_events( $t_events ); -timeline_print_events( $t_events, ( $f_all ? 0 : MAX_EVENTS ) ); +$t_num_events = timeline_print_events( $t_events, ( $f_all ? 0 : MAX_EVENTS ) ); -if( $f_all == 0 ) { +# Don't display "More Events" link if there are no more entries to show +# Note: as of 2015-01-19, this does not cover the case of entries excluded +# by filtering (e.g. Status Change not in RESOLVED, CLOSED, REOPENED) +if( !$f_all && $t_num_events < count( $t_events )) { echo '

' . $t_prev_link = ' [ ' . lang_get( 'timeline_more' ) . ' ]

'; }