From d5bf4071b5d878b44a97fbcd668378837bdf14ca Mon Sep 17 00:00:00 2001 From: Victor Boctor Date: Fri, 3 Jul 2015 21:59:19 -0700 Subject: [PATCH 1/8] history_get_raw_events_array() shoudl span issues Enable history_get_raw_events_array() to query history events within a time range independent of the issue number. That is in addition to the standard mode of getting all history events for an issue. --- core/history_api.php | 40 +++++++++++++++++++++++++--------------- 1 file changed, 25 insertions(+), 15 deletions(-) diff --git a/core/history_api.php b/core/history_api.php index 67dc838b5f..8062c98cae 100644 --- a/core/history_api.php +++ b/core/history_api.php @@ -179,7 +179,8 @@ function history_count_user_recent_events( $p_duration_in_seconds, $p_user_id = * Retrieves the raw history events for the specified bug id and returns it in an array * The array is indexed from 0 to N-1. The second dimension is: 'date', 'userid', 'username', * 'field','type','old_value','new_value' - * @param integer $p_bug_id A valid bug identifier. + * @param integer $p_bug_id A valid bug identifier or null to not filter by bug. If no bug id is specified, + * then returned array will have a field for bug_id, otherwise it won't. * @param integer $p_user_id A valid user identifier. * @param integer $p_start_time The start time to filter by, or null for all. * @param integer $p_end_time The end time to filter by, or null for all. @@ -200,11 +201,15 @@ function history_get_raw_events_array( $p_bug_id, $p_user_id = null, $p_start_ti # bug we will find the line related to the relationship mixed with the custom fields (the history is creted # for the new bug with the same timestamp...) - $t_params = array( $p_bug_id ); + $t_query = 'SELECT * FROM {bug_history}'; + $t_params = array(); + $t_where = array(); - $t_query = 'SELECT * FROM {bug_history} WHERE bug_id=' . db_param(); + if ( $p_bug_id !== null ) { + $t_where[] = 'bug_id=' . db_param(); + $t_params = array( $p_bug_id ); + } - $t_where = array(); if ( $p_start_time !== null ) { $t_where[] = 'date_modified >= ' . db_param(); $t_params[] = $p_start_time; @@ -216,7 +221,7 @@ function history_get_raw_events_array( $p_bug_id, $p_user_id = null, $p_start_ti } if ( count( $t_where ) > 0 ) { - $t_query .= ' AND ' . implode( ' AND ', $t_where ); + $t_query .= ' WHERE ' . implode( ' AND ', $t_where ); } $t_query .= ' ORDER BY date_modified ' . $t_history_order . ',id'; @@ -224,7 +229,7 @@ function history_get_raw_events_array( $p_bug_id, $p_user_id = null, $p_start_ti $t_result = db_query( $t_query, $t_params ); $t_raw_history = array(); - $t_private_bugnote_visible = access_has_bug_level( config_get( 'private_bugnote_threshold' ), $p_bug_id, $t_user_id ); + $t_private_bugnote_threshold = config_get( 'private_bugnote_threshold' ); $t_tag_view_threshold = config_get( 'tag_view_threshold' ); $t_view_attachments_threshold = config_get( 'view_attachments_threshold' ); $t_show_monitor_list_threshold = config_get( 'show_monitor_list_threshold' ); @@ -239,20 +244,20 @@ function history_get_raw_events_array( $p_bug_id, $p_user_id = null, $p_start_ti if( !in_array( $v_field_name, $t_standard_fields ) ) { # check that the item should be visible to the user $t_field_id = custom_field_get_id_from_name( $v_field_name ); - if( false !== $t_field_id && !custom_field_has_read_access( $t_field_id, $p_bug_id, $t_user_id ) ) { + if( false !== $t_field_id && !custom_field_has_read_access( $t_field_id, $v_bug_id, $t_user_id ) ) { continue; } } - if( ( $v_field_name == 'target_version' ) && !access_has_bug_level( $t_roadmap_view_access_level, $p_bug_id, $t_user_id ) ) { + if( ( $v_field_name == 'target_version' ) && !access_has_bug_level( $t_roadmap_view_access_level, $v_bug_id, $t_user_id ) ) { continue; } - if( ( $v_field_name == 'due_date' ) && !access_has_bug_level( $t_due_date_view_threshold, $p_bug_id, $t_user_id ) ) { + if( ( $v_field_name == 'due_date' ) && !access_has_bug_level( $t_due_date_view_threshold, $v_bug_id, $t_user_id ) ) { continue; } - if( ( $v_field_name == 'handler_id' ) && !access_has_bug_level( $t_show_handler_threshold, $p_bug_id, $t_user_id ) ) { + if( ( $v_field_name == 'handler_id' ) && !access_has_bug_level( $t_show_handler_threshold, $v_bug_id, $t_user_id ) ) { continue; } } @@ -261,13 +266,13 @@ function history_get_raw_events_array( $p_bug_id, $p_user_id = null, $p_start_ti if( $t_user_id != $v_user_id ) { # bypass if user originated note if( ( $v_type == BUGNOTE_ADDED ) || ( $v_type == BUGNOTE_UPDATED ) || ( $v_type == BUGNOTE_DELETED ) ) { - if( !$t_private_bugnote_visible && ( bugnote_get_field( $v_old_value, 'view_state' ) == VS_PRIVATE ) ) { + if( !access_has_bug_level( $t_private_bugnote_threshold, $v_bug_id, $t_user_id ) && ( bugnote_get_field( $v_old_value, 'view_state' ) == VS_PRIVATE ) ) { continue; } } if( $v_type == BUGNOTE_STATE_CHANGED ) { - if( !$t_private_bugnote_visible && ( bugnote_get_field( $v_new_value, 'view_state' ) == VS_PRIVATE ) ) { + if( !access_has_bug_level( $t_private_bugnote_threshold, $v_bug_id, $t_user_id ) && ( bugnote_get_field( $v_new_value, 'view_state' ) == VS_PRIVATE ) ) { continue; } } @@ -275,21 +280,21 @@ function history_get_raw_events_array( $p_bug_id, $p_user_id = null, $p_start_ti # tags if( $v_type == TAG_ATTACHED || $v_type == TAG_DETACHED || $v_type == TAG_RENAMED ) { - if( !access_has_bug_level( $t_tag_view_threshold, $p_bug_id, $t_user_id ) ) { + if( !access_has_bug_level( $t_tag_view_threshold, $v_bug_id, $t_user_id ) ) { continue; } } # attachments if( $v_type == FILE_ADDED || $v_type == FILE_DELETED ) { - if( !access_has_bug_level( $t_view_attachments_threshold, $p_bug_id, $t_user_id ) ) { + if( !access_has_bug_level( $t_view_attachments_threshold, $v_bug_id, $t_user_id ) ) { continue; } } # monitoring if( $v_type == BUG_MONITOR || $v_type == BUG_UNMONITOR ) { - if( !access_has_bug_level( $t_show_monitor_list_threshold, $p_bug_id, $t_user_id ) ) { + if( !access_has_bug_level( $t_show_monitor_list_threshold, $v_bug_id, $t_user_id ) ) { continue; } } @@ -305,6 +310,11 @@ function history_get_raw_events_array( $p_bug_id, $p_user_id = null, $p_start_ti } } + # if history is not scoped to a single bug, then include the bug id in the returned array for each element. + if ( $p_bug_id === null ) { + $t_raw_history[$j]['bug_id'] = $v_bug_id; + } + $t_raw_history[$j]['date'] = $v_date_modified; $t_raw_history[$j]['userid'] = $v_user_id; From 8c9114e9f5cf511f646e43fc6326272df74a7800 Mon Sep 17 00:00:00 2001 From: Victor Boctor Date: Fri, 3 Jul 2015 22:00:56 -0700 Subject: [PATCH 2/8] Optimize timeline_events() performance Change timeline_events() to work directory on history events rather than finding issues that are modified within a date range, then getting history entries for such issues, and then filtering them. --- core/timeline_api.php | 47 ++----------------------------------------- 1 file changed, 2 insertions(+), 45 deletions(-) diff --git a/core/timeline_api.php b/core/timeline_api.php index 701b0227de..dd383fcf00 100644 --- a/core/timeline_api.php +++ b/core/timeline_api.php @@ -31,41 +31,6 @@ require_api( 'bug_api.php' ); require_api( 'history_api.php' ); -/** - * Get list of affected issues between a given time period - * @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 - */ -function timeline_get_affected_issues( $p_start_time, $p_end_time ) { - $t_query = 'SELECT DISTINCT(bug_id) from {bug_history} WHERE date_modified >= ' . db_param() . ' AND date_modified < ' . db_param(); - $t_result = db_query( $t_query, array( $p_start_time, $p_end_time ) ); - - $t_current_project = helper_get_current_project(); - - $t_all_issue_ids = array(); - while( ( $t_row = db_fetch_array( $t_result ) ) !== false ) { - $t_all_issue_ids[] = $t_row['bug_id']; - } - - bug_cache_array_rows( $t_all_issue_ids ); - - $t_issue_ids = array(); - foreach( $t_all_issue_ids as $t_issue_id ) { - if( $t_current_project != ALL_PROJECTS && $t_current_project != bug_get_field( $t_issue_id, 'project_id' ) ) { - continue; - } - - if( !access_has_bug_level( config_get( 'view_bug_threshold' ), $t_issue_id ) ) { - continue; - } - - $t_issue_ids[] = $t_issue_id; - } - - return $t_issue_ids; -} - /** * Get an array of timeline events * Events for which the skip() method returns true will be excluded @@ -74,23 +39,16 @@ function timeline_get_affected_issues( $p_start_time, $p_end_time ) { * @return array */ function timeline_events( $p_start_time, $p_end_time ) { - $t_issue_ids = timeline_get_affected_issues( $p_start_time, $p_end_time ); - $t_timeline_events = array(); - foreach ( $t_issue_ids as $t_issue_id ) { - $t_history_events_array = history_get_raw_events_array( $t_issue_id, null, $p_start_time, $p_end_time ); + $t_history_events_array = history_get_raw_events_array( null, null, $p_start_time, $p_end_time ); $t_history_events_array = array_reverse( $t_history_events_array ); foreach ( $t_history_events_array as $t_history_event ) { - if( $t_history_event['date'] < $p_start_time || - $t_history_event['date'] >= $p_end_time ) { - continue; - } - $t_event = null; $t_user_id = $t_history_event['userid']; $t_timestamp = $t_history_event['date']; + $t_issue_id = $t_history_event['bug_id']; switch( $t_history_event['type'] ) { case NEW_BUG: @@ -133,7 +91,6 @@ function timeline_events( $p_start_time, $p_end_time ) { $t_timeline_events[] = $t_event; } } - } return $t_timeline_events; } From 2afb86370c96449805dc31e1e237e24b23931b80 Mon Sep 17 00:00:00 2001 From: Victor Boctor Date: Mon, 6 Jul 2015 11:50:15 +1000 Subject: [PATCH 3/8] Limit processing to events that will be displayed Remove need to process events that will not be displayed and remove sorting. --- .../IssueAssignedTimelineEvent.class.php | 2 +- .../IssueCreatedTimelineEvent.class.php | 2 +- .../IssueMonitorTimelineEvent.class.php | 2 +- .../IssueNoteCreatedTimelineEvent.class.php | 2 +- .../IssueStatusChangeTimelineEvent.class.php | 2 +- core/classes/IssueTagTimelineEvent.class.php | 2 +- core/classes/TimelineEvent.class.php | 31 +------------ core/timeline_api.php | 44 ++++--------------- core/timeline_inc.php | 10 ++--- 9 files changed, 20 insertions(+), 77 deletions(-) diff --git a/core/classes/IssueAssignedTimelineEvent.class.php b/core/classes/IssueAssignedTimelineEvent.class.php index 39870fdf22..a713500eeb 100644 --- a/core/classes/IssueAssignedTimelineEvent.class.php +++ b/core/classes/IssueAssignedTimelineEvent.class.php @@ -38,7 +38,7 @@ class IssueAssignedTimelineEvent extends TimelineEvent { * @param integer $p_handler_id An user identifier. */ public function __construct( $p_timestamp, $p_user_id, $p_issue_id, $p_handler_id ) { - parent::__construct( $p_timestamp, $p_user_id, $p_issue_id ); + parent::__construct( $p_timestamp, $p_user_id ); $this->issue_id = $p_issue_id; $this->handler_id = $p_handler_id; diff --git a/core/classes/IssueCreatedTimelineEvent.class.php b/core/classes/IssueCreatedTimelineEvent.class.php index 467b682bd7..d011d4842b 100644 --- a/core/classes/IssueCreatedTimelineEvent.class.php +++ b/core/classes/IssueCreatedTimelineEvent.class.php @@ -36,7 +36,7 @@ class IssueCreatedTimelineEvent extends TimelineEvent { * @param integer $p_issue_id A issue identifier. */ public function __construct( $p_timestamp, $p_user_id, $p_issue_id ) { - parent::__construct( $p_timestamp, $p_user_id, $p_issue_id ); + parent::__construct( $p_timestamp, $p_user_id ); $this->issue_id = $p_issue_id; } diff --git a/core/classes/IssueMonitorTimelineEvent.class.php b/core/classes/IssueMonitorTimelineEvent.class.php index cf2b35654e..6f941191df 100644 --- a/core/classes/IssueMonitorTimelineEvent.class.php +++ b/core/classes/IssueMonitorTimelineEvent.class.php @@ -38,7 +38,7 @@ class IssueMonitorTimelineEvent extends TimelineEvent { * @param boolean $p_monitor Whether issue was being monitored or unmonitored. */ public function __construct( $p_timestamp, $p_user_id, $p_issue_id, $p_monitor ) { - parent::__construct( $p_timestamp, $p_user_id, $p_issue_id ); + parent::__construct( $p_timestamp, $p_user_id ); $this->issue_id = $p_issue_id; $this->monitor = $p_monitor; diff --git a/core/classes/IssueNoteCreatedTimelineEvent.class.php b/core/classes/IssueNoteCreatedTimelineEvent.class.php index 490426a71c..cb14f762f1 100644 --- a/core/classes/IssueNoteCreatedTimelineEvent.class.php +++ b/core/classes/IssueNoteCreatedTimelineEvent.class.php @@ -38,7 +38,7 @@ class IssueNoteCreatedTimelineEvent extends TimelineEvent { * @param integer $p_issue_note_id A issue note identifier. */ public function __construct( $p_timestamp, $p_user_id, $p_issue_id, $p_issue_note_id ) { - parent::__construct( $p_timestamp, $p_user_id, $p_issue_id ); + parent::__construct( $p_timestamp, $p_user_id ); $this->issue_id = $p_issue_id; $this->issue_note_id = $p_issue_note_id; diff --git a/core/classes/IssueStatusChangeTimelineEvent.class.php b/core/classes/IssueStatusChangeTimelineEvent.class.php index 67dec64a97..b384df15cc 100644 --- a/core/classes/IssueStatusChangeTimelineEvent.class.php +++ b/core/classes/IssueStatusChangeTimelineEvent.class.php @@ -50,7 +50,7 @@ class IssueStatusChangeTimelineEvent extends TimelineEvent { * @param integer $p_new_status New status value of issue. */ public function __construct( $p_timestamp, $p_user_id, $p_issue_id, $p_old_status, $p_new_status ) { - parent::__construct( $p_timestamp, $p_user_id, $p_issue_id ); + parent::__construct( $p_timestamp, $p_user_id ); $this->issue_id = $p_issue_id; $this->old_status = $p_old_status; diff --git a/core/classes/IssueTagTimelineEvent.class.php b/core/classes/IssueTagTimelineEvent.class.php index 28d4debd7f..7f9bbe5c30 100644 --- a/core/classes/IssueTagTimelineEvent.class.php +++ b/core/classes/IssueTagTimelineEvent.class.php @@ -40,7 +40,7 @@ class IssueTagTimelineEvent extends TimelineEvent { * @param boolean $p_tag Whether tag was being linked or unlinked from the issue. */ public function __construct( $p_timestamp, $p_user_id, $p_issue_id, $p_tag_name, $p_tag ) { - parent::__construct( $p_timestamp, $p_user_id, $p_issue_id ); + parent::__construct( $p_timestamp, $p_user_id ); $this->issue_id = $p_issue_id; $this->tag_name = $p_tag_name; diff --git a/core/classes/TimelineEvent.class.php b/core/classes/TimelineEvent.class.php index 3c84b73374..8eb250b86d 100644 --- a/core/classes/TimelineEvent.class.php +++ b/core/classes/TimelineEvent.class.php @@ -30,43 +30,14 @@ class TimelineEvent { protected $timestamp; protected $user_id; - protected $tie_breaker; /** * @param integer $p_timestamp Timestamp representing the time the event occurred. * @param integer $p_user_id An user identifier. - * @param boolean $p_tie_breaker A value to sort events by if timestamp matches (generally issue identifier). */ - public function __construct( $p_timestamp, $p_user_id, $p_tie_breaker ) { + public function __construct( $p_timestamp, $p_user_id ) { $this->timestamp = $p_timestamp; $this->user_id = $p_user_id; - $this->tie_breaker = $p_tie_breaker; - } - - /** - * Comparision function for ordering of timeline events. - * We compare first by timestamp, then by the tie_breaker field. - * @param TimelineEvent $p_other An instance of TimelineEvent to compare against. - * @return integer - */ - public function compare( TimelineEvent $p_other ) { - if( $this->timestamp < $p_other->timestamp ) { - return -1; - } - - if( $this->timestamp > $p_other->timestamp ) { - return 1; - } - - if( $this->tie_breaker < $p_other->tie_breaker ) { - return -1; - } - - if( $this->tie_breaker > $p_other->tie_breaker ) { - return 1; - } - - return 0; } /** diff --git a/core/timeline_api.php b/core/timeline_api.php index dd383fcf00..dc75399410 100644 --- a/core/timeline_api.php +++ b/core/timeline_api.php @@ -36,13 +36,15 @@ * 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. + * @param integer $p_max_events The maximum number of events to return or 0 for unlimited. * @return array */ -function timeline_events( $p_start_time, $p_end_time ) { +function timeline_events( $p_start_time, $p_end_time, $p_max_events ) { $t_timeline_events = array(); $t_history_events_array = history_get_raw_events_array( null, null, $p_start_time, $p_end_time ); $t_history_events_array = array_reverse( $t_history_events_array ); + $t_count = 0; foreach ( $t_history_events_array as $t_history_event ) { $t_event = null; @@ -89,57 +91,29 @@ function timeline_events( $p_start_time, $p_end_time ) { # Do not include skipped events if( $t_event != null && !$t_event->skip() ) { $t_timeline_events[] = $t_event; - } - } + $t_count++; - return $t_timeline_events; -} - -/** - * Sort an array of timeline events - * @param array $p_events Array of events being sorted. - * @return array Sorted array of events. - */ -function timeline_sort_events( array $p_events ) { - $t_count = count( $p_events ); - $t_stable = false; - - while( !$t_stable ) { - $t_stable = true; - - for( $i = 0; $i < $t_count - 1; ++$i ) { - if( $p_events[$i]->compare( $p_events[$i+1] ) < 0 ) { - $t_temp = $p_events[$i]; - $p_events[$i] = $p_events[$i+1]; - $p_events[$i+1] = $t_temp; - $t_stable = false; + if ( $p_max_events > 0 && $t_count >= $p_max_events ) { + break; + } } } - } - return $p_events; + return $t_timeline_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 int Number of displayed events */ -function timeline_print_events( array $p_events, $p_max_num = 0 ) { +function timeline_print_events( array $p_events ) { if( empty( $p_events ) ) { echo '

' . lang_get( 'timeline_no_activity' ) . '

'; return 0; } - $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(); } - return min( $p_max_num, $i); } diff --git a/core/timeline_inc.php b/core/timeline_inc.php index eb69294f14..1cb4be02b9 100644 --- a/core/timeline_inc.php +++ b/core/timeline_inc.php @@ -17,14 +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_max_events = $f_all ? 0 : 50; $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 ); +$t_events = timeline_events( $t_start_time, $t_end_time, $t_max_events ); echo '
'; @@ -44,14 +43,13 @@ } 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 ); -$t_num_events = timeline_print_events( $t_events, ( $f_all ? 0 : MAX_EVENTS ) ); +timeline_print_events( $t_events ); # 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 )) { +if( !$f_all && count( $t_events ) == $t_max_events ) { echo '

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

'; } From b6a86a070bc1962aa9b011499ee50b29e5c33b12 Mon Sep 17 00:00:00 2001 From: Victor Boctor Date: Mon, 6 Jul 2015 12:44:02 +1000 Subject: [PATCH 4/8] Check access level for issues within time window --- core/history_api.php | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/core/history_api.php b/core/history_api.php index 8062c98cae..583b1518bb 100644 --- a/core/history_api.php +++ b/core/history_api.php @@ -234,12 +234,24 @@ function history_get_raw_events_array( $p_bug_id, $p_user_id = null, $p_start_ti $t_view_attachments_threshold = config_get( 'view_attachments_threshold' ); $t_show_monitor_list_threshold = config_get( 'show_monitor_list_threshold' ); $t_show_handler_threshold = config_get( 'view_handler_threshold' ); + $t_bug_visible = array(); $t_standard_fields = columns_get_standard(); $j = 0; while( $t_row = db_fetch_array( $t_result ) ) { extract( $t_row, EXTR_PREFIX_ALL, 'v' ); + # if no specific bug id specified, check access level for the bug associated with current row. + if ( $p_bug_id === null ) { + if ( !isset( $t_bug_visible[$v_bug_id] ) ) { + $t_bug_visible[$v_bug_id] = access_has_bug_level( VIEWER, $v_bug_id ); + } + + if ( !$t_bug_visible[$v_bug_id] ) { + continue; + } + } + if( $v_type == NORMAL_TYPE ) { if( !in_array( $v_field_name, $t_standard_fields ) ) { # check that the item should be visible to the user From 25ec97fef236a550568cc8425d11a32b9584622b Mon Sep 17 00:00:00 2001 From: Victor Boctor Date: Mon, 6 Jul 2015 13:07:49 +1000 Subject: [PATCH 5/8] Add timeline issue access check + reduce queries - Add access level that issues are visible now that we work directly on history rows. - Only retrieve the N required rows for the default view, some of the N may be filtered later, causing the view to have less than N entries. --- core/history_api.php | 13 +++++++++++-- core/timeline_api.php | 3 +-- core/timeline_inc.php | 6 ++---- 3 files changed, 14 insertions(+), 8 deletions(-) diff --git a/core/history_api.php b/core/history_api.php index 583b1518bb..888a91b6eb 100644 --- a/core/history_api.php +++ b/core/history_api.php @@ -184,10 +184,15 @@ function history_count_user_recent_events( $p_duration_in_seconds, $p_user_id = * @param integer $p_user_id A valid user identifier. * @param integer $p_start_time The start time to filter by, or null for all. * @param integer $p_end_time The end time to filter by, or null for all. + * @param integer $p_max_events The maximum number of events to return or null/0 for all. * @return array */ -function history_get_raw_events_array( $p_bug_id, $p_user_id = null, $p_start_time = null, $p_end_time = null ) { - $t_history_order = config_get( 'history_order' ); +function history_get_raw_events_array( $p_bug_id, $p_user_id = null, $p_start_time = null, $p_end_time = null, $p_max_events = null, $p_sort_order = null ) { + if ( $p_sort_order === null ) { + $t_history_order = config_get( 'history_order' ); + } else { + $t_history_order = $p_sort_order; + } $t_user_id = (( null === $p_user_id ) ? auth_get_current_user_id() : $p_user_id ); @@ -339,6 +344,10 @@ function history_get_raw_events_array( $p_bug_id, $p_user_id = null, $p_start_ti $t_raw_history[$j]['new_value'] = $v_new_value; $j++; + + if ( $p_max_events !== null && $p_max_events !== 0 && $j >= $p_max_events ) { + break; + } } # end for loop diff --git a/core/timeline_api.php b/core/timeline_api.php index dc75399410..75d9fd9556 100644 --- a/core/timeline_api.php +++ b/core/timeline_api.php @@ -42,8 +42,7 @@ function timeline_events( $p_start_time, $p_end_time, $p_max_events ) { $t_timeline_events = array(); - $t_history_events_array = history_get_raw_events_array( null, null, $p_start_time, $p_end_time ); - $t_history_events_array = array_reverse( $t_history_events_array ); + $t_history_events_array = history_get_raw_events_array( null, null, $p_start_time, $p_end_time, $p_max_events, 'DESC' ); $t_count = 0; foreach ( $t_history_events_array as $t_history_event ) { diff --git a/core/timeline_inc.php b/core/timeline_inc.php index 1cb4be02b9..c5d597f796 100644 --- a/core/timeline_inc.php +++ b/core/timeline_inc.php @@ -46,10 +46,8 @@ timeline_print_events( $t_events ); -# 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 && count( $t_events ) == $t_max_events ) { +# We will compromise accuracy of more link and showing exactly N without clicking more in favor or reducing load. +if( !$f_all ) { echo '

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

'; } From 0d5ea1378863561875e151c1c4593feff4e87af7 Mon Sep 17 00:00:00 2001 From: Victor Boctor Date: Mon, 6 Jul 2015 18:31:43 +1000 Subject: [PATCH 6/8] Misc line breaks and phpdoc updates --- core/history_api.php | 5 ++++- core/timeline_inc.php | 3 ++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/core/history_api.php b/core/history_api.php index 888a91b6eb..09c7472c27 100644 --- a/core/history_api.php +++ b/core/history_api.php @@ -185,6 +185,8 @@ function history_count_user_recent_events( $p_duration_in_seconds, $p_user_id = * @param integer $p_start_time The start time to filter by, or null for all. * @param integer $p_end_time The end time to filter by, or null for all. * @param integer $p_max_events The maximum number of events to return or null/0 for all. + * @param integer $p_sort_order The sort order ASC, DESC, or null to use default ordering + * as per config. * @return array */ function history_get_raw_events_array( $p_bug_id, $p_user_id = null, $p_start_time = null, $p_end_time = null, $p_max_events = null, $p_sort_order = null ) { @@ -327,7 +329,8 @@ function history_get_raw_events_array( $p_bug_id, $p_user_id = null, $p_start_ti } } - # if history is not scoped to a single bug, then include the bug id in the returned array for each element. + # if history is not scoped to a single bug, then include the bug id in the returned array + # for each element. if ( $p_bug_id === null ) { $t_raw_history[$j]['bug_id'] = $v_bug_id; } diff --git a/core/timeline_inc.php b/core/timeline_inc.php index c5d597f796..664dd6af36 100644 --- a/core/timeline_inc.php +++ b/core/timeline_inc.php @@ -46,7 +46,8 @@ timeline_print_events( $t_events ); -# We will compromise accuracy of more link and showing exactly N without clicking more in favor or reducing load. +# We will compromise accuracy of more link and showing exactly N without clicking more in favor or +# reducing load. if( !$f_all ) { echo '

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

'; } From 96120fda60010990a8cce9fb9ba0bdc6d72430d4 Mon Sep 17 00:00:00 2001 From: Victor Boctor Date: Mon, 6 Jul 2015 23:22:24 +1000 Subject: [PATCH 7/8] Refactor for better api + optimize for accuracy Refactor the API for re-usability of code between normal issue history api and APIs used to populate the timeline functionality. The new API also enables only doing the minimum necessary work to render the default/full timeline view while maintaining accuracy in terms of the number of events in the default view. --- core/history_api.php | 141 +++++++++++++++++++++++------------------- core/timeline_api.php | 8 +-- core/timeline_inc.php | 14 +++-- 3 files changed, 91 insertions(+), 72 deletions(-) diff --git a/core/history_api.php b/core/history_api.php index 09c7472c27..8eebef4e2f 100644 --- a/core/history_api.php +++ b/core/history_api.php @@ -176,38 +176,20 @@ function history_count_user_recent_events( $p_duration_in_seconds, $p_user_id = } /** - * Retrieves the raw history events for the specified bug id and returns it in an array - * The array is indexed from 0 to N-1. The second dimension is: 'date', 'userid', 'username', - * 'field','type','old_value','new_value' - * @param integer $p_bug_id A valid bug identifier or null to not filter by bug. If no bug id is specified, - * then returned array will have a field for bug_id, otherwise it won't. - * @param integer $p_user_id A valid user identifier. - * @param integer $p_start_time The start time to filter by, or null for all. - * @param integer $p_end_time The end time to filter by, or null for all. - * @param integer $p_max_events The maximum number of events to return or null/0 for all. - * @param integer $p_sort_order The sort order ASC, DESC, or null to use default ordering - * as per config. - * @return array + * Creates and executes a query for the history rows matching the specified criteria. + * @param integer $p_bug_id The bug id or null for matching any bug. + * @param integer $p_start_time The start time to filter by, or null for all. + * @param integer $p_end_time The end time to filter by, or null for all. + * @param string $p_history_order The sort order. + * @return database result to pass into history_get_event_from_row(). */ -function history_get_raw_events_array( $p_bug_id, $p_user_id = null, $p_start_time = null, $p_end_time = null, $p_max_events = null, $p_sort_order = null ) { - if ( $p_sort_order === null ) { +function history_get_range_result( $p_bug_id = null, $p_start_time = null, $p_end_time = null, $p_history_order = null ) { + if ( $p_history_order === null ) { $t_history_order = config_get( 'history_order' ); } else { - $t_history_order = $p_sort_order; + $t_history_order = $p_history_order; } - $t_user_id = (( null === $p_user_id ) ? auth_get_current_user_id() : $p_user_id ); - - $t_roadmap_view_access_level = config_get( 'roadmap_view_threshold' ); - $t_due_date_view_threshold = config_get( 'due_date_view_threshold' ); - - # grab history and display by date_modified then field_name - # @@@ by MASC I guess it's better by id then by field_name. When we have more history lines with the same - # date, it's better to respect the storing order otherwise we should risk to mix different information - # I give you an example. We create a child of a bug with different custom fields. In the history of the child - # bug we will find the line related to the relationship mixed with the custom fields (the history is creted - # for the new bug with the same timestamp...) - $t_query = 'SELECT * FROM {bug_history}'; $t_params = array(); $t_where = array(); @@ -234,22 +216,26 @@ function history_get_raw_events_array( $p_bug_id, $p_user_id = null, $p_start_ti $t_query .= ' ORDER BY date_modified ' . $t_history_order . ',id'; $t_result = db_query( $t_query, $t_params ); - $t_raw_history = array(); - $t_private_bugnote_threshold = config_get( 'private_bugnote_threshold' ); - $t_tag_view_threshold = config_get( 'tag_view_threshold' ); - $t_view_attachments_threshold = config_get( 'view_attachments_threshold' ); - $t_show_monitor_list_threshold = config_get( 'show_monitor_list_threshold' ); - $t_show_handler_threshold = config_get( 'view_handler_threshold' ); - $t_bug_visible = array(); + return $t_result; +} - $t_standard_fields = columns_get_standard(); - $j = 0; - while( $t_row = db_fetch_array( $t_result ) ) { +/** + * Gets the next accessible history event for current user and specified db result. + * @param string $p_result The database result. + * @param integer $p_user_id The user id or null for logged in user. + * @param boolean $p_check_access_to_issue true: check that user has access to bugs, + * false otherwise. + * @return array containing the history event or false if no more matches. + */ +function history_get_event_from_row( $p_result, $p_user_id = null, $p_check_access_to_issue = true ) { + $t_user_id = ( null === $p_user_id ) ? auth_get_current_user_id() : $p_user_id; + + while ( $t_row = db_fetch_array( $p_result ) ) { extract( $t_row, EXTR_PREFIX_ALL, 'v' ); # if no specific bug id specified, check access level for the bug associated with current row. - if ( $p_bug_id === null ) { + if ( $p_check_access_to_issue === null ) { if ( !isset( $t_bug_visible[$v_bug_id] ) ) { $t_bug_visible[$v_bug_id] = access_has_bug_level( VIEWER, $v_bug_id ); } @@ -260,7 +246,7 @@ function history_get_raw_events_array( $p_bug_id, $p_user_id = null, $p_start_ti } if( $v_type == NORMAL_TYPE ) { - if( !in_array( $v_field_name, $t_standard_fields ) ) { + if( !in_array( $v_field_name, columns_get_standard() ) ) { # check that the item should be visible to the user $t_field_id = custom_field_get_id_from_name( $v_field_name ); if( false !== $t_field_id && !custom_field_has_read_access( $t_field_id, $v_bug_id, $t_user_id ) ) { @@ -268,15 +254,15 @@ function history_get_raw_events_array( $p_bug_id, $p_user_id = null, $p_start_ti } } - if( ( $v_field_name == 'target_version' ) && !access_has_bug_level( $t_roadmap_view_access_level, $v_bug_id, $t_user_id ) ) { + if( ( $v_field_name == 'target_version' ) && !access_has_bug_level( config_get( 'roadmap_view_threshold' ), $v_bug_id, $t_user_id ) ) { continue; } - if( ( $v_field_name == 'due_date' ) && !access_has_bug_level( $t_due_date_view_threshold, $v_bug_id, $t_user_id ) ) { + if( ( $v_field_name == 'due_date' ) && !access_has_bug_level( config_get( 'due_date_view_threshold' ), $v_bug_id, $t_user_id ) ) { continue; } - if( ( $v_field_name == 'handler_id' ) && !access_has_bug_level( $t_show_handler_threshold, $v_bug_id, $t_user_id ) ) { + if( ( $v_field_name == 'handler_id' ) && !access_has_bug_level( config_get( 'view_handler_threshold' ), $v_bug_id, $t_user_id ) ) { continue; } } @@ -285,13 +271,13 @@ function history_get_raw_events_array( $p_bug_id, $p_user_id = null, $p_start_ti if( $t_user_id != $v_user_id ) { # bypass if user originated note if( ( $v_type == BUGNOTE_ADDED ) || ( $v_type == BUGNOTE_UPDATED ) || ( $v_type == BUGNOTE_DELETED ) ) { - if( !access_has_bug_level( $t_private_bugnote_threshold, $v_bug_id, $t_user_id ) && ( bugnote_get_field( $v_old_value, 'view_state' ) == VS_PRIVATE ) ) { + if( !access_has_bug_level( config_get( 'private_bugnote_threshold' ), $v_bug_id, $t_user_id ) && ( bugnote_get_field( $v_old_value, 'view_state' ) == VS_PRIVATE ) ) { continue; } } if( $v_type == BUGNOTE_STATE_CHANGED ) { - if( !access_has_bug_level( $t_private_bugnote_threshold, $v_bug_id, $t_user_id ) && ( bugnote_get_field( $v_new_value, 'view_state' ) == VS_PRIVATE ) ) { + if( !access_has_bug_level( config_get( 'private_bugnote_threshold' ), $v_bug_id, $t_user_id ) && ( bugnote_get_field( $v_new_value, 'view_state' ) == VS_PRIVATE ) ) { continue; } } @@ -299,21 +285,21 @@ function history_get_raw_events_array( $p_bug_id, $p_user_id = null, $p_start_ti # tags if( $v_type == TAG_ATTACHED || $v_type == TAG_DETACHED || $v_type == TAG_RENAMED ) { - if( !access_has_bug_level( $t_tag_view_threshold, $v_bug_id, $t_user_id ) ) { + if( !access_has_bug_level( config_get( 'tag_view_threshold' ), $v_bug_id, $t_user_id ) ) { continue; } } # attachments if( $v_type == FILE_ADDED || $v_type == FILE_DELETED ) { - if( !access_has_bug_level( $t_view_attachments_threshold, $v_bug_id, $t_user_id ) ) { + if( !access_has_bug_level( config_get( 'view_attachments_threshold' ), $v_bug_id, $t_user_id ) ) { continue; } } # monitoring if( $v_type == BUG_MONITOR || $v_type == BUG_UNMONITOR ) { - if( !access_has_bug_level( $t_show_monitor_list_threshold, $v_bug_id, $t_user_id ) ) { + if( !access_has_bug_level( config_get( 'show_monitor_list_threshold' ), $v_bug_id, $t_user_id ) ) { continue; } } @@ -329,28 +315,59 @@ function history_get_raw_events_array( $p_bug_id, $p_user_id = null, $p_start_ti } } - # if history is not scoped to a single bug, then include the bug id in the returned array - # for each element. - if ( $p_bug_id === null ) { - $t_raw_history[$j]['bug_id'] = $v_bug_id; - } - - $t_raw_history[$j]['date'] = $v_date_modified; - $t_raw_history[$j]['userid'] = $v_user_id; + $t_event = array(); + $t_event['bug_id'] = $v_bug_id; + $t_event['date'] = $v_date_modified; + $t_event['userid'] = $v_user_id; # user_get_name handles deleted users, and username vs realname - $t_raw_history[$j]['username'] = user_get_name( $v_user_id ); + $t_event['username'] = user_get_name( $v_user_id ); - $t_raw_history[$j]['field'] = $v_field_name; - $t_raw_history[$j]['type'] = $v_type; - $t_raw_history[$j]['old_value'] = $v_old_value; - $t_raw_history[$j]['new_value'] = $v_new_value; + $t_event['field'] = $v_field_name; + $t_event['type'] = $v_type; + $t_event['old_value'] = $v_old_value; + $t_event['new_value'] = $v_new_value; - $j++; + return $t_event; + } - if ( $p_max_events !== null && $p_max_events !== 0 && $j >= $p_max_events ) { + return false; +} + +/** + * Retrieves the raw history events for the specified bug id and returns it in an array + * The array is indexed from 0 to N-1. The second dimension is: 'date', 'userid', 'username', + * 'field','type','old_value','new_value' + * @param integer $p_bug_id A valid bug identifier or null to not filter by bug. If no bug id is specified, + * then returned array will have a field for bug_id, otherwise it won't. + * @param integer $p_user_id A valid user identifier. + * @param integer $p_start_time The start time to filter by, or null for all. + * @param integer $p_end_time The end time to filter by, or null for all. + * @return array + */ +function history_get_raw_events_array( $p_bug_id, $p_user_id = null, $p_start_time = null, $p_end_time = null ) { + $t_user_id = (( null === $p_user_id ) ? auth_get_current_user_id() : $p_user_id ); + + # grab history and display by date_modified then field_name + # @@@ by MASC I guess it's better by id then by field_name. When we have more history lines with the same + # date, it's better to respect the storing order otherwise we should risk to mix different information + # I give you an example. We create a child of a bug with different custom fields. In the history of the child + # bug we will find the line related to the relationship mixed with the custom fields (the history is creted + # for the new bug with the same timestamp...) + + $t_result = history_get_range_result( $p_bug_id, $p_start_time, $p_end_time ); + + $t_raw_history = array(); + + $j = 0; + while( true ) { + $t_event = history_get_event_from_row( $t_result, $t_user_id, /* check access */ true ); + if ( $t_event === false ) { break; } + + $t_raw_history[$j] = $t_event; + $j++; } # end for loop diff --git a/core/timeline_api.php b/core/timeline_api.php index 75d9fd9556..51dbe49562 100644 --- a/core/timeline_api.php +++ b/core/timeline_api.php @@ -42,10 +42,10 @@ function timeline_events( $p_start_time, $p_end_time, $p_max_events ) { $t_timeline_events = array(); - $t_history_events_array = history_get_raw_events_array( null, null, $p_start_time, $p_end_time, $p_max_events, 'DESC' ); - $t_count = 0; + $t_result = history_get_range_result( /* $p_bug_id */ null, $p_start_time, $p_end_time, 'DESC' ); + $t_count = 0; - foreach ( $t_history_events_array as $t_history_event ) { + while ( $t_history_event = history_get_event_from_row( $t_result, /* $p_user_id */ auth_get_current_user_id(), /* $p_check_access_to_issue */ true ) ) { $t_event = null; $t_user_id = $t_history_event['userid']; $t_timestamp = $t_history_event['date']; @@ -96,7 +96,7 @@ function timeline_events( $p_start_time, $p_end_time, $p_max_events ) { break; } } - } + } return $t_timeline_events; } diff --git a/core/timeline_inc.php b/core/timeline_inc.php index 664dd6af36..60df714886 100644 --- a/core/timeline_inc.php +++ b/core/timeline_inc.php @@ -17,9 +17,11 @@ 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_max_events = $f_all ? 0 : 50; +$t_max_events = $f_all ? 0 : MAX_EVENTS + 1; $t_end_time = time() - ( $f_days * SECONDS_PER_DAY ); $t_start_time = $t_end_time - ( 7 * SECONDS_PER_DAY ); @@ -44,12 +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 . '
'; -timeline_print_events( $t_events ); - -# We will compromise accuracy of more link and showing exactly N without clicking more in favor or -# reducing load. -if( !$f_all ) { +if( !$f_all && count( $t_events ) > MAX_EVENTS ) { + $t_events = array_slice( $t_events, 0, MAX_EVENTS ); + timeline_print_events( $t_events ); echo '

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

'; +} else { + timeline_print_events( $t_events ); } echo '
'; From e5023cec79dd2c21a799873e501370015bd24cef Mon Sep 17 00:00:00 2001 From: Victor Boctor Date: Mon, 6 Jul 2015 23:23:28 +1000 Subject: [PATCH 8/8] Indentation fix --- core/timeline_api.php | 92 +++++++++++++++++++++---------------------- 1 file changed, 46 insertions(+), 46 deletions(-) diff --git a/core/timeline_api.php b/core/timeline_api.php index 51dbe49562..f79b486960 100644 --- a/core/timeline_api.php +++ b/core/timeline_api.php @@ -46,56 +46,56 @@ function timeline_events( $p_start_time, $p_end_time, $p_max_events ) { $t_count = 0; while ( $t_history_event = history_get_event_from_row( $t_result, /* $p_user_id */ auth_get_current_user_id(), /* $p_check_access_to_issue */ true ) ) { - $t_event = null; - $t_user_id = $t_history_event['userid']; - $t_timestamp = $t_history_event['date']; - $t_issue_id = $t_history_event['bug_id']; + $t_event = null; + $t_user_id = $t_history_event['userid']; + $t_timestamp = $t_history_event['date']; + $t_issue_id = $t_history_event['bug_id']; - switch( $t_history_event['type'] ) { - case NEW_BUG: - $t_event = new IssueCreatedTimelineEvent( $t_timestamp, $t_user_id, $t_issue_id ); - break; - case BUGNOTE_ADDED: - $t_bugnote_id = $t_history_event['old_value']; - $t_event = new IssueNoteCreatedTimelineEvent( $t_timestamp, $t_user_id, $t_issue_id, $t_bugnote_id ); - break; - case BUG_MONITOR: - # Skip monitors added for others due to reminders, only add monitor events where added - # user is the same as the logged in user. - if( (int)$t_history_event['old_value'] == (int)$t_history_event['userid'] ) { - $t_event = new IssueMonitorTimelineEvent( $t_timestamp, $t_user_id, $t_issue_id, true ); - } - break; - case BUG_UNMONITOR: - $t_event = new IssueMonitorTimelineEvent( $t_timestamp, $t_user_id, $t_issue_id, false ); - break; - case TAG_ATTACHED: - $t_event = new IssueTagTimelineEvent( $t_timestamp, $t_user_id, $t_issue_id, $t_history_event['old_value'], true ); - break; - case TAG_DETACHED: - $t_event = new IssueTagTimelineEvent( $t_timestamp, $t_user_id, $t_issue_id, $t_history_event['old_value'], false ); - break; - case NORMAL_TYPE: - switch( $t_history_event['field'] ) { - case 'status': - $t_event = new IssueStatusChangeTimelineEvent( $t_timestamp, $t_user_id, $t_issue_id, $t_history_event['old_value'], $t_history_event['new_value'] ); - break; - case 'handler_id': - $t_event = new IssueAssignedTimelineEvent( $t_timestamp, $t_user_id, $t_issue_id, $t_history_event['new_value'] ); - break; - } - break; - } + switch( $t_history_event['type'] ) { + case NEW_BUG: + $t_event = new IssueCreatedTimelineEvent( $t_timestamp, $t_user_id, $t_issue_id ); + break; + case BUGNOTE_ADDED: + $t_bugnote_id = $t_history_event['old_value']; + $t_event = new IssueNoteCreatedTimelineEvent( $t_timestamp, $t_user_id, $t_issue_id, $t_bugnote_id ); + break; + case BUG_MONITOR: + # Skip monitors added for others due to reminders, only add monitor events where added + # user is the same as the logged in user. + if( (int)$t_history_event['old_value'] == (int)$t_history_event['userid'] ) { + $t_event = new IssueMonitorTimelineEvent( $t_timestamp, $t_user_id, $t_issue_id, true ); + } + break; + case BUG_UNMONITOR: + $t_event = new IssueMonitorTimelineEvent( $t_timestamp, $t_user_id, $t_issue_id, false ); + break; + case TAG_ATTACHED: + $t_event = new IssueTagTimelineEvent( $t_timestamp, $t_user_id, $t_issue_id, $t_history_event['old_value'], true ); + break; + case TAG_DETACHED: + $t_event = new IssueTagTimelineEvent( $t_timestamp, $t_user_id, $t_issue_id, $t_history_event['old_value'], false ); + break; + case NORMAL_TYPE: + switch( $t_history_event['field'] ) { + case 'status': + $t_event = new IssueStatusChangeTimelineEvent( $t_timestamp, $t_user_id, $t_issue_id, $t_history_event['old_value'], $t_history_event['new_value'] ); + break; + case 'handler_id': + $t_event = new IssueAssignedTimelineEvent( $t_timestamp, $t_user_id, $t_issue_id, $t_history_event['new_value'] ); + break; + } + break; + } - # Do not include skipped events - if( $t_event != null && !$t_event->skip() ) { - $t_timeline_events[] = $t_event; - $t_count++; + # Do not include skipped events + if( $t_event != null && !$t_event->skip() ) { + $t_timeline_events[] = $t_event; + $t_count++; - if ( $p_max_events > 0 && $t_count >= $p_max_events ) { - break; - } + if ( $p_max_events > 0 && $t_count >= $p_max_events ) { + break; } + } } return $t_timeline_events;