[PATCH] Make delete and merge links create an auto-accepted request
This lets us have a better paper-trail over what happens in the AUR. Signed-off-by: Johannes Löthberg <johannes@kyriasis.com> --- web/html/pkgbase.php | 23 +++++++++++++---------- web/lib/pkgreqfuncs.inc.php | 14 +++++++------- 2 files changed, 20 insertions(+), 17 deletions(-) diff --git a/web/html/pkgbase.php b/web/html/pkgbase.php index 46ad77e..8efa51e 100644 --- a/web/html/pkgbase.php +++ b/web/html/pkgbase.php @@ -70,12 +70,10 @@ if (check_token()) { list($ret, $output) = pkgbase_vote($ids, false); } elseif (current_action("do_Delete")) { if (isset($_POST['confirm'])) { - if (!isset($_POST['merge_Into']) || empty($_POST['merge_Into'])) { - list($ret, $output) = pkgbase_delete($ids, NULL, $via); - unset($_GET['ID']); - unset($base_id); - } - else { + $type = "deletion"; + $merge_base_id = NULL; + if (isset($_POST['merge_Into']) && !empty($_POST['merge_Into'])) { + $type = "merge"; $merge_base_id = pkgbase_from_name($_POST['merge_Into']); if (!$merge_base_id) { $output = __("Cannot find package to merge votes and comments into."); @@ -83,12 +81,17 @@ if (check_token()) { } elseif (in_array($merge_base_id, $ids)) { $output = __("Cannot merge a package base with itself."); $ret = false; - } else { + } + } + if ($type == "deletion" || ($type == "merge" && $merge_base_id)) { + list($ret, $output, $request_id) = pkgreq_file($ids, $type, $_POST['merge_Into'], + "Trusted User-requested auto-accepted ".$type."."); + if ($ret) { list($ret, $output) = pkgbase_delete($ids, $merge_base_id, $via); - unset($_GET['ID']); - unset($base_id); } } + unset($_GET['ID']); + unset($base_id); } else { $output = __("The selected packages have not been deleted, check the confirmation checkbox."); @@ -112,7 +115,7 @@ if (check_token()) { } elseif (current_action("do_SetKeywords")) { list($ret, $output) = pkgbase_set_keywords($base_id, preg_split("/[\s,;]+/", $_POST['keywords'], -1, PREG_SPLIT_NO_EMPTY)); } elseif (current_action("do_FileRequest")) { - list($ret, $output) = pkgreq_file($ids, $_POST['type'], $_POST['merge_into'], $_POST['comments']); + list($ret, $output, $_reqid) = pkgreq_file($ids, $_POST['type'], $_POST['merge_into'], $_POST['comments']); } elseif (current_action("do_CloseRequest")) { list($ret, $output) = pkgreq_close($_POST['reqid'], $_POST['reason'], $_POST['comments']); } elseif (current_action("do_EditComaintainers")) { diff --git a/web/lib/pkgreqfuncs.inc.php b/web/lib/pkgreqfuncs.inc.php index 774ebe7..a15f101 100644 --- a/web/lib/pkgreqfuncs.inc.php +++ b/web/lib/pkgreqfuncs.inc.php @@ -124,19 +124,19 @@ function pkgreq_get_creator_email($id) { */ function pkgreq_file($ids, $type, $merge_into, $comments) { if (!has_credential(CRED_PKGREQ_FILE)) { - return array(false, __("You must be logged in to file package requests.")); + return array(false, __("You must be logged in to file package requests."), NULL); } if (!empty($merge_into) && !preg_match("/^[a-z0-9][a-z0-9\.+_-]*$/D", $merge_into)) { - return array(false, __("Invalid name: only lowercase letters are allowed.")); + return array(false, __("Invalid name: only lowercase letters are allowed."), NULL); } if (!empty($merge_into) && !pkgbase_from_name($merge_into)) { - return array(false, __("Cannot find package to merge votes and comments into.")); + return array(false, __("Cannot find package to merge votes and comments into."), NULL); } if (empty($comments)) { - return array(false, __("The comment field must not be empty.")); + return array(false, __("The comment field must not be empty."), NULL); } $dbh = DB::connect(); @@ -147,7 +147,7 @@ function pkgreq_file($ids, $type, $merge_into, $comments) { $pkgbase_name = pkgbase_name_from_id($base_id); if ($merge_into == $pkgbase_name) { - return array(false, __("Cannot merge a package base with itself.")); + return array(false, __("Cannot merge a package base with itself."), NULL); } $q = "SELECT ID FROM RequestTypes WHERE Name = " . $dbh->quote($type); @@ -155,7 +155,7 @@ function pkgreq_file($ids, $type, $merge_into, $comments) { if ($row = $result->fetch(PDO::FETCH_ASSOC)) { $type_id = $row['ID']; } else { - return array(false, __("Invalid request type.")); + return array(false, __("Invalid request type."), NULL); } $q = "INSERT INTO PackageRequests "; @@ -208,7 +208,7 @@ function pkgreq_file($ids, $type, $merge_into, $comments) { pkgbase_delete(array($base_id), NULL, NULL, true); } - return array(true, __("Added request successfully.")); + return array(true, __("Added request successfully."), $request_id); } /** -- 2.20.1
On 12/23/18 4:14 PM, Johannes Löthberg wrote:
This lets us have a better paper-trail over what happens in the AUR. I'm opposed to this change.
The purpose of those links is arguably to do things in exceptional circumstances. There are cases where an auto-accepted request is simply uninteresting information. e.g., the following cases: - user submits deletion request, Eli Schwartz uses "close" instead of "accept", with the message "merged instead". Do we really need additional notifications here? - deletion of spam packages Moreover, the current proposal is simply inferior in every possible way compared to simply submitting a request, then accepting it. That way you get to leave a message saying why it happened... I would simply never ever use the delete/merge links if I actually wanted to send out notifications. On top of this, where is the notification for orphaning packages against the will of the maintainer? ... It's basically accepted practice regardless that when TUs adopt a package into community, they submit a deletion request and then accept it. This will traditionally include the high-content comment "moved to community". The current patchset was proposed in response to one TU on IRC, who feels strongly about the goal of said paper trail and desired to have the entire feature removed from aurweb altogether. I propose instead that we follow my recommendation to document on the wiki at https://wiki.archlinux.org/index.php/AUR_Trusted_User_Guidelines that TUs should submit a deletion request *with the relevant comment* rather than deleting the package. I see no reason to modify the emergency override tools. I'm unaware of the lack of a paper trail being a significant issue. Perhaps we should just trust each TU in good faith that the reason we elected them is because they're not going to misuse the tools? Note: regarding the person who suggested on IRC that the links should be removed, the orphan link in particular is utterly crucial to remain, since aurweb includes a feature to accept an orphan request early by leaving a comment and *not* actually orphaning the package. This requires the TU to manually use the orphan link. If orphan requests were given fair treatment with the other two request types, this would result in a second notification every time it was used. More generally, if you wish to leave a comment in the acceptance notification, you must use the same comment form followed by manual followup when closing a deletion or merge request as well (although those are not locked for the duration of a 14-day grace period). ... tl;dr this is a documentation problem, not a code problem. It is also not a practical problem, since we're supposedly already doing the preferred method... right? -- Eli Schwartz Bug Wrangler and Trusted User
participants (2)
-
Eli Schwartz
-
Johannes Löthberg