Re: [aur-dev] [PATCH v2] Implement capability to pin comments above others
Adds functions and credential information to pin comments before others.
This needs two extra columns (PinnedTS and PinnedUsersID) to the PackageComments table.
Implements FS#10863
Signed-off-by: Mark Weiman <mark.weiman@markzz.com>
For starters, you've used spaces instead of tabs everywhere for indentation, so you're going to have to fix that, and won't comment on every instance of it, and it seems you forgot to add the details of the new SQL schema to schema/ and upgrading/ <snip>
* @param bool $include_deleted True if deleted comments should be included + * @param bool $show_only_pinned True when only pinned comments are to be included * * @return array All package comment information for a specific package base */ -function pkgbase_comments($base_id, $limit, $include_deleted) { +function pkgbase_comments($base_id, $limit, $include_deleted, $show_only_pinned=false) {
$show_only_pinned is misleading since this function doesn't show anything at all. Something like $only_pinned would be a more accurate name. <snip>
/** + * Pin a package comment + * + * @return array Tuple of success/failure indicator and error message + */ +function pkgbase_pin_comment() { + $uid = uid_from_sid($_COOKIE["AURSID"]); + if (!$uid) { + return array(false, __("You must be logged in before you can edit package information.")); + } +
<snip> Both here and below you should check that the user has the right credentials, otherwise anyone that's logged in could send a POST request to pin any comment. Should probably set a max number of pinnable comments as well. <snip>
index 21ce16f..8f1fb9f 100644 --- a/web/template/pkg_comments.php +++ b/web/template/pkg_comments.php @@ -6,13 +6,20 @@ if (isset($row['BaseID'])) { /* On a package base details page. */ $base_id = $row['ID']; } -$include_deleted = has_credential(CRED_COMMENT_VIEW_DELETED); -$count = pkgbase_comments_count($base_id, $include_deleted); +if (!isset($count)) { + $count = pkgbase_comments_count($base_id, $include_deleted); +} ?> <div id="news"> <h3> - <a href="<?= htmlentities(get_pkgbase_uri($pkgbase_name), ENT_QUOTES) . '?' . mkurl('comments=all') ?>" title="<?= __('View all comments' , $count) ?> (<?= $count ?>)"><?= __('Latest Comments') ?></a> - <span class="arrow"></span> + <?php if (!isset($comments)): ?> + <?php $comments = $pinned ?> + <a href="<?= htmlentities(get_pkgbase_uri($pkgbase_name), ENT_QUOTES) . '?' . mkurl('comments=all') ?>" title="<?= __('View all comments' , $count) ?> (<?= $count ?>)"><?= __('Pinned Comments') ?></a> + <span class="arrow"></span> + <?php else: ?> + <a href="<?= htmlentities(get_pkgbase_uri($pkgbase_name), ENT_QUOTES) . '?' . mkurl('comments=all') ?>" title="<?= __('View all comments' , $count) ?> (<?= $count ?>)"><?= __('Latest Comments') ?></a> + <span class="arrow"></span> + <?php endif; ?> </h3>
The indentation is really all over the place here. <snip>
+ alt="<?= __('Unin comment') ?>" title="<?= __('Pin comment') ?>" name="submit" value="1" />
"Unin comment" and "Pin comment" instead of "Unpin comment" -- Sincerely, Johannes Löthberg PGP Key ID: 0x50FB9B273A9D0BB5 https://theos.kyriasis.com/~kyrias/
On Sat, 2015-12-05 at 21:01 +0100, Johannes Löthberg wrote:
For starters, you've used spaces instead of tabs everywhere for indentation, so you're going to have to fix that, and won't comment on every instance of it, and it seems you forgot to add the details of the new SQL schema to schema/ and upgrading/
Just for clarification, I just modify the aur-schema.sql file to include the new columns and for the upgrading file, do I put the query for the change in the 4.2.0.txt or in it's own file?
<snip>
$show_only_pinned is misleading since this function doesn't show anything at all. Something like $only_pinned would be a more accurate name.
I agree, it will be changed in v3.
<snip> <snip>
Both here and below you should check that the user has the right credentials, otherwise anyone that's logged in could send a POST request to pin any comment.
I'm pretty sure I did the check (if (can_pin_comment($comment_id))). It 's pretty much based on the pkgbase_delete_comment() function, so if I did indeed miss that here, that function should be checked as well.
Should probably set a max number of pinnable comments as well.
Seems reasonable, I'll make that change.
<snip>
The indentation is really all over the place here.
Will be fixed in v3.
<snip>
"Unin comment" and "Pin comment" instead of "Unpin comment"
WHOOPS! I noticed that and I thought I changed it, but I guess it slipped through. Mark Weiman
On Sun, 06 Dec 2015 at 04:51:05, Mark Weiman wrote:
[...] Just for clarification, I just modify the aur-schema.sql file to include the new columns and for the upgrading file, do I put the query for the change in the 4.2.0.txt or in it's own file?
Yes, add the new columns to schema/aur-schema.sql and add a new section to upgrading/4.2.0.txt with SQL queries to upgrade an existing database.
[...] I'm pretty sure I did the check (if (can_pin_comment($comment_id))). It 's pretty much based on the pkgbase_delete_comment() function, so if I did indeed miss that here, that function should be checked as well.
Yeah, looks good to me. One minor additional nit, though. When checking this kind of preconditions, we usually write if (!can_pin_comment($comment_id)) { return array(false, __("You are not allowed to pin this comment.")); } and then put the main code below, without any indentation. This makes the code a bit easier to read/follow.
[...]
"Unin comment" and "Pin comment" instead of "Unpin comment"
WHOOPS! I noticed that and I thought I changed it, but I guess it slipped through. [...]
One more thing: It would be nice if there was a pin/unpin icon similar to the images we use for editing or deleting a comment. Thanks!
On Mon, 2015-12-07 at 09:32 +0100, Lukas Fleischer wrote:
Yes, add the new columns to schema/aur-schema.sql and add a new section to upgrading/4.2.0.txt with SQL queries to upgrade an existing database.
Yeah, looks good to me. One minor additional nit, though. When checking this kind of preconditions, we usually write
if (!can_pin_comment($comment_id)) { return array(false, __("You are not allowed to pin this comment.")); }
and then put the main code below, without any indentation. This makes the code a bit easier to read/follow.
I will do that then.
One more thing: It would be nice if there was a pin/unpin icon similar to the images we use for editing or deleting a comment.
I will try my hardest to create one, but I'm not making any promises on how good it will look.
Thanks!
Mark Weiman
On Mon, 07 Dec 2015 at 16:20:45, Mark Weiman wrote:
[...]
One more thing: It would be nice if there was a pin/unpin icon similar to the images we use for editing or deleting a comment.
I will try my hardest to create one, but I'm not making any promises on how good it will look. [...]
The icons we are currently using are from the Iconic icon collection. Maybe we can use [1] as the "Pin" icon (and a crossed out version for "Unpin")? Speaking of that, do we need to include the Iconic MIT license somewhere in our source tree? [1] https://github.com/iconic/open-iconic/blob/master/svg/pin.svg
On Mon, 2015-12-07 at 17:39 +0100, Lukas Fleischer wrote:
On Mon, 07 Dec 2015 at 16:20:45, Mark Weiman wrote:
[...]
One more thing: It would be nice if there was a pin/unpin icon similar to the images we use for editing or deleting a comment.
I will try my hardest to create one, but I'm not making any promises on how good it will look. [...]
The icons we are currently using are from the Iconic icon collection. Maybe we can use [1] as the "Pin" icon (and a crossed out version for "Unpin")?
Speaking of that, do we need to include the Iconic MIT license somewhere in our source tree?
If you think I should include that in my patch, just tell me where to put it. If not, I'll just include the two icons. Just before I submit v3, are these icons appropriate or what changes should be made? [1] I also just had a thought, do you think I should make it so co- maintainers can also pin and unpin comments or should it just be the maintainer? Mark Weiman [1] https://markzz.com/images/pin-unpin.html
On Tue, 08 Dec 2015 at 01:25:22, Mark Weiman wrote:
[...] If you think I should include that in my patch, just tell me where to put it. If not, I'll just include the two icons.
Not as part of this patch set. I will add the license in a separate commit.
Just before I submit v3, are these icons appropriate or what changes should be made? [1]
Look good to me.
I also just had a thought, do you think I should make it so co- maintainers can also pin and unpin comments or should it just be the maintainer?
I think it's fine if only the maintainer can pin comments for now.
Mark Weiman
participants (3)
-
Johannes Löthberg
-
Lukas Fleischer
-
Mark Weiman