| Message ID | 20180206025456.22071-1-mark.weiman@markzz.com |
|---|---|
| State | Accepted, archived |
| Headers | show |
On Tue, 06 Feb 2018 at 03:54:56, Mark Weiman wrote: > Implements FS#53832 > > Signed-off-by: Mark Weiman <mark.weiman@markzz.com> > --- > web/html/pkgbase.php | 3 +++ > web/html/pkgdisown.php | 13 ++++++++++--- > web/lib/pkgbasefuncs.inc.php | 12 ++++++++++-- > web/template/pkgbase_actions.php | 2 +- > 4 files changed, 24 insertions(+), 6 deletions(-) > > [...] > @@ -23,7 +26,11 @@ if (has_credential(CRED_PKGBASE_DISOWN, $maintainer_uids)): ?> > <?php endforeach; ?> > </ul> > <p> > - <?php if (count($comaintainers) > 0 && !has_credential(CRED_PKGBASE_DISOWN)): ?> > + > + <?php if (in_array(uid_from_sid($_COOKIE["AURSID"]), $comaintainer_uids) && !has_credential(CRED_PKGBASE_DISOWN)): > + $action = "do_DisownComaintainer"; ?> > + <?= __("By selecting the checkbox, you confirm that you want to no longer be a package co-maintainer.") ?> > + <?php elseif (count($comaintainers) > 0 && !has_credential(CRED_PKGBASE_DISOWN)): ?> I am not sure whether it is a good idea to use the same button for disowning a package as a maintainer or as a co-maintainer? What happens if a user is both a maintainer and a co-maintainer (and what is the expected behavior)? Anyway, merged this into pu as-is for now; we can still replace it later. Regards, Lukas
On 02/23/2018 12:46 AM, Lukas Fleischer wrote: > I am not sure whether it is a good idea to use the same button for > disowning a package as a maintainer or as a co-maintainer? What happens > if a user is both a maintainer and a co-maintainer (and what is the > expected behavior)? If it *is* possible to be both, maybe we should fix that instead? :p Anyway, if you click the disown button we assume you want to ditch the package altogether... if you are the maintainer and want to edit the comaintainer list we have a UI for that already.
On Fri, 2018-02-23 at 08:21 -0500, Eli Schwartz wrote: > On 02/23/2018 12:46 AM, Lukas Fleischer wrote: > > I am not sure whether it is a good idea to use the same button for > > disowning a package as a maintainer or as a co-maintainer? What happens > > if a user is both a maintainer and a co-maintainer (and what is the > > expected behavior)? > > If it *is* possible to be both, maybe we should fix that instead? :p > It is possible and I will submit a patch to fix that this week. > Anyway, if you click the disown button we assume you want to ditch the > package altogether... if you are the maintainer and want to edit the > comaintainer list we have a UI for that already. >
On Mon, 05 Mar 2018 at 07:03:01, Mark Weiman wrote: > On Fri, 2018-02-23 at 08:21 -0500, Eli Schwartz wrote: > > On 02/23/2018 12:46 AM, Lukas Fleischer wrote: > > > I am not sure whether it is a good idea to use the same button for > > > disowning a package as a maintainer or as a co-maintainer? What happens > > > if a user is both a maintainer and a co-maintainer (and what is the > > > expected behavior)? > > > > If it *is* possible to be both, maybe we should fix that instead? :p > > > > It is possible and I will submit a patch to fix that this week. > > > Anyway, if you click the disown button we assume you want to ditch the > > package altogether... if you are the maintainer and want to edit the > > comaintainer list we have a UI for that already. > > I am not too sure. The current implementation allows the maintainer to nominate a new maintainer and make himself a co-maintainer (by putting himself at the end of the list of co-maintainers and disowning the package). I guess this functionality will be lost after the "fix"? Regards, Lukas
On Mon, Mar 05, 2018 at 07:39:04AM +0100, Lukas Fleischer wrote: > On Mon, 05 Mar 2018 at 07:03:01, Mark Weiman wrote: > > On Fri, 2018-02-23 at 08:21 -0500, Eli Schwartz wrote: > > > On 02/23/2018 12:46 AM, Lukas Fleischer wrote: > > > > I am not sure whether it is a good idea to use the same button for > > > > disowning a package as a maintainer or as a co-maintainer? What happens > > > > if a user is both a maintainer and a co-maintainer (and what is the > > > > expected behavior)? > > > > > > If it *is* possible to be both, maybe we should fix that instead? :p > > > > > > > It is possible and I will submit a patch to fix that this week. > > > > > Anyway, if you click the disown button we assume you want to ditch the > > > package altogether... if you are the maintainer and want to edit the > > > comaintainer list we have a UI for that already. > > > > > I am not too sure. The current implementation allows the maintainer to > nominate a new maintainer and make himself a co-maintainer (by putting > himself at the end of the list of co-maintainers and disowning the > package). My thought is that the disown button should not change the co-maintainers if the disowning user is the maintainer. A user that is the maintainer and co-maintainer of a package should have to disown twice to be removed as a maintainer and co-maintainer. I don't think this edge-case merits a second button. Regards, Mikael
On Mon, 05 Mar 2018 at 16:50:01, Mikael Blomstrand wrote: > [...] > My thought is that the disown button should not change the > co-maintainers if the disowning user is the maintainer. > A user that is the maintainer and co-maintainer of a package should have > to disown twice to be removed as a maintainer and co-maintainer. That sounds like a great idea. Do we need any adjustments to your patch to get this feature, Mark?
diff --git a/web/html/pkgbase.php b/web/html/pkgbase.php index 03b0eee..cf9a6c6 100644 --- a/web/html/pkgbase.php +++ b/web/html/pkgbase.php @@ -60,6 +60,9 @@ if (check_token()) { $output = __("The selected packages have not been disowned, check the confirmation checkbox."); $ret = false; } + } elseif (current_action("do_DisownComaintainer")) { + $uid = uid_from_sid($_COOKIE["AURSID"]); + list($ret, $output) = pkgbase_remove_comaintainer($base_id, $uid); } elseif (current_action("do_Vote")) { list($ret, $output) = pkgbase_vote($ids, true); } elseif (current_action("do_UnVote")) { diff --git a/web/html/pkgdisown.php b/web/html/pkgdisown.php index 4b04e85..8da08cf 100644 --- a/web/html/pkgdisown.php +++ b/web/html/pkgdisown.php @@ -7,10 +7,13 @@ include_once("pkgfuncs.inc.php"); html_header(__("Disown Package")); +$action = "do_Disown"; + $maintainer_uids = array(pkgbase_maintainer_uid($base_id)); $comaintainers = pkgbase_get_comaintainers($base_id); +$comaintainer_uids = pkgbase_get_comaintainer_uids(array($base_id)); -if (has_credential(CRED_PKGBASE_DISOWN, $maintainer_uids)): ?> +if (has_credential(CRED_PKGBASE_DISOWN, array_merge($maintainer_uids, $comaintainer_uids))): ?> <div class="box"> <h2><?= __('Disown Package') ?>: <?= htmlspecialchars($pkgbase_name) ?></h2> <p> @@ -23,7 +26,11 @@ if (has_credential(CRED_PKGBASE_DISOWN, $maintainer_uids)): ?> <?php endforeach; ?> </ul> <p> - <?php if (count($comaintainers) > 0 && !has_credential(CRED_PKGBASE_DISOWN)): ?> + + <?php if (in_array(uid_from_sid($_COOKIE["AURSID"]), $comaintainer_uids) && !has_credential(CRED_PKGBASE_DISOWN)): + $action = "do_DisownComaintainer"; ?> + <?= __("By selecting the checkbox, you confirm that you want to no longer be a package co-maintainer.") ?> + <?php elseif (count($comaintainers) > 0 && !has_credential(CRED_PKGBASE_DISOWN)): ?> <?= __('By selecting the checkbox, you confirm that you want to disown the package and transfer ownership to %s%s%s.', '<strong>', $comaintainers[0], '</strong>'); ?> <?php else: ?> @@ -40,7 +47,7 @@ if (has_credential(CRED_PKGBASE_DISOWN, $maintainer_uids)): ?> <?php endif; ?> <p><label class="confirmation"><input type="checkbox" name="confirm" value="1" /> <?= __("Confirm to disown the package") ?></label</p> - <p><input type="submit" class="button" name="do_Disown" value="<?= __("Disown") ?>" /></p> + <p><input type="submit" class="button" name="<?= $action ?>" value="<?= __("Disown") ?>" /></p> </fieldset> </form> </div> diff --git a/web/lib/pkgbasefuncs.inc.php b/web/lib/pkgbasefuncs.inc.php index ff1bc90..16f95da 100644 --- a/web/lib/pkgbasefuncs.inc.php +++ b/web/lib/pkgbasefuncs.inc.php @@ -1158,11 +1158,12 @@ function pkgbase_get_comaintainer_uids($base_ids) { * * @param int $base_id The package base ID to update the co-maintainers of * @param array $users Array of co-maintainer user names + * @param boolean $override Override credential check if true * * @return array Tuple of success/failure indicator and error message */ -function pkgbase_set_comaintainers($base_id, $users) { - if (!has_credential(CRED_PKGBASE_EDIT_COMAINTAINERS, array(pkgbase_maintainer_uid($base_id)))) { +function pkgbase_set_comaintainers($base_id, $users, $override=false) { + if (!$override && !has_credential(CRED_PKGBASE_EDIT_COMAINTAINERS, array(pkgbase_maintainer_uid($base_id)))) { return array(false, __("You are not allowed to manage co-maintainers of this package base.")); } @@ -1213,3 +1214,10 @@ function pkgbase_set_comaintainers($base_id, $users) { return array(true, __("The package base co-maintainers have been updated.")); } + +function pkgbase_remove_comaintainer($base_id, $uid) { + $uname = username_from_id($uid); + $names = pkgbase_get_comaintainers($base_id); + $names = array_diff($names, array($uname)); + return pkgbase_set_comaintainers($base_id, $names, true); +} diff --git a/web/template/pkgbase_actions.php b/web/template/pkgbase_actions.php index d3f0592..5eee547 100644 --- a/web/template/pkgbase_actions.php +++ b/web/template/pkgbase_actions.php @@ -41,7 +41,7 @@ <?php if ($uid && $row["MaintainerUID"] === NULL): ?> <li><?= html_action_form($base_uri . 'adopt/', "do_Adopt", __('Adopt Package')) ?></li> - <?php elseif (has_credential(CRED_PKGBASE_DISOWN, array($row["MaintainerUID"]))): ?> + <?php elseif (has_credential(CRED_PKGBASE_DISOWN, array_merge(array($row["MaintainerUID"]), pkgbase_get_comaintainer_uids(array($base_id))))): ?> <li><?= html_action_form($base_uri . 'disown/', "do_Disown", __('Disown Package')) ?></li> <?php endif; ?> </ul>
Implements FS#53832 Signed-off-by: Mark Weiman <mark.weiman@markzz.com> --- web/html/pkgbase.php | 3 +++ web/html/pkgdisown.php | 13 ++++++++++--- web/lib/pkgbasefuncs.inc.php | 12 ++++++++++-- web/template/pkgbase_actions.php | 2 +- 4 files changed, 24 insertions(+), 6 deletions(-)