Re: [aur-dev] [PATCH 1/2] Add capability to pin comments above others

On Sat, 28 Nov 2015 at 01:44:26, Mark Weiman wrote:

Adds functions and credential information to pin comments before others.

This needs two extra columns (PinnedTS and PinnedUsersID) to the PackageComments table.

Signed-off-by: Mark Weiman --- web/html/pkgbase.php | 4 ++ web/lib/credentials.inc.php | 2 + web/lib/pkgbasefuncs.inc.php | 123 ++++++++++++++++++++++++++++++++++++++++++- web/lib/pkgfuncs.inc.php | 54 +++++++++++++++++++ 4 files changed, 182 insertions(+), 1 deletion(-)

First of all, thanks for working on this! I planned on implementing this for a while but never got around to doing it. The main difference of this series to what I had in mind is that you allow for pinning multiple comments whereas I planned to have a single pinned comment (per package base) only. Can you think of a use case where having several pinned comments is desirable? Can't the maintainer simply edit the pinned comment and add new information? Would we ever want to pin a comment from a user other than the current maintainer? More comments on the implementation below.

[...] /** + * Get all pinned package comment information for a specific package base + * + * @param int $base_id The package base ID to get comments for + * @param int $limit Maximum number of comments to return (0 means unlimited) + * @param bool $include_deleted True if deleted comments should be included + * + * @return array Pinned package comment information for specific package base + */ +function pkgbase_pinned_comments($base_id, $limit, $include_deleted) { [...]

I did not have a very detailed look at the second patch in this series but is this really needed? Can't we simply retrieve all comments by calling pkgbase_comments() and order them such that pinned comments are displayed first?

[...] +/** + * Display pinned comments before the other comments on a package page + * + * @param string $base_id The package base ID to add the comment on + * @param string $uid The user ID of the individual who pinned the comment + * @param string $comment_id The comment id to be pinned + * + * @return void + */ +function pkgbase_disp_pin_comment($base_id, $uid, $comment_id) { + $dbh = DB::connect(); + + $q = "UPDATE PackageComments "; + $q.= "SET PinnedTS = UNIXTIMESTAMP()), PinnedUsersID=" . $uid . " "; + $q.= "WHERE ID = " . intval($comment_id);

After having read the function name and the comment, I would have expected this function to print a pinned comment but that does not seem to be what this function does... Anyway, what is the difference between this function and pkgbase_pin_comment() below? Can't we somehow merge those functions?

[...] /** + * Determine if the user can pin a specific package comment + * + * Only the Package Maintainer, Trusted Users, and Developers can pin + * comments. This function is used for the backend side of comment pinning. + * + * @param string $comment_id The comment ID in the database + * + * @return bool True if the user can pin the comment, otherwise false + */ +function can_pin_comment($comment_id=0) { + $dbh = DB::connect(); + + $q = "SELECT MaintainerUID FROM PackageBases AS pb "; + $q.= "LEFT JOIN PackageComments AS pc ON pb.ID = pc.PackageBaseID "; + $q.= "WHERE pc.ID = " . intval($comment_id); + $result = $dbh->query($q); + + if (!$result) { + return false; + } + + $uid = $result->fetch(PDO::FETCH_COLUMN, 0); + + return has_credential(CRED_COMMENT_PIN, array($uid)); +} + +/** + * Determine if the user can edit a specific package comment using an array + * + * Only the Package Maintainer, Trusted Users, and Developers can pin + * comments. This function is used for the frontend side of comment pinning. + * + * @param array $comment All database information relating a specific comment + * + * @return bool True if the user can edit the comment, otherwise false + */ +function can_pin_comment_array($comment) { + $dbh = DB::connect(); + + $q = "SELECT MaintainerUID FROM PackageBases AS pb "; + $q.= "LEFT JOIN PackageComments AS pc ON pb.ID = pc.PackageBaseID "; + $q.= "WHERE pc.ID = " . intval($comment['ID']); + $result = $dbh->query($q); + + if (!$result) { + return false; + } + + $uid = $result->fetch(PDO::FETCH_COLUMN, 0); + + return has_credential(CRED_COMMENT_PIN, array($uid)); +} [...]

Isn't this equivalent to function can_pin_comment_array($comment) { return can_pin_comment($comment['ID']); } ...? I also think that the function name is slightly misleading. I would have expected that can_pin_comment_array() accepts an array of comment IDs and returns an array of boolean values. Do we need that function at all? Can't we simply call can_pin_comment($comment['ID']) instead? Regards, Lukas