[PATCH v3 1/2] Allow listing all comments from a user
Johannes Löthberg
johannes at kyriasis.com
Sun Aug 5 23:58:23 UTC 2018
Quoting Lukas Fleischer (2018-08-05 18:31:46)
> > + [$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?
>
While I prefer the short syntax since it's both less noisy and more
consistent with the short array creation syntax, it doesn't matter much,
so I'll change it.
> > +
> > + $username = $row["Username"];
> > + $uid = $row["ID"];
> > + $comments = account_comments($row["ID"], $per_page, $offset);
>
> We can replace the first parameter with $uid here.
Fixed.
>
> > 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?
>
I had to add more divs to be able to style the pagination links in a
reasonable manner, which means that the archweb.css styles no longer
apply.
> > 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).
>
Both fixed.
> > + }
> > +
> > + $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();"?
>
Yeah, fixed.
> > +}
> > 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?
>
Yep, that's the plan.
> > 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().
>
Fixed.
> 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!
>
Yeah, I hadn't even thought about letting regular users list their own
comments, but there's no real reason not to, so fixed.
--
Sincerely,
Johannes Löthberg
PGP Key ID: 0x50FB9B273A9D0BB5
PGP Key FP: 5134 EF9E AF65 F95B 6BB1 608E 50FB 9B27 3A9D 0BB5
https://theos.kyriasis.com/~kyrias/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 1727 bytes
Desc: signature
URL: <https://lists.archlinux.org/pipermail/aur-dev/attachments/20180806/ea4d3574/attachment.asc>
More information about the aur-dev
mailing list