Code review comment for lp:~vasilev/psiphon/sprint3-579678

Revision history for this message
Robert Vasilev (vasilev) wrote :

Most of your recommendations have been implemented.

In detail:
> 1. Display the total number of users in all cases, even when no filter.
Done.

> 2. Add the paging controls to the top of the page (but also leave at the bottom).
Done.

> 3. Originally when add/edit user and return to list, the new/modified user was
> shown in bold. Now it returns to the default with no filter and page 1. Can
> you make it return to the page with the new user, shown in bold? This isn't
> required: please make a bug to backlog it if it's too much work.
Implemented for editing/disabling case.
For adding case it's quite unobvious to predict on which page new user will appear for different sorting modes.
It will be better to implement highlinghting feature for new users as anover ticket.

> - For code readability, don't mix access control check and input validation
> ...
> Should validate same value used, even if we can reason that it's safe and/or equivalent
Done.

> - Always escape user input in output, even if we don't think it's a risk:
...
> See other files for examples of escaping. We use the helper function escape_html.
Have not found any example of escaping integers with escape_html() in Psiphon code.
Escaping integers with escape_html() not implemented because of unnecessarity.

> - "SELECT COUNT(*) AS cnt FROM user WHERE {$query_where_sql}" is repeated
> twice in a row; could keep result in variable. Even though MySQL caches
> results. If you always display the user total (filter or no) then: 1. always
> get total; 2. get filtered count when filter.
Done. But could not 'keep result in variable' because of '$query_where_sql' may be modified by adding filtering conditions, and results will not be equivalent.

> - Please use a more descriptive variable name: "echo ($pn);"
Done.

« Back to merge proposal