Skip to content
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

Made UnityGroup class more generic #98

Open
wants to merge 25 commits into
base: main
Choose a base branch
from

Conversation

sheldor1510
Copy link
Collaborator

@sheldor1510 sheldor1510 requested a review from hakasapl August 3, 2023 01:01
@hakasapl
Copy link
Collaborator

hakasapl commented Aug 4, 2023

This is a good start. Now that you have renamed the pi_groups to unity_groups/unitygroups, you'll now need to build the logic around the UnityGroup class. We had discussed that it would be good to include the frontend and the backend in the same PR so it's easier to test together.

@hakasapl hakasapl marked this pull request as ready for review August 30, 2023 21:04
@hakasapl
Copy link
Collaborator

@sheldor1510 this PR looks good. I think I had previously said I would add LDAP attributes for groupType, startTime, and endTime. I've thought about it a bit and I think it would be better to store these things in MySQL and use LDAP only for the objects. That being said, could you add to this PR a groupAttributes table in MySQL that has the columns groupID, startTime, endTime, and groupType? We can add any future attributes as columns to this table. In addition, could you also write the relevant methods for setting/modifying these values and add them to the UnityGroup class and wherever they need to be called?

Copy link
Collaborator

@hakasapl hakasapl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ran into some issues that I left comments on. Unfortunately I can't continue testing because the comment I mentioned with the fatal error prevents me from continuing testing. Once that is resolved I will continue.

In addition, it would be great if you could comment all of the new methods/code that you added. For methods, use phpdoc syntax, which you can find examples in other parts of this code base. Comments will help me track your code a bit better.

Thank you for your work!

webroot/panel/new_group.php Outdated Show resolved Hide resolved
webroot/panel/new_group.php Show resolved Hide resolved
tools/docker-dev/sql/bootstrap.sql Show resolved Hide resolved
webroot/admin/group-mgmt.php Show resolved Hide resolved
return $users;
}

public function assignRole($role, $uid, $gid)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Getting a fatal error when trying to approve a group request for a class/center using the user [email protected]. I tried approving the class group using the same user and got this:

Notice: Trying to access array offset on value of type bool in /var/www/unity-web-portal/resources/lib/UnitySQL.php on line 646

Fatal error: Uncaught PDOException: SQLSTATE[23000]: Integrity constraint violation: 1048 Column 'user' cannot be null in /var/www/unity-web-portal/resources/lib/UnitySQL.php:524 Stack trace: #0 /var/www/unity-web-portal/resources/lib/UnitySQL.php(524): PDOStatement->execute() #1 /var/www/unity-web-portal/resources/lib/UnitySQL.php(751): UnityWebPortal\lib\UnitySQL->assignRole() #2 /var/www/unity-web-portal/resources/lib/UnityGroup.php(538): UnityWebPortal\lib\UnitySQL->assignSuperRole() #3 /var/www/unity-web-portal/resources/lib/UnityGroup.php(172): UnityWebPortal\lib\UnityGroup->init() #4 /var/www/unity-web-portal/webroot/admin/group-mgmt.php(25): UnityWebPortal\lib\UnityGroup->approveGroup() #5 {main} thrown in /var/www/unity-web-portal/resources/lib/UnitySQL.php on line 524

I haven't tracked down the origin of this

webroot/panel/groups.php Show resolved Hide resolved
@sheldor1510 sheldor1510 requested a review from Shaswat975 May 24, 2024 15:35
Copy link
Collaborator

@hakasapl hakasapl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work on this. Keep in mind that I am reproducing the notes in this review in Firefox, but that shouldn't cause an issue as most of this is server-side code.

A few other more generic things:

  • The user management page takes a very long time to load, can you reproduce that?

For the next iteration, let's test the following cases:

  • A non-admin user requesting to join a non-admin group
    • Test approve/deny from the account that owns the group
  • A non-admin user requesting any of the 3 example group types
  • An admin approving or denying all of the 3 example group types

<input type='hidden' name='pi' value='" . $group->getPIUID() . "'>
<input type='submit' value='Leave Group'>
<input type='hidden' name='pi' value='" . $group->getGroupUID() . "'>
<input type='submit' value='Leave Group' style='margin-top: 0px'>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pressing on the Leave Group button on the groups page initially shows a dialog but then immediately redirects to the group details page.

<form id="newGroupForm" action="" method="POST">
<p>Fill in the following information to request a new group</p>
<div>
<strong>Type of Group</strong><br>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For specifically the PI group type, I am unable to request a new group. The page refreshes but nothing happens. The other 2 types work fine.

Comment on lines +21 to +25
$group_type = $_POST["group_type"];
$group_name = $_POST["group_name"];
$group_uid = $group_type . "_" . $group_name;
$group = new UnityGroup($group_uid, $LDAP, $SQL, $MAILER, $REDIS, $WEBHOOK);
$group->approveGroup($_POST["uid"], $OPERATOR);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When trying to approve any group, I get this php error. In my case the requestor of the group is [email protected] and the approver is also [email protected].


Notice: Trying to access array offset on value of type bool in /var/www/unity-web-portal/resources/lib/UnitySQL.php on line 649

Fatal error: Uncaught PDOException: SQLSTATE[23000]: Integrity constraint violation: 1048 Column 'user' cannot be null in /var/www/unity-web-portal/resources/lib/UnitySQL.php:524 Stack trace: #0 /var/www/unity-web-portal/resources/lib/UnitySQL.php(524): PDOStatement->execute() #1 /var/www/unity-web-portal/resources/lib/UnitySQL.php(766): UnityWebPortal\lib\UnitySQL->assignRole() #2 /var/www/unity-web-portal/resources/lib/UnityGroup.php(538): UnityWebPortal\lib\UnitySQL->assignSuperRole() #3 /var/www/unity-web-portal/resources/lib/UnityGroup.php(172): UnityWebPortal\lib\UnityGroup->init() #4 /var/www/unity-web-portal/webroot/admin/group-mgmt.php(25): UnityWebPortal\lib\UnityGroup->approveGroup() #5 {main} thrown in /var/www/unity-web-portal/resources/lib/UnitySQL.php on line 524

@hakasapl
Copy link
Collaborator

This PR covers the issue described here.

In its current state it should be feature complete. The remaining things to touch on are:

  • Bug fixes (there are probably a good amount of both server-side, and client-side issues at the moment)
  • Frontend touch-ups - there seems to be some inconsistent frontend changes that might need to be touched a bit to get looking completely right.
  • Merge conflicts - this PR has been open a while so there are some things to fix with the new main branch

The entire goal of this PR is making groups on the web portal generic. This means that people deploying the web portal define the group types and requirements, and users can select these. Before this PR, UnityGroup class in this code base was meant to be a PI group and nothing else. In this PR's bootstrap.sql there are example group types for PI, Class, and Center, which is what was originally requested by the Unity cluster, but it's designed in a way that types are arbitrary and can be extended to whatever the deployment needs. For example, you can make the Class group require a timeframe when requesting a group.

The issue linked about goes into all of this in more detail, so this is just an overview.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants