-
Notifications
You must be signed in to change notification settings - Fork 11
disband, reinstate PI groups #520
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
b4f49d4 to
8b797d7
Compare
6da966d to
a8dcf02
Compare
9c25537 to
7b65b64
Compare
5bbab62 to
0d8a076
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request restores the ability to disable and re-enable PI groups (previously called "disband"), using new terminology and implementing it via an LDAP attribute rather than deleting the group. When a PI group is disabled, it's marked with an isDisabled attribute in LDAP and is excluded from most portal operations.
Changes:
- Adds a new LDAP schema with
isDisabledattribute andunityClusterPIGroupobject class - Implements
disable(),reenable(),getIsDisabled(), andsetIsDisabled()methods in UnityGroup - Updates all LDAP queries to filter out disabled PI groups using a new
NON_DEFUNCT_FILTER - Adds UI controls in pi-mgmt.php and pi.php for disabling groups
- Updates email templates to reflect "disabled" terminology instead of "disbanded"
- Adds comprehensive test coverage for disable/reenable functionality
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 17 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/docker-dev/identity/unity-cluster-schema.ldif | New LDAP schema defining isDisabled attribute and unityClusterPIGroup object class |
| tools/docker-dev/identity/bootstrap.ldif | Adds unityClusterPIGroup object class to all PI groups and creates test disabled groups |
| tools/docker-dev/identity/Dockerfile | Loads the new LDAP schema during Docker build |
| resources/lib/UnityGroup.php | Core disable/reenable logic, getIsDisabled/setIsDisabled methods, updated approveGroup to handle re-enabling |
| resources/lib/UnityLDAP.php | Renamed methods to getAllNonDisabledPIGroups variants, added NON_DEFUNCT_FILTER |
| resources/lib/UnityUser.php | Updated isPI() to check disabled status, changed objectclass arrays |
| resources/lib/UnityOrg.php | Updated objectclass array from constant to inline |
| webroot/admin/pi-mgmt.php | Added disable action and UI button |
| webroot/panel/pi.php | Added disable action and "Danger Zone" UI section |
| webroot/panel/modal/pi_search.php | Uses getAllNonDisabledPIGroups |
| workers/group_user_request_owner_reminder.php | Uses getAllNonDisabledPIGroups |
| resources/mail/group_disabled.php | Updated template for disabled (not disbanded) |
| resources/mail/group_reenabled.php | New template for re-enabled groups |
| resources/mail/user_flag_removed.php | Changed "Deactivated" to "Disabled" |
| test/functional/PIDisableTest.php | New comprehensive tests for disable functionality |
| test/functional/PIBecomeApproveTest.php | Added testReenableGroup |
| test/functional/RegisterUserTest.php | Removed unused import |
| test/phpunit-bootstrap.php | Uses getNonDisabledPIGroupGIDsWithMemberUID |
| README.md | Added upgrade instructions for LDAP schema |
| LDAP.md | Added disabled group terminology |
Comments suppressed due to low confidence (3)
resources/mail/group_disabled.php:9
- The email message states "Any jobs associated with this PI account have been killed." However, there's no code in the disable method that actually kills jobs. This message may be misleading to users if job killing is not implemented. Either implement job killing in the disable method or update this message to reflect what actually happens.
resources/lib/UnityGroup.php:131 - In approveGroup, when re-enabling a disabled group, the email template "group_created" is sent. This is misleading since the group is being re-enabled, not created for the first time. Either use the "group_reenabled" template when re-enabling (which is what reenable() already does), or send the "group_created" email only when actually creating a new group. Currently, the user would receive both "group_reenabled" (from reenable()) and "group_created" (from approveGroup()) which is redundant.
public function approveGroup(bool $send_mail = true): void
{
$uid = $this->getOwner()->uid;
$request = $this->SQL->getRequest($uid, UnitySQL::REQUEST_BECOME_PI);
\ensure($this->getOwner()->exists());
if (!$this->entry->exists()) {
$this->init();
} elseif ($this->getIsDisabled()) {
$this->reenable();
} else {
throw new Exception("cannot approve group that already exists and is not disabled");
}
$this->SQL->removeRequest($this->getOwner()->uid, UnitySQL::REQUEST_BECOME_PI);
$this->SQL->addLog("approved_group", $this->getOwner()->uid);
if ($send_mail) {
$this->MAILER->sendMail($this->getOwner()->getMail(), "group_created");
}
// having your own group makes you qualified
$this->getOwner()->setFlag(UserFlag::QUALIFIED, true);
}
resources/mail/group_disabled.php:11
- The sentence is missing a period at the end. Should be "If you believe this to be a mistake, please reply to this email."
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
66f42fb to
6b306ac
Compare
6ed7483 to
0936727
Compare
3aab9a8 to
994be0d
Compare
f0970ce to
322378f
Compare
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
This reverts commit 8b797d7.
Closes #392
Puts back the old "disband PI" functionality (removed in #207) using the term "disable" instead of "disband". A PI group can now have the
isDisabledattribute set to"TRUE"and it will be ignored by the portal. If a PI is once again approved for a PI group, the old group will haveisDisabledset to"FALSE"or unset.Note: Disabled PI groups should have no members.
Note: I used OIDs
1.1.1and1.1.2for my new LDAP stuff.1.1is documented as a "dead namespace" in OID land and is advisable for "private experiments". copilot doesn't like that I did this.Note: if a large PI group is disabled, while updating the qualified status of each member, the portal will query for the members' attributes one-at-a-time and then do
$user->getPIGroupGIDs()one-at-a-time, which is a lot of LDAP queries. I implemented an optimized version of this, but it's very ugly.Test cases added:
UnityGroup->setIsDisabled()changes return value of futureUnityGroup->getIsDisabled()$user->isPI()returns truepi-mgmt.php,$group->getIsDisabled()returns true and$owner->isPI()returns falsepi.php,$group->getIsDisabled()returns true and$owner->isPI()returns falseisDisabledunset and groups withisDisabled = "FALSE"are both displayed inpi-mgmt.php