On Sun, 22 Jul 2018 at 17:54:35, Johannes Löthberg wrote:
Signed-off-by: Johannes Löthberg <johannes@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