SAK-51713 Tests & Quizzes Instructors should see all student submissions when a assessment is published to the entire site#278
Conversation
WalkthroughAdds database migration scripts for Sakai 25.1 to 25.2 supporting MySQL and Oracle. Introduces 'assessment.all.groups' permission and uses temporary tables to map roles to functions, applying permissions across realms while preventing duplicates via NOT EXISTS clauses. Changes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/conversion/sakai_26_mysql_conversion.sql(1 hunks)docs/conversion/sakai_26_oracle_conversion.sql(1 hunks)
…ons when a assessment is published to the entire site
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
docs/conversion/sakai_25_1-25_2_mysql_conversion.sql (1)
8-8: Temporary table operations lack defensive guards.
CREATE TABLEwill fail if tables already exist from a partial previous run, andDROP TABLEwill fail if tables were never created. Adding defensive guards improves script resilience.🔎 Suggested fix:
-- Add this to populate existing sites with the permission +DROP TABLE IF EXISTS PERMISSIONS_SRC_TEMP; CREATE TABLE PERMISSIONS_SRC_TEMP (ROLE_NAME VARCHAR(99), FUNCTION_NAME VARCHAR(99));+DROP TABLE IF EXISTS PERMISSIONS_TEMP; CREATE TABLE PERMISSIONS_TEMP (ROLE_KEY INTEGER, FUNCTION_KEY INTEGER);-DROP TABLE PERMISSIONS_TEMP; -DROP TABLE PERMISSIONS_SRC_TEMP; +DROP TABLE IF EXISTS PERMISSIONS_TEMP; +DROP TABLE IF EXISTS PERMISSIONS_SRC_TEMP;Also applies to: 12-12, 32-33
docs/conversion/sakai_25_1-25_2_oracle_conversion.sql (1)
16-16: Temporary table operations may fail on partial re-runs.Oracle's
CREATE TABLEfails if the table exists, andDROP TABLEfails if it doesn't. For robustness, consider dropping before creating. In Oracle 12.2+, you can use thePRIVATE TEMPORARY TABLEfeature, or for older versions, add a preceding drop with exception handling.🔎 Simpler approach - drop before create:
-- Add this to populate existing sites with the permission +BEGIN EXECUTE IMMEDIATE 'DROP TABLE PERMISSIONS_SRC_TEMP'; EXCEPTION WHEN OTHERS THEN NULL; END; +/ CREATE TABLE PERMISSIONS_SRC_TEMP (ROLE_NAME VARCHAR(99), FUNCTION_NAME VARCHAR(99));+BEGIN EXECUTE IMMEDIATE 'DROP TABLE PERMISSIONS_TEMP'; EXCEPTION WHEN OTHERS THEN NULL; END; +/ CREATE TABLE PERMISSIONS_TEMP (ROLE_KEY INTEGER, FUNCTION_KEY INTEGER);Or at the end:
-DROP TABLE PERMISSIONS_TEMP; -DROP TABLE PERMISSIONS_SRC_TEMP; +BEGIN EXECUTE IMMEDIATE 'DROP TABLE PERMISSIONS_TEMP'; EXCEPTION WHEN OTHERS THEN NULL; END; +/ +BEGIN EXECUTE IMMEDIATE 'DROP TABLE PERMISSIONS_SRC_TEMP'; EXCEPTION WHEN OTHERS THEN NULL; END; +/Also applies to: 20-20, 41-42
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/conversion/sakai_25_1-25_2_mysql_conversion.sql(1 hunks)docs/conversion/sakai_25_1-25_2_oracle_conversion.sql(1 hunks)
🔇 Additional comments (3)
docs/conversion/sakai_25_1-25_2_mysql_conversion.sql (1)
19-30: LGTM!The bulk insert logic correctly uses
NOT EXISTSfor idempotency and appropriately excludes helper and user template realms. The join strategy to propagate permissions to existing realms is sound.docs/conversion/sakai_25_1-25_2_oracle_conversion.sql (2)
3-11: LGTM!The
MERGEstatement properly handles idempotent insertion of the function, avoiding errors on re-run. This is the correct Oracle pattern for "insert if not exists".
27-39: LGTM!The bulk insert logic is correct and idempotent. The
NOT EXISTSclause prevents duplicates, and the realm exclusions are appropriate.
| INSERT INTO SAKAI_REALM_RL_FN VALUES((select REALM_KEY from SAKAI_REALM where REALM_ID = '!site.template'), (select ROLE_KEY from SAKAI_REALM_ROLE where ROLE_NAME = 'maintain'), (select FUNCTION_KEY from SAKAI_REALM_FUNCTION where FUNCTION_NAME = 'assessment.all.groups')); | ||
| INSERT INTO SAKAI_REALM_RL_FN VALUES((select REALM_KEY from SAKAI_REALM where REALM_ID = '!site.template.course'), (select ROLE_KEY from SAKAI_REALM_ROLE where ROLE_NAME = 'Instructor'), (select FUNCTION_KEY from SAKAI_REALM_FUNCTION where FUNCTION_NAME = 'assessment.all.groups')); |
There was a problem hiding this comment.
Template inserts lack idempotency and may fail on re-run.
These direct INSERT statements will fail with a duplicate key error if the script runs a second time (or if the permission already exists). Additionally, if any subquery returns NULL (e.g., role or realm not found), the insert will either fail or insert invalid data.
Consider using INSERT IGNORE or wrapping with a NOT EXISTS check to match the idempotent pattern used in the bulk insert below.
🔎 Suggested fix using INSERT IGNORE:
-- Add this for every role able to create and manage conditions on a site, you'll need to add the tool too
-INSERT INTO SAKAI_REALM_RL_FN VALUES((select REALM_KEY from SAKAI_REALM where REALM_ID = '!site.template'), (select ROLE_KEY from SAKAI_REALM_ROLE where ROLE_NAME = 'maintain'), (select FUNCTION_KEY from SAKAI_REALM_FUNCTION where FUNCTION_NAME = 'assessment.all.groups'));
-INSERT INTO SAKAI_REALM_RL_FN VALUES((select REALM_KEY from SAKAI_REALM where REALM_ID = '!site.template.course'), (select ROLE_KEY from SAKAI_REALM_ROLE where ROLE_NAME = 'Instructor'), (select FUNCTION_KEY from SAKAI_REALM_FUNCTION where FUNCTION_NAME = 'assessment.all.groups'));
+INSERT IGNORE INTO SAKAI_REALM_RL_FN VALUES((select REALM_KEY from SAKAI_REALM where REALM_ID = '!site.template'), (select ROLE_KEY from SAKAI_REALM_ROLE where ROLE_NAME = 'maintain'), (select FUNCTION_KEY from SAKAI_REALM_FUNCTION where FUNCTION_NAME = 'assessment.all.groups'));
+INSERT IGNORE INTO SAKAI_REALM_RL_FN VALUES((select REALM_KEY from SAKAI_REALM where REALM_ID = '!site.template.course'), (select ROLE_KEY from SAKAI_REALM_ROLE where ROLE_NAME = 'Instructor'), (select FUNCTION_KEY from SAKAI_REALM_FUNCTION where FUNCTION_NAME = 'assessment.all.groups'));🤖 Prompt for AI Agents
In docs/conversion/sakai_25_1-25_2_mysql_conversion.sql around lines 5 to 6, the
two direct INSERT statements are not idempotent and will error on duplicate keys
or insert NULLs if the subqueries return no rows; change them to perform
conditional inserts (e.g., INSERT IGNORE ... SELECT ... or INSERT ... SELECT ...
WHERE NOT EXISTS (...) ) and ensure the SELECTs only run when the subquery
values are non-NULL (add WHERE clauses checking existence of the realm, role and
function keys or join to those tables instead of scalar subqueries) so
re-running the script is safe and no invalid rows are inserted.
| INSERT INTO SAKAI_REALM_RL_FN VALUES((select REALM_KEY from SAKAI_REALM where REALM_ID = '!site.template'), (select ROLE_KEY from SAKAI_REALM_ROLE where ROLE_NAME = 'maintain'), (select FUNCTION_KEY from SAKAI_REALM_FUNCTION where FUNCTION_NAME = 'assessment.all.groups')); | ||
| INSERT INTO SAKAI_REALM_RL_FN VALUES((select REALM_KEY from SAKAI_REALM where REALM_ID = '!site.template.course'), (select ROLE_KEY from SAKAI_REALM_ROLE where ROLE_NAME = 'Instructor'), (select FUNCTION_KEY from SAKAI_REALM_FUNCTION where FUNCTION_NAME = 'assessment.all.groups')); |
There was a problem hiding this comment.
Template inserts lack idempotency and may fail on re-run.
Same issue as the MySQL version: these will fail with a constraint violation if run twice. Oracle doesn't have INSERT IGNORE, so consider using MERGE or wrapping each insert with a NOT EXISTS check.
🔎 Suggested fix using MERGE:
-- Add this for every role able to create and manage conditions on a site, you'll need to add the tool too
-INSERT INTO SAKAI_REALM_RL_FN VALUES((select REALM_KEY from SAKAI_REALM where REALM_ID = '!site.template'), (select ROLE_KEY from SAKAI_REALM_ROLE where ROLE_NAME = 'maintain'), (select FUNCTION_KEY from SAKAI_REALM_FUNCTION where FUNCTION_NAME = 'assessment.all.groups'));
-INSERT INTO SAKAI_REALM_RL_FN VALUES((select REALM_KEY from SAKAI_REALM where REALM_ID = '!site.template.course'), (select ROLE_KEY from SAKAI_REALM_ROLE where ROLE_NAME = 'Instructor'), (select FUNCTION_KEY from SAKAI_REALM_FUNCTION where FUNCTION_NAME = 'assessment.all.groups'));
+MERGE INTO SAKAI_REALM_RL_FN t
+USING (
+ SELECT
+ (SELECT REALM_KEY FROM SAKAI_REALM WHERE REALM_ID = '!site.template') AS REALM_KEY,
+ (SELECT ROLE_KEY FROM SAKAI_REALM_ROLE WHERE ROLE_NAME = 'maintain') AS ROLE_KEY,
+ (SELECT FUNCTION_KEY FROM SAKAI_REALM_FUNCTION WHERE FUNCTION_NAME = 'assessment.all.groups') AS FUNCTION_KEY
+ FROM dual
+) s ON (t.REALM_KEY = s.REALM_KEY AND t.ROLE_KEY = s.ROLE_KEY AND t.FUNCTION_KEY = s.FUNCTION_KEY)
+WHEN NOT MATCHED THEN INSERT (REALM_KEY, ROLE_KEY, FUNCTION_KEY) VALUES (s.REALM_KEY, s.ROLE_KEY, s.FUNCTION_KEY);
+
+MERGE INTO SAKAI_REALM_RL_FN t
+USING (
+ SELECT
+ (SELECT REALM_KEY FROM SAKAI_REALM WHERE REALM_ID = '!site.template.course') AS REALM_KEY,
+ (SELECT ROLE_KEY FROM SAKAI_REALM_ROLE WHERE ROLE_NAME = 'Instructor') AS ROLE_KEY,
+ (SELECT FUNCTION_KEY FROM SAKAI_REALM_FUNCTION WHERE FUNCTION_NAME = 'assessment.all.groups') AS FUNCTION_KEY
+ FROM dual
+) s ON (t.REALM_KEY = s.REALM_KEY AND t.ROLE_KEY = s.ROLE_KEY AND t.FUNCTION_KEY = s.FUNCTION_KEY)
+WHEN NOT MATCHED THEN INSERT (REALM_KEY, ROLE_KEY, FUNCTION_KEY) VALUES (s.REALM_KEY, s.ROLE_KEY, s.FUNCTION_KEY);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| INSERT INTO SAKAI_REALM_RL_FN VALUES((select REALM_KEY from SAKAI_REALM where REALM_ID = '!site.template'), (select ROLE_KEY from SAKAI_REALM_ROLE where ROLE_NAME = 'maintain'), (select FUNCTION_KEY from SAKAI_REALM_FUNCTION where FUNCTION_NAME = 'assessment.all.groups')); | |
| INSERT INTO SAKAI_REALM_RL_FN VALUES((select REALM_KEY from SAKAI_REALM where REALM_ID = '!site.template.course'), (select ROLE_KEY from SAKAI_REALM_ROLE where ROLE_NAME = 'Instructor'), (select FUNCTION_KEY from SAKAI_REALM_FUNCTION where FUNCTION_NAME = 'assessment.all.groups')); | |
| MERGE INTO SAKAI_REALM_RL_FN t | |
| USING ( | |
| SELECT | |
| (SELECT REALM_KEY FROM SAKAI_REALM WHERE REALM_ID = '!site.template') AS REALM_KEY, | |
| (SELECT ROLE_KEY FROM SAKAI_REALM_ROLE WHERE ROLE_NAME = 'maintain') AS ROLE_KEY, | |
| (SELECT FUNCTION_KEY FROM SAKAI_REALM_FUNCTION WHERE FUNCTION_NAME = 'assessment.all.groups') AS FUNCTION_KEY | |
| FROM dual | |
| ) s ON (t.REALM_KEY = s.REALM_KEY AND t.ROLE_KEY = s.ROLE_KEY AND t.FUNCTION_KEY = s.FUNCTION_KEY) | |
| WHEN NOT MATCHED THEN INSERT (REALM_KEY, ROLE_KEY, FUNCTION_KEY) VALUES (s.REALM_KEY, s.ROLE_KEY, s.FUNCTION_KEY); | |
| MERGE INTO SAKAI_REALM_RL_FN t | |
| USING ( | |
| SELECT | |
| (SELECT REALM_KEY FROM SAKAI_REALM WHERE REALM_ID = '!site.template.course') AS REALM_KEY, | |
| (SELECT ROLE_KEY FROM SAKAI_REALM_ROLE WHERE ROLE_NAME = 'Instructor') AS ROLE_KEY, | |
| (SELECT FUNCTION_KEY FROM SAKAI_REALM_FUNCTION WHERE FUNCTION_NAME = 'assessment.all.groups') AS FUNCTION_KEY | |
| FROM dual | |
| ) s ON (t.REALM_KEY = s.REALM_KEY AND t.ROLE_KEY = s.ROLE_KEY AND t.FUNCTION_KEY = s.FUNCTION_KEY) | |
| WHEN NOT MATCHED THEN INSERT (REALM_KEY, ROLE_KEY, FUNCTION_KEY) VALUES (s.REALM_KEY, s.ROLE_KEY, s.FUNCTION_KEY); |
🤖 Prompt for AI Agents
In docs/conversion/sakai_25_1-25_2_oracle_conversion.sql around lines 13 to 14,
the two INSERT statements for SAKAI_REALM_RL_FN are not idempotent and will fail
on duplicate key constraint on re-run; replace each INSERT with an
Oracle-friendly idempotent operation (e.g., use a MERGE that matches on the
composite key columns or wrap the insert in a conditional INSERT ... SELECT that
only inserts when a corresponding row does not exist using NOT EXISTS), ensuring
you compute the same subselect values for realm/role/function and use them as
the MERGE key predicates or in the NOT EXISTS check so repeated runs do nothing
if the mapping already exists.
1 participant
https://sakaiproject.atlassian.net/browse/SAK-51713
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.