From 96120fda60010990a8cce9fb9ba0bdc6d72430d4 Mon Sep 17 00:00:00 2001 From: Victor Boctor Date: Mon, 6 Jul 2015 23:22:24 +1000 Subject: [PATCH] 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 '';