[aur-dev] [PATCH v3] Implement capability to pin comments above others
Lukas Fleischer
lfleischer at archlinux.org
Fri Dec 11 19:42:58 UTC 2015
On Wed, 09 Dec 2015 at 16:08:39, Mark Weiman wrote:
> Adds capability to pin comments before others.
>
> Implements FS#10863
>
> Signed-off-by: Mark Weiman <mark.weiman at markzz.com>
> ---
> schema/aur-schema.sql | 1 +
> upgrading/4.2.0.txt | 6 +++
> web/html/css/aurweb.css | 6 +--
> web/html/images/pin.min.svg | 1 +
> web/html/images/pin.svg | 3 ++
> web/html/images/unpin.min.svg | 1 +
> web/html/images/unpin.svg | 4 ++
> web/html/index.php | 2 +
> web/html/pkgbase.php | 4 ++
> web/lib/credentials.inc.php | 2 +
> web/lib/pkgbasefuncs.inc.php | 93 ++++++++++++++++++++++++++++++++++++++++---
> web/lib/pkgfuncs.inc.php | 51 +++++++++++++++++++++++-
> web/template/pkg_comments.php | 54 ++++++++++++++++++++-----
> 13 files changed, 207 insertions(+), 21 deletions(-)
This version already looks pretty good! There are still some whitespace
errors. You should see error messages like
.git/rebase-apply/patch:212: trailing whitespace.
when you try to apply the patch yourself, e.g. by running
$ git format-patch HEAD^
$ git reset --hard HEAD^
$ git am 0001-Implement-capability-to-pin-comments-above-others.patch
More comments below.
> create mode 100755 web/html/images/pin.min.svg
> create mode 100755 web/html/images/pin.svg
> create mode 100755 web/html/images/unpin.min.svg
> create mode 100755 web/html/images/unpin.svg
>
Why are these files executable?
> diff --git a/schema/aur-schema.sql b/schema/aur-schema.sql
> index 315a75c..5561278 100644
> --- a/schema/aur-schema.sql
> +++ b/schema/aur-schema.sql
> @@ -261,6 +261,7 @@ CREATE TABLE PackageComments (
> EditedTS BIGINT UNSIGNED NULL DEFAULT NULL,
> EditedUsersID INTEGER UNSIGNED NULL DEFAULT NULL,
> DelUsersID INTEGER UNSIGNED NULL DEFAULT NULL,
> + PinnedTS BIGINT UNSIGNED NOT NULL DEFAULT 0,
> PRIMARY KEY (ID),
> INDEX (UsersID),
> INDEX (PackageBaseID),
> diff --git a/upgrading/4.2.0.txt b/upgrading/4.2.0.txt
> index 1f92ec5..2b3f983 100644
> --- a/upgrading/4.2.0.txt
> +++ b/upgrading/4.2.0.txt
> @@ -15,3 +15,9 @@ CREATE UNIQUE INDEX ProviderNameProvides ON OfficialProviders (Name, Provides);
> ----
> ALTER TABLE Users MODIFY Email VARCHAR(254) NOT NULL;
> ----
> +
> +3. Add new column in PackageComments for pinning system.
> +
> +----
> +ALTER TABLE PackageComments ADD PinnedTS bigint(20) DEFAULT 0;
Why is this different from the schema line above?
ALTER TABLE PackageComments ADD COLUMN PinnedTS BIGINT UNSIGNED NOT NULL DEFAULT 0;
is what I would have expected here.
> [...]
> /**
> + * 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."));
> + }
> +
> + if (isset($_POST["comment_id"])) {
> + $comment_id = $_POST["comment_id"];
> + } else {
> + return array(false, __("Missing comment ID."));
> + }
> +
> + if (pkgbase_comments_count($_POST['package_base'], false, true) >= 5){
> + return array(false, __("No more than 5 comments can be pinned."));
> + }
> +
> + if (!can_pin_comment($comment_id)) {
> + return array(false, __("You are not allowed to pin this comment."));
> + }
> +
> + $dbh = DB::connect();
> + $q = "UPDATE PackageComments ";
> + $q.= "SET PinnedTS = UNIX_TIMESTAMP() ";
> + $q.= "WHERE ID = ".intval($comment_id);
> + $dbh->exec($q);
> + return array(true, __("Comment has been pinned."));
> +}
> +
> +/**
> + * Unpin a package comment
> + *
> + * @return array Tuple of success/failure indicator and error message
> + */
> +function pkgbase_unpin_comment() {
> + $uid = uid_from_sid($_COOKIE["AURSID"]);
> + if (!$uid) {
> + return array(false, __("You must be logged in before you can edit package information."));
> + }
> +
> + if (isset($_POST["comment_id"])) {
> + $comment_id = $_POST["comment_id"];
> + } else {
> + return array(false, __("Missing comment ID."));
> + }
> +
> + if (!can_pin_comment($comment_id)) {
> + return array(false, __("You are not allowed to unpin this comment."));
> + }
> +
> + $dbh = DB::connect();
> + $q = "UPDATE PackageComments ";
> + $q.= "SET PinnedTS = 0 ";
> + $q.= "WHERE ID = ".intval($comment_id);
> + $dbh->exec($q);
> + return array(true, __("Comment has been unpinned."));
> +}
> +
> +/**
> +
Seems like pkgbase_pin_comment() and pkgbase_unpin_comment() are almost
identical. Can we share most of the code?
Also, we make sure that one cannot pin more than five comments but I
would have expected the "Pin" icons to be hidden when the limit is
reached. The current UI behavior is quite confusing.
> [...]
> @@ -581,9 +622,17 @@ function pkg_display_details($id=0, $row, $SID="") {
> if ($SID) {
> include('pkg_comment_box.php');
> }
> +
> + $include_deleted = has_credential(CRED_COMMENT_VIEW_DELETED);
> +
> + $limit_pinned = isset($_GET['pinned']) ? 0 : 5;
> + $pinned = pkgbase_comments($base_id, $limit_pinned, false, true);
> + if (!empty($pinned)) {
> + include('pkg_comments.php');
> + unset($pinned);
> + }
This looks a bit weird. Wrong indentation?
>
> $limit = isset($_GET['comments']) ? 0 : 10;
> - $include_deleted = has_credential(CRED_COMMENT_VIEW_DELETED);
> $comments = pkgbase_comments($base_id, $limit, $include_deleted);
> if (!empty($comments)) {
> include('pkg_comments.php');
> diff --git a/web/template/pkg_comments.php b/web/template/pkg_comments.php
> index 21ce16f..dd71b93 100644
> --- a/web/template/pkg_comments.php
> +++ b/web/template/pkg_comments.php
> @@ -1,18 +1,27 @@
> <?php
> -if (isset($row['BaseID'])) {
> - /* On a package details page. */
> - $base_id = $row['BaseID'];
> -} else {
> - /* On a package base details page. */
> - $base_id = $row['ID'];
> +if (!isset($base_id)) {
> + if (isset($row['BaseID'])) {
> + /* On a package details page. */
> + $base_id = $row['BaseID'];
> + } else {
> + /* On a package base details page. */
> + $base_id = $row['ID'];
> + }
> +}
> [...]
Why is this needed? Could you elaborate please?
Thank you for all your hard work on implementing this!
Regards,
Lukas
More information about the aur-dev
mailing list