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@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