Closed afig closed 7 years ago
Super work.
On duplicated code: desperate times, desperate measures. :)
I wonder if dbman should be able to create PUBLIC schema, especially right now that role is also doubling as "Teaching Assistant". However, I would not change anything on this, except may be add an enhancement issue that a TA role is required.
addCatalogMgmt.sql:
Use WITH foldedPgSchema(id) (...)
instead of using the AS
clause at end of column expression: make column names apparentid
seems like an odd name for a column that joins with a schema name: foldedSchemaName
?JOIN
instead of WHERE
: make intention explicit, be declarativeWould you please opportunistically fix #112 in your next commit?
Looks good so far. I would also add the line I mention in #106, which drops the default privileges in public from an instructor when they are dropped. Otherwise, the default privileges block them from being dropped.
I am a bit lost having just re-joined the conversation. I will wait for @afig to chime in with a summary of status/clarification/decision on the points by @srrollo and @s-aithal.
Several concerns raised in comments have been fixed:
The clarity fixes for addCatalogMgmt.sql that were proposed by @smurthys have been implemented. Syntax and naming such as foldedPgSchema(foldedSchemaName)
is now used instead of AS id
in the Common Table Expressions. Also a JOIN is now used rather than matching specific WHERE conditions.
Issue #112 that was brought up by @s-aithal has been resolved. Comments in removeFromServer.sql
have been updated to match the new filename of what is now removeFromDB.sql
.
A comment by @srrollo in Issue #106 brought up another problem that was introduced by this PR, where instructors could not be dropped due them still having default privileges set in the public schema. A line in dropInstructor
has been added which removes the default privileges before dropping the instructor's role.
This PR fixes most of the issues that were identified during the development an testing of the privilege tests.
classdb.Instructor
table.foldPgID
is no longer used for thecatalogMgmt
functions. This is because that function was in theclassdb
schema, which is inaccessible to Students. Currently, code is being duplicated in both functions insidecatalogMgmt
, rather messily. We may even want to move thefoldPgID
function to the public schema instead. I will open an issue regarding this.public
schema can now be read by any role (public)This PR also fixes two other issues that were identified but did not have Issues created for them:
dropAllStudents
did not execute at all: The statement inside of the procedure needed to use PERFORM rather than SELECT. This has been fixed.Students and DBManagers could create tables in the public schema: I do not recall this being an issue early on in development, but at some point CREATE ON public was not being REVOKEd to all users. This has also been fixed.