Add teams membership

[afig]

This PR implements changes that allow students to become members of a team. Becoming a member of a team allows students to create and access objects in the team's shared schema.

In regards to team membership, three functions have been created: addToTeam(), removeFromTeam(), and removeAllFromTeam(). As discussed in PR #221 , removeFromTeam() also reassigns ownership of objects in the teams schema to the team whenever a student is removed. To do so, two other functions have been added: reassignObjectOwnership() and reassignOwnedInSchema().

Tests have been created to verify the nominal functionality of the new functions. These tests could be expanded further, particularly for exceptional cases, but I believe the current tests will suffice for M3.

A couple of semi-related changes were made:

• The COMMIT at the end of testHelpers.sql has been changed to a ROLLBACK. See PR #197
• A tiny but incredibly difficult to trace down bug was fixed in listOwnedObjects(). See commit 86353bdeee54def133351d6e4f3c3afda14e99ba

Currently, not all object types are reassigned when removing a student from a team. This is due to a known limitation of listOwnedObjects(). The affected object types are relatively uncommon and can be addressed at a later point. Also note that while "foreign tables" are listed by listOwnedObjects(), they are not supported by reassignObjectOwnership(), since testing is not currently feasible.

Closes: #221

[smurthys]

Thanks @afig for this work. All unit tests pass in a new database. Privilege tests cause errors, but they seem unrelated to the changes in this PR.

Nice catch and fix on the bug in listOwnedObjects. Also, thanks for the well-researched implementation of function reassignObjectOwnership. 👍

addClassDBRolesMgmtCore.sql:

• L729-740: The various ALTER statements can be executed in one query batch, but I like the current organization because it gives (better) visibility to the operations being performed. However, I recommend capturing the result of getSchemaName in a variable and reusing it throughout. (Similar observations apply to L743-L754, L796-L807, L811-L822.)
• L793: I recommend defining and using a function isTeamMember(student, team) that is accessible to instructors and dbman.

addHelpersCore.sql:

• L479-L485: I am curious about the reason to not support foreign tables because it seems all information needed to reassign ownership is available, but perhaps there is more to it than meets the eye? EDIT: I now see in your PR comment that foreign tables are excluded due to no tests being available.
  • ALTER FOREIGN TABLE ONLY objectName TO newOwner
• L488-L493: It seems converting $1 to upper case in L464 eliminates the need for conversions and makes it possible to use NOT IN to detect invalid object types
• L542: Not an issue, but the use of PERFORM * is neat.

I have not reviewed the changes to testHelpers.sql, but all tests pass.

[smurthys]

@afig Please add a "missing" issue about your observation of listOwnedObjects() not returning all object types.

[wildtayne]

I haven't done a thorough review of the code yet, but everything works well so far. Everything seems to work in casual usage, and all the tests pass. Note that if you have the changes #240 in your DB, testClassDBRolesMgmt.sql will fail due to #243 (but applying any of the fixes mentioned in #243 will at least temporarily fix the tests).

As a sidenote, I also ran the privilege tests with a current branches merged in and installed and they all seem to pass. The only errors I got were expected. They could probably use some expansion with the new features added, however.

[afig]

Thanks for the review and suggestions @smurthys. The three changes outlined have been implemented in the latest commits.

I was not sure whether to allow foreign tables. The syntax appears the same, however, I did not want to add it without testing it, and testing it would involve studying how to create a foreign table (which seems to involve creating a foreign server).

Also thanks to @srrollo for reviewing as well. The privilege tests do need some expansion. I plan to do this in a separate PR.

[smurthys]

I still haven't review the test scripts, but the other changes look good. I'm curious to know what other reviews say.

[wildtayne]

I think the tests look good. I think this is ready to merge, pending the remaining decisions about object reassignment.

[KevinKelly25]

Everything looks really good so far. Tests all pass

[smurthys]

@srrollo by "pending the remaining decisions about object reassignment", I assume you mean Issue #244. If so, that issue will not be addressed in M3.