[PATCH v3 1/2] Allow listing all comments from a user
Lukas Fleischer
lfleischer at archlinux.org
Sun Aug 5 16:31:46 UTC 2018
On Sun, 22 Jul 2018 at 17:54:35, Johannes Löthberg wrote:
> Signed-off-by: Johannes Löthberg <johannes at kyriasis.com>
> ---
> Since v2:
> Refactor things a bit to use the same pkg_comments.php for both
> per-package and per-user comment listings
>
> web/html/account.php | 20 +++++-
> web/html/css/aurweb.css | 42 +++++++++++++
> web/html/index.php | 2 +
> web/html/pkgbase.php | 10 ++-
> web/lib/acctfuncs.inc.php | 42 +++++++++++++
> web/lib/aur.inc.php | 53 ++++++++++++++++
> web/lib/credentials.inc.php | 2 +
> web/lib/pkgbasefuncs.inc.php | 10 ++-
> web/lib/pkgfuncs.inc.php | 4 ++
> web/template/account_details.php | 3 +
> web/template/account_edit_form.php | 1 +
> web/template/pkg_comments.php | 99 ++++++++++++++++++++++--------
> 12 files changed, 258 insertions(+), 30 deletions(-)
>
Sorry for reviewing this so late. The patch is pretty invasive, though,
and we should make sure we are not missing anything.
> @@ -166,6 +166,24 @@ if (isset($_COOKIE["AURSID"])) {
> $row["Username"]);
> }
>
> + } elseif ($action == "ListComments") {
> + if (has_credential(CRED_ACCOUNT_LIST_COMMENTS)) {
> + # display the comment list if they're a TU/dev
> +
> + $total_comment_count = account_comments_count($row["ID"]);
> + [$pagination_templs, $per_page, $offset] = calculate_pagination($total_comment_count);
Interesting. I don't think we are using this syntax anywhere so far and
it might be a good idea to use list() instead, even if it is just for
consistency. Thoughts?
> +
> + $username = $row["Username"];
> + $uid = $row["ID"];
> + $comments = account_comments($row["ID"], $per_page, $offset);
We can replace the first parameter with $uid here.
> diff --git a/web/html/css/aurweb.css b/web/html/css/aurweb.css
> index f5e1037..593c9ae 100644
> --- a/web/html/css/aurweb.css
> +++ b/web/html/css/aurweb.css
> @@ -148,3 +148,45 @@ label.confirmation,
> color: red;
> font-weight: bold;
> }
> +
> +.package-comments {
> + margin-top: 1.5em;
> +}
> +
> +.comments-header {
> + display: flex;
> + justify-content: space-between;
> + align-items: flex-start;
> +}
> +
> +/* arrowed headings */
> +.comments-header h3 span.text {
> + display: block;
> + background: #1794D1;
> + font-size: 15px;
> + padding: 2px 10px;
> + color: white;
> +}
> +
> +.comments-header .comments-header-nav {
> + align-self: flex-end;
> +}
> +
> +.comment-header {
> + clear: both;
> + font-size: 1em;
> + margin-top: 1.5em;
> + border-bottom: 1px dotted #bbb;
> +}
> +
> +.comments div {
> + margin-bottom: 1em;
> +}
> +
> +.comments div p {
> + margin-bottom: 0.5em;
> +}
> +
> +.comments .more {
> + font-weight: normal;
> +}
What is the rationale for adding those new styles? Can't we just display
comments the way they are currently displayed under the package details?
> diff --git a/web/lib/acctfuncs.inc.php b/web/lib/acctfuncs.inc.php
> index df57375..192d879 100644
> --- a/web/lib/acctfuncs.inc.php
> +++ b/web/lib/acctfuncs.inc.php
> @@ -1403,3 +1403,45 @@ function accept_terms($uid, $termrev) {
> $dbh->exec($q);
> }
> }
> +
> +function account_comments($uid, $limit, $offset=0) {
> + $dbh = DB::connect();
> + $q = "SELECT PackageComments.ID, Comments, UsersID, ";
> + $q.= "PackageBaseId, CommentTS, DelTS, EditedTS, B.UserName AS EditUserName, ";
> + $q.= "PinnedTS, ";
> + $q.= "C.UserName as DelUserName, RenderedComment, ";
> + $q.= "PB.ID as PackageBaseID, PB.Name as PackageBaseName ";
> + $q.= "FROM PackageComments ";
> + $q.= "LEFT JOIN PackageBases PB ON PackageComments.PackageBaseID = PB.ID ";
> + $q.= "LEFT JOIN Users A ON PackageComments.UsersID = A.ID ";
> + $q.= "LEFT JOIN Users B ON PackageComments.EditedUsersID = B.ID ";
> + $q.= "LEFT JOIN Users C ON PackageComments.DelUsersID = C.ID ";
> + $q.= "WHERE A.ID = " . $dbh->quote($uid) . " ";
> + $q.= "ORDER BY CommentTS DESC";
> +
> + if ($limit > 0) {
> + $q.=" LIMIT " . $limit;
Please add intval() around $limit here. Even if we know that the current
callers always pass integer values, it does not hurt to enforce the
conversion here to prevent from potential security issues in the future.
> + }
> +
> + if ($offset > 0) {
> + $q.=" OFFSET " . $offset;
Same here. Add intval($offset).
> + }
> +
> + $result = $dbh->query($q);
> + if (!$result) {
> + return null;
> + }
> +
> + return $result->fetchAll();
> +}
> +
> +function account_comments_count($uid) {
> + $dbh = DB::connect();
> + $q = "SELECT COUNT(*) ";
> + $q.= "FROM PackageComments ";
> + $q.= "LEFT JOIN Users A ON PackageComments.UsersID = A.ID ";
> + $q.= "WHERE A.ID = " . $dbh->quote($uid);
> +
> + $result = $dbh->query($q);
> + return $result->fetch(PDO::FETCH_NUM)[0];
Is this equivalent to "return $result->fetchColumn();"?
> +}
> diff --git a/web/lib/aur.inc.php b/web/lib/aur.inc.php
> index feb4006..89da81a 100644
> --- a/web/lib/aur.inc.php
> +++ b/web/lib/aur.inc.php
> @@ -705,3 +705,56 @@ function aur_location() {
> }
> return $location;
> }
> +
> +/**
> + * Calculate pagination templates
> + *
> + * @return array The array of pagination templates, per page, and offset values
> + */
> +function calculate_pagination($total_comment_count) {
> [...]
Nice! I wonder whether we can use this helper function elsewhere, such
as for paginating package search results?
> diff --git a/web/lib/pkgbasefuncs.inc.php b/web/lib/pkgbasefuncs.inc.php
> index 72c33b6..953a581 100644
> --- a/web/lib/pkgbasefuncs.inc.php
> +++ b/web/lib/pkgbasefuncs.inc.php
> [...]
> @@ -71,6 +71,9 @@ function pkgbase_comments($base_id, $limit, $include_deleted, $only_pinned=false
> if ($limit > 0) {
> $q.=" LIMIT " . $limit;
> }
> + if ($offset > 0) {
> + $q.=" OFFSET " . $offset;
Same as above, please add intval().
Everything else looks good so far. And there's FS#59512 [1] (which you
are probably already aware of) to be addressed in a re-roll. Thanks a
lot put working on this!
Regards,
Lukas
[1] https://bugs.archlinux.org/task/59512
More information about the aur-dev
mailing list