DASSL / ClassDB

An open-source system to let students experiment with relational data
https://dassl.github.io/ClassDB/
Other
7 stars 2 forks source link

Add team management #226

Closed afig closed 6 years ago

afig commented 6 years ago

This PR focuses on adding functions to create, revoke, and drop teams in ClassDB. In particular, it leaves out functionality that manages team membership, which will be added in a separate PR due to the large number of changes.

Unit tests for all 4 functions have been created, and all pass on my instance except for testCreateTeam(), which fails with Code 4 due to a pending decision related to optional fullNames (see Issue #218). Currently, if a fullName is not provided, it is set to teamName (final behavior will depend on the decision made in #218, as such, do not approve this PR until then).

As @smurthys discussed in PR #211, the tests in testClassDBRolesMgmt.sql need several improvements in relation to avoiding unnecessary operations and improving consistency. The new tests were created with these improvements in mind, but the rest of the test script has been left unchanged, leaving the improvements for future development.

Changes to privilege tests will wait for team membership functionality to be added.

Closes: #218, Closes: #219, Closes: #220

smurthys commented 6 years ago

Thanks for making these enhancements @afig. It looks like only a small amount of work is left to fully support teams.

afig commented 6 years ago

I just pushed a commit that addresses the issue of not allowing full names to be optional. This involved slightly modifying the behavior of createRole() and a CHECK constraint in the RoleBase table (see discussion at issue #218) for more information.

The order that the arguments are verified in createRole() had to be slightly modified so that isTeam is validated before using it to check whether or not to validate fullName

I think this PR is ready for a final review. As mentioned earlier, functionality that allows team members to be added and removed will be added in a separate PR.

wildtayne commented 6 years ago

Starting reviewing this and it looks good so far. I ran into an issue with the tests crashing because NOT NULL was still being universally enforced for 'FullName' in ClassDB.RoleBase. I believe this issue is that if you apply the changes to an existing system, RoleBase will (correctly) not be replaced.

This is similar to the upgrade issues with connection logging, and I propose the following fix. Add:

   ALTER TABLE ClassDB.RoleBase ALTER COLUMN FullName DROP NOT NULL;
   ALTER TABLE ClassDB.RoleBase ADD CONSTRAINT EnforceFullName CHECK(isTeam OR (TRIM(FullName) <> '' AND FullName IS NOT NULL));

before ClassDB.RoleBase is created with a comment stating this is for 2.0 to 2.x upgrade, etc. (This appears to work on my system). I think this will need some logic to check if RoleBase exists and if the new constraint exists. DROP NOT NULL is idempotent, but ADD CONSTRAINT fails if the constraint already exists.

afig commented 6 years ago

Thanks for pointing that out @srrollo, I had not tried running the scripts on a DB that already had ClassDB installed. I also noticed another change that needed an upgrade statement, so I added that.

Thankfully, both ALTER TABLE and ... DROP CONSTRAINT have IF EXISTS variants, so nothing procedural needed to be written. rolebase_fullname_check is the default name for the constraint, so to keep it simple, I kept that name even when adding the constraint separately.

EDIT: The Role Creation Process diagram (both sizes) and the corresponding HTML file has been updated to match the latest behavior of createRole()

smurthys commented 6 years ago

Excellent enhancement @afig. I have eyeballed the code and don't see any issue. I will run the scripts in my installation in a few hours and report back.

wildtayne commented 6 years ago

Nice find on the IF EXISTS clauses, it's quite nice to have all of those. I think this looks good to go.

smurthys commented 6 years ago

The changed scripts compile and the tests run successfully.

I find only two minor issues in testClassDBRolesMgmt.sql. Other than these, this PR seems is ready to merge.

afig commented 6 years ago

Thanks for pointing out those comment issues @smurthys, they have been addressed in the most recent commit.