[PATCH 1 of 2 RFC] summary: add "last status change author" tooltip to status icon

[Mads Kiilerich]

On 02/18/2016 05:36 PM, Angel Ezquerra wrote:
> # HG changeset patch
> # User ezquerra

ui.username ? ;-)

> # Date 1455812934 -3600
> #      Thu Feb 18 17:28:54 2016 +0100
> # Branch stable
> # Node ID 8087e93914290771452ad99d6d6a7f57c7fa02c3
> # Parent  511aa91947fe31ae4b123a483fac7f2fb1408af2
> summary: add "last status change author" tooltip to status icon
>
> This adds a tooltip to the status change icon, indicating who did the last
> status change. This lets you quickly check who last changed the status of each
> changeset.
>
> Note that this is RFC. A final solution should be improved in several ways:
> - Apply this same change to the changelog template

Indeed. We have a lot of stupid code duplication between the summary 
page and the changelog view. I think the summary page should be very 
different from what it is now and not show the short changelog. 
Something like "most recent active branches" would be much more useful. 
Until then, I wouldn't care much about the summary page. Code sharing 
between for example changelog and PR and compare is much more important.

> - Move the code that this change embeds in the HTML somewhere else
> - Maybe the calculations could be done once and stored in the database?

As mentioned below, we almost already get the data you are looking for 
in db.py .

> Since this is the first time that I change this sort of code (I did not even
> know what mako was an hour ago!) I send this as an RFC in the hopes of getting
> pointers to where this could be changed more cleanly. I also worry whether this
> could have some negative performance impact?
>
> diff --git a/kallithea/templates/changelog/changelog_summary_data.html b/kallithea/templates/changelog/changelog_summary_data.html
> --- a/kallithea/templates/changelog/changelog_summary_data.html
> +++ b/kallithea/templates/changelog/changelog_summary_data.html
> @@ -1,5 +1,18 @@
>   ## -*- coding: utf-8 -*-
>   %if c.repo_changesets:
> +<%
> +def find_last_status_change_comment():
> +    """find the last comment which changed the review state"""
> +    last_cm = None
> +    for cm in c.comments[cs.raw_id]:
> +        if not cm.status_change:
> +            continue
> +        last_cm = cm
> +    return last_cm
> +
> +def get_comment_username(cm):
> +    return cm.author.username

The function name is longer than the body and it says pretty much the 
same. Just inline it?

> +%>
>   <table>
>       <tr>
>           <th class="left"></th>
> @@ -21,7 +34,14 @@
>                       <i class="icon-circle changeset-status-${c.statuses.get(cs.raw_id)[0]}"></i>
>                     </a>
>                   %else:
> -                  <i class="icon-circle changeset-status-${c.statuses.get(cs.raw_id)[0]}"></i>
> +                    <%last_status_change_cm = find_last_status_change_comment()%>
> +                    %if last_status_change_cm is not None:
> +                        <a class="tooltip" title="${get_comment_username(last_status_change_cm)}">
> +                    %endif
> +                    <i class="icon-circle changeset-status-${c.statuses.get(cs.raw_id)[0]}"></i>
> +                    %if last_status_change_cm is not None:
> +                        </a>
> +                    %endif

The approval color for the changeset comes from c.statuses. I think we 
should get the name of whoever gave it that color from the same place. 
That will avoid the extra loop that might be O(n*n) and slow. I think it 
will be easy to make the statuses function in db.py return the user_id too.

>                   %endif
>                   </span>
>                 %endif
>

/Mads