Navigation Menu

Skip to content

Commit

Permalink
Refactor for better api + optimize for accuracy
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@vboctor
vboctor committed Jul 6, 2015
1 parent 0d5ea13 commit 96120fd
Show file tree
Hide file tree
Showing 3 changed files with 91 additions and 72 deletions.
141 changes: 79 additions & 62 deletions core/history_api.php
View file
Expand Up @@ -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();
Expand All @@ -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 );
}
Expand All @@ -260,23 +246,23 @@ 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 ) ) {
continue;
}
}

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;
}
}
Expand All @@ -285,35 +271,35 @@ 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;
}
}
}

# 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;
}
}
Expand All @@ -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
Expand Down
8 changes: 4 additions & 4 deletions core/timeline_api.php
View file
Expand Up @@ -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'];
Expand Down Expand Up @@ -96,7 +96,7 @@ function timeline_events( $p_start_time, $p_end_time, $p_max_events ) {
break;
}
}
}
}

return $t_timeline_events;
}
Expand Down
14 changes: 8 additions & 6 deletions core/timeline_inc.php
View file
Expand Up @@ -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 );
Expand All @@ -44,12 +46,12 @@

echo '<div class="date-range">' . date( $t_short_date_format, $t_start_time ) . ' .. ' . date( $t_short_date_format, $t_end_time ) . $t_prev_link . $t_next_link . '</div>';

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 '<p>' . $t_prev_link = ' [ <a href="my_view_page.php?days=' . $f_days . '&amp;all=1">' . lang_get( 'timeline_more' ) . '</a> ]</p>';
} else {
timeline_print_events( $t_events );
}

echo '</div>';

0 comments on commit 96120fd

Please sign in to comment.