From 7198eb6429c06787154575e39ba64c6fb8cd2d05 Mon Sep 17 00:00:00 2001 From: Dmitry Zenovich Date: Wed, 6 Nov 2024 15:46:53 +0300 Subject: [PATCH 01/12] add and initialize a new column groups_groups.is_team_membership --- ...0230158_add_column_groups_groups_is_team_membership.sql | 7 +++++++ ..._initialize_column_groups_groups_is_team_membership.sql | 4 ++++ 2 files changed, 11 insertions(+) create mode 100644 db/migrations/2410230158_add_column_groups_groups_is_team_membership.sql create mode 100644 db/migrations/2410230202_initialize_column_groups_groups_is_team_membership.sql diff --git a/db/migrations/2410230158_add_column_groups_groups_is_team_membership.sql b/db/migrations/2410230158_add_column_groups_groups_is_team_membership.sql new file mode 100644 index 000000000..5423c18fc --- /dev/null +++ b/db/migrations/2410230158_add_column_groups_groups_is_team_membership.sql @@ -0,0 +1,7 @@ +-- +migrate Up +ALTER TABLE `groups_groups` ADD COLUMN `is_team_membership` TINYINT(1) NOT NULL DEFAULT 0 + COMMENT 'true if the parent group is a team' + AFTER `expires_at`; + +-- +migrate Down +ALTER TABLE `groups_groups` DROP COLUMN `is_team_membership`; diff --git a/db/migrations/2410230202_initialize_column_groups_groups_is_team_membership.sql b/db/migrations/2410230202_initialize_column_groups_groups_is_team_membership.sql new file mode 100644 index 000000000..a61347e62 --- /dev/null +++ b/db/migrations/2410230202_initialize_column_groups_groups_is_team_membership.sql @@ -0,0 +1,4 @@ +-- +migrate Up +UPDATE `groups_groups` SET `is_team_membership` = 1 WHERE `parent_group_id` IN (SELECT `id` FROM `groups` WHERE `type` = 'Team'); + +-- +migrate Down From 8b88353b6a80944f2cbd55521e9e4f5776ce488d Mon Sep 17 00:00:00 2001 From: Dmitry Zenovich Date: Wed, 6 Nov 2024 15:47:47 +0300 Subject: [PATCH 02/12] modify trigger before_insert_groups_groups to set groups_groups.is_team_membership --- ...t_groups_groups_to_set_is_team_membership.sql | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) create mode 100644 db/migrations/2410260221_modify_trigger_before_insert_groups_groups_to_set_is_team_membership.sql diff --git a/db/migrations/2410260221_modify_trigger_before_insert_groups_groups_to_set_is_team_membership.sql b/db/migrations/2410260221_modify_trigger_before_insert_groups_groups_to_set_is_team_membership.sql new file mode 100644 index 000000000..2c51c709c --- /dev/null +++ b/db/migrations/2410260221_modify_trigger_before_insert_groups_groups_to_set_is_team_membership.sql @@ -0,0 +1,16 @@ +-- +migrate Up +DROP TRIGGER IF EXISTS `before_insert_groups_groups`; +-- +migrate StatementBegin +CREATE TRIGGER `before_insert_groups_groups` BEFORE INSERT ON `groups_groups` FOR EACH ROW BEGIN + SET NEW.is_team_membership = (SELECT type = 'Team' FROM `groups` WHERE id = NEW.parent_group_id FOR SHARE); + IF NOT NEW.is_team_membership THEN + INSERT IGNORE INTO `groups_propagate` (id, ancestors_computation_state) VALUES (NEW.child_group_id, 'todo') ON DUPLICATE KEY UPDATE `ancestors_computation_state` = 'todo'; + END IF; +END +-- +migrate StatementEnd + +-- +migrate Down +DROP TRIGGER IF EXISTS `before_insert_groups_groups`; +-- +migrate StatementBegin +CREATE TRIGGER `before_insert_groups_groups` BEFORE INSERT ON `groups_groups` FOR EACH ROW BEGIN INSERT IGNORE INTO `groups_propagate` (id, ancestors_computation_state) VALUES (NEW.child_group_id, 'todo') ON DUPLICATE KEY UPDATE `ancestors_computation_state` = 'todo' ; END +-- +migrate StatementEnd From cf5c1f2285f4bf3dad7f3911fbdc10af9cea726f Mon Sep 17 00:00:00 2001 From: Dmitry Zenovich Date: Wed, 6 Nov 2024 15:48:41 +0300 Subject: [PATCH 03/12] modify trigger before_update_groups_groups to disallow modifying groups_groups.is_team_membership --- ...sallow_modifying_of_is_team_membership.sql | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) create mode 100644 db/migrations/2410281951_modify_trigger_before_update_groups_groups_to_disallow_modifying_of_is_team_membership.sql diff --git a/db/migrations/2410281951_modify_trigger_before_update_groups_groups_to_disallow_modifying_of_is_team_membership.sql b/db/migrations/2410281951_modify_trigger_before_update_groups_groups_to_disallow_modifying_of_is_team_membership.sql new file mode 100644 index 000000000..1310efca2 --- /dev/null +++ b/db/migrations/2410281951_modify_trigger_before_update_groups_groups_to_disallow_modifying_of_is_team_membership.sql @@ -0,0 +1,19 @@ +-- +migrate Up +DROP TRIGGER `before_update_groups_groups`; +-- +migrate StatementBegin +CREATE TRIGGER `before_update_groups_groups` BEFORE UPDATE ON `groups_groups` FOR EACH ROW BEGIN + IF OLD.`parent_group_id` != NEW.`parent_group_id` OR OLD.`child_group_id` != NEW.`child_group_id` OR OLD.`is_team_membership` != NEW.`is_team_membership` THEN + SIGNAL SQLSTATE '45000' SET MESSAGE_TEXT = 'Unable to change immutable columns of groups_groups (parent_group_id/child_group_id/is_team_membership)'; + END IF; +END +-- +migrate StatementEnd + +-- +migrate Down +DROP TRIGGER `before_update_groups_groups`; +-- +migrate StatementBegin +CREATE TRIGGER `before_update_groups_groups` BEFORE UPDATE ON `groups_groups` FOR EACH ROW BEGIN + IF OLD.`parent_group_id` != NEW.`parent_group_id` OR OLD.`child_group_id` != NEW.`child_group_id` THEN + SIGNAL SQLSTATE '45000' SET MESSAGE_TEXT = 'Unable to change immutable groups_groups.parent_group_id and/or groups_groups.child_group_id'; + END IF; +END +-- +migrate StatementEnd From 7cf492d9c7a92e42af892541a8235df7f877772a Mon Sep 17 00:00:00 2001 From: Dmitry Zenovich Date: Wed, 6 Nov 2024 15:50:14 +0300 Subject: [PATCH 04/12] do not mark groups for propagation on changing team memberships --- ...oups_groups_to_ignore_team_memberships.sql | 98 ++++++++++++++++ ...oups_groups_to_ignore_team_memberships.sql | 111 ++++++++++++++++++ ...oups_groups_to_ignore_team_memberships.sql | 19 +++ 3 files changed, 228 insertions(+) create mode 100644 db/migrations/2410282058_modify_trigger_after_insert_groups_groups_to_ignore_team_memberships.sql create mode 100644 db/migrations/2410282314_modify_trigger_after_update_groups_groups_to_ignore_team_memberships.sql create mode 100644 db/migrations/2410282327_modify_trigger_before_delete_groups_groups_to_ignore_team_memberships.sql diff --git a/db/migrations/2410282058_modify_trigger_after_insert_groups_groups_to_ignore_team_memberships.sql b/db/migrations/2410282058_modify_trigger_after_insert_groups_groups_to_ignore_team_memberships.sql new file mode 100644 index 000000000..20efddf45 --- /dev/null +++ b/db/migrations/2410282058_modify_trigger_after_insert_groups_groups_to_ignore_team_memberships.sql @@ -0,0 +1,98 @@ +-- +migrate Up +DROP TRIGGER `after_insert_groups_groups`; +-- +migrate StatementBegin +CREATE TRIGGER `after_insert_groups_groups` AFTER INSERT ON `groups_groups` FOR EACH ROW BEGIN + IF NEW.`expires_at` > NOW() AND NOT NEW.`is_team_membership` THEN + INSERT IGNORE INTO `results_propagate` + SELECT `participant_id`, `attempt_id`, `results`.`item_id`, 'to_be_propagated' AS `state` + FROM ( + SELECT `item_id` + FROM ( + SELECT DISTINCT `item_id` + FROM `results` + JOIN `groups_ancestors_active` + ON `groups_ancestors_active`.`child_group_id` = `results`.`participant_id` AND + `groups_ancestors_active`.`ancestor_group_id` = NEW.`child_group_id` + FOR SHARE + ) AS `result_items` + WHERE EXISTS( + SELECT 1 + FROM `permissions_generated` + JOIN `groups_ancestors_active` AS `grand_ancestors` + ON `grand_ancestors`.`child_group_id` = NEW.`parent_group_id` AND + `grand_ancestors`.`ancestor_group_id` = `permissions_generated`.`group_id` + JOIN `items_ancestors` + ON `items_ancestors`.`ancestor_item_id` = `permissions_generated`.`item_id` + WHERE `items_ancestors`.`child_item_id` = `result_items`.`item_id` + AND `permissions_generated`.`can_view_generated` != 'none' + FOR SHARE + ) + AND NOT EXISTS( + SELECT 1 + FROM `permissions_generated` + JOIN `groups_ancestors_active` AS `child_ancestors` + ON `child_ancestors`.`child_group_id` = NEW.`child_group_id` AND + `child_ancestors`.`ancestor_group_id` = `permissions_generated`.`group_id` + JOIN `items_ancestors` + ON `items_ancestors`.`ancestor_item_id` = `permissions_generated`.`item_id` + WHERE `items_ancestors`.`child_item_id` = `result_items`.`item_id` + AND `permissions_generated`.`can_view_generated` != 'none' + FOR SHARE + ) + FOR SHARE + ) AS `result_items_filtered` + JOIN `results` ON `results`.`item_id` = `result_items_filtered`.`item_id` + JOIN `groups_ancestors_active` + ON `groups_ancestors_active`.`child_group_id` = `results`.`participant_id` AND + `groups_ancestors_active`.`ancestor_group_id` = NEW.`child_group_id` + FOR SHARE; + END IF; +END +-- +migrate StatementEnd + +-- +migrate Down +DROP TRIGGER `after_insert_groups_groups`; +-- +migrate StatementBegin +CREATE TRIGGER `after_insert_groups_groups` AFTER INSERT ON `groups_groups` FOR EACH ROW BEGIN + IF NEW.`expires_at` > NOW() THEN + INSERT IGNORE INTO `results_propagate` + SELECT `participant_id`, `attempt_id`, `results`.`item_id`, 'to_be_propagated' AS `state` + FROM ( + SELECT `item_id` + FROM ( + SELECT DISTINCT `item_id` + FROM `results` + JOIN `groups_ancestors_active` + ON `groups_ancestors_active`.`child_group_id` = `results`.`participant_id` AND + `groups_ancestors_active`.`ancestor_group_id` = NEW.`child_group_id` + ) AS `result_items` + WHERE EXISTS( + SELECT 1 + FROM `permissions_generated` + JOIN `groups_ancestors_active` AS `grand_ancestors` + ON `grand_ancestors`.`child_group_id` = NEW.`parent_group_id` AND + `grand_ancestors`.`ancestor_group_id` = `permissions_generated`.`group_id` + JOIN `items_ancestors` + ON `items_ancestors`.`ancestor_item_id` = `permissions_generated`.`item_id` + WHERE `items_ancestors`.`child_item_id` = `result_items`.`item_id` + AND `permissions_generated`.`can_view_generated` != 'none' + ) + AND NOT EXISTS( + SELECT 1 + FROM `permissions_generated` + JOIN `groups_ancestors_active` AS `child_ancestors` + ON `child_ancestors`.`child_group_id` = NEW.`child_group_id` AND + `child_ancestors`.`ancestor_group_id` = `permissions_generated`.`group_id` + JOIN `items_ancestors` + ON `items_ancestors`.`ancestor_item_id` = `permissions_generated`.`item_id` + WHERE `items_ancestors`.`child_item_id` = `result_items`.`item_id` + AND `permissions_generated`.`can_view_generated` != 'none' + ) + ) AS `result_items_filtered` + JOIN `results` ON `results`.`item_id` = `result_items_filtered`.`item_id` + JOIN `groups_ancestors_active` + ON `groups_ancestors_active`.`child_group_id` = `results`.`participant_id` AND + `groups_ancestors_active`.`ancestor_group_id` = NEW.`child_group_id`; + END IF; +END +-- +migrate StatementEnd diff --git a/db/migrations/2410282314_modify_trigger_after_update_groups_groups_to_ignore_team_memberships.sql b/db/migrations/2410282314_modify_trigger_after_update_groups_groups_to_ignore_team_memberships.sql new file mode 100644 index 000000000..c8c32f5d6 --- /dev/null +++ b/db/migrations/2410282314_modify_trigger_after_update_groups_groups_to_ignore_team_memberships.sql @@ -0,0 +1,111 @@ +-- +migrate Up +DROP TRIGGER `after_update_groups_groups`; +-- +migrate StatementBegin +CREATE TRIGGER `after_update_groups_groups` AFTER UPDATE ON `groups_groups` FOR EACH ROW BEGIN + IF OLD.expires_at != NEW.expires_at AND NOT NEW.`is_team_membership` THEN + IF NEW.`expires_at` > NOW() THEN + INSERT IGNORE INTO `results_propagate` + SELECT `participant_id`, `attempt_id`, `results`.`item_id`, 'to_be_propagated' AS `state` + FROM ( + SELECT `item_id` + FROM ( + SELECT DISTINCT `item_id` + FROM `results` + JOIN `groups_ancestors_active` + ON `groups_ancestors_active`.`child_group_id` = `results`.`participant_id` AND + `groups_ancestors_active`.`ancestor_group_id` = NEW.`child_group_id` + FOR SHARE + ) AS `result_items` + WHERE EXISTS( + SELECT 1 + FROM `permissions_generated` + JOIN `groups_ancestors_active` AS `grand_ancestors` + ON `grand_ancestors`.`child_group_id` = NEW.`parent_group_id` AND + `grand_ancestors`.`ancestor_group_id` = `permissions_generated`.`group_id` + JOIN `items_ancestors` + ON `items_ancestors`.`ancestor_item_id` = `permissions_generated`.`item_id` + WHERE `items_ancestors`.`child_item_id` = `result_items`.`item_id` + AND `permissions_generated`.`can_view_generated` != 'none' + FOR SHARE + ) + AND NOT EXISTS( + SELECT 1 + FROM `permissions_generated` + JOIN `groups_ancestors_active` AS `child_ancestors` + ON `child_ancestors`.`child_group_id` = NEW.`child_group_id` AND + `child_ancestors`.`ancestor_group_id` = `permissions_generated`.`group_id` + JOIN `items_ancestors` + ON `items_ancestors`.`ancestor_item_id` = `permissions_generated`.`item_id` + WHERE `items_ancestors`.`child_item_id` = `result_items`.`item_id` + AND `permissions_generated`.`can_view_generated` != 'none' + FOR SHARE + ) + FOR SHARE + ) AS `result_items_filtered` + JOIN `results` ON `results`.`item_id` = `result_items_filtered`.`item_id` + JOIN `groups_ancestors_active` + ON `groups_ancestors_active`.`child_group_id` = `results`.`participant_id` AND + `groups_ancestors_active`.`ancestor_group_id` = NEW.`child_group_id` + FOR SHARE; + END IF; + + INSERT IGNORE INTO `groups_propagate` (`id`, `ancestors_computation_state`) VALUES (NEW.child_group_id, 'todo') + ON DUPLICATE KEY UPDATE `ancestors_computation_state` = 'todo'; + END IF; +END +-- +migrate StatementEnd + +-- +migrate Down +DROP TRIGGER `after_update_groups_groups`; +-- +migrate StatementBegin +CREATE TRIGGER `after_update_groups_groups` AFTER UPDATE ON `groups_groups` FOR EACH ROW BEGIN + IF OLD.expires_at != NEW.expires_at THEN + IF NEW.`expires_at` > NOW() THEN + INSERT IGNORE INTO `results_propagate` + SELECT `participant_id`, `attempt_id`, `results`.`item_id`, 'to_be_propagated' AS `state` + FROM ( + SELECT `item_id` + FROM ( + SELECT DISTINCT `item_id` + FROM `results` + JOIN `groups_ancestors_active` + ON `groups_ancestors_active`.`child_group_id` = `results`.`participant_id` AND + `groups_ancestors_active`.`ancestor_group_id` = NEW.`child_group_id` + ) AS `result_items` + WHERE EXISTS( + SELECT 1 + FROM `permissions_generated` + JOIN `groups_ancestors_active` AS `grand_ancestors` + ON `grand_ancestors`.`child_group_id` = NEW.`parent_group_id` AND + `grand_ancestors`.`ancestor_group_id` = `permissions_generated`.`group_id` + JOIN `items_ancestors` + ON `items_ancestors`.`ancestor_item_id` = `permissions_generated`.`item_id` + WHERE `items_ancestors`.`child_item_id` = `result_items`.`item_id` + AND `permissions_generated`.`can_view_generated` != 'none' + ) + AND NOT EXISTS( + SELECT 1 + FROM `permissions_generated` + JOIN `groups_ancestors_active` AS `child_ancestors` + ON `child_ancestors`.`child_group_id` = NEW.`child_group_id` AND + `child_ancestors`.`ancestor_group_id` = `permissions_generated`.`group_id` + JOIN `items_ancestors` + ON `items_ancestors`.`ancestor_item_id` = `permissions_generated`.`item_id` + WHERE `items_ancestors`.`child_item_id` = `result_items`.`item_id` + AND `permissions_generated`.`can_view_generated` != 'none' + ) + ) AS `result_items_filtered` + JOIN `results` ON `results`.`item_id` = `result_items_filtered`.`item_id` + JOIN `groups_ancestors_active` + ON `groups_ancestors_active`.`child_group_id` = `results`.`participant_id` AND + `groups_ancestors_active`.`ancestor_group_id` = NEW.`child_group_id`; + END IF; + + INSERT IGNORE INTO `groups_propagate` (`id`, `ancestors_computation_state`) VALUES (OLD.child_group_id, 'todo') + ON DUPLICATE KEY UPDATE `ancestors_computation_state` = 'todo'; + + INSERT IGNORE INTO `groups_propagate` (id, ancestors_computation_state) VALUES (NEW.child_group_id, 'todo') + ON DUPLICATE KEY UPDATE `ancestors_computation_state` = 'todo'; + END IF; +END +-- +migrate StatementEnd diff --git a/db/migrations/2410282327_modify_trigger_before_delete_groups_groups_to_ignore_team_memberships.sql b/db/migrations/2410282327_modify_trigger_before_delete_groups_groups_to_ignore_team_memberships.sql new file mode 100644 index 000000000..d4eff1a7b --- /dev/null +++ b/db/migrations/2410282327_modify_trigger_before_delete_groups_groups_to_ignore_team_memberships.sql @@ -0,0 +1,19 @@ +-- +migrate Up +DROP TRIGGER `before_delete_groups_groups`; +-- +migrate StatementBegin +CREATE TRIGGER `before_delete_groups_groups` BEFORE DELETE ON `groups_groups` FOR EACH ROW BEGIN + IF NOT OLD.`is_team_membership` THEN + INSERT IGNORE INTO `groups_propagate` (`id`, `ancestors_computation_state`) VALUES (OLD.child_group_id, 'todo') + ON DUPLICATE KEY UPDATE `ancestors_computation_state` = 'todo'; + END IF; +END +-- +migrate StatementEnd + +-- +migrate Down +DROP TRIGGER `before_delete_groups_groups`; +-- +migrate StatementBegin +CREATE TRIGGER `before_delete_groups_groups` BEFORE DELETE ON `groups_groups` FOR EACH ROW BEGIN + INSERT IGNORE INTO `groups_propagate` (`id`, `ancestors_computation_state`) VALUES (OLD.child_group_id, 'todo') + ON DUPLICATE KEY UPDATE `ancestors_computation_state` = 'todo'; +END +-- +migrate StatementEnd From 3c23612a0860ef6b4143706cca5e4a0bf8e4cbb9 Mon Sep 17 00:00:00 2001 From: Dmitry Zenovich Date: Wed, 6 Nov 2024 15:51:09 +0300 Subject: [PATCH 05/12] add trigger before_update_groups to disallow modifying groups.type --- ...efore_update_groups_to_disallow_modifying_type.sql | 11 +++++++++++ 1 file changed, 11 insertions(+) create mode 100644 db/migrations/2410282334_add_trigger_before_update_groups_to_disallow_modifying_type.sql diff --git a/db/migrations/2410282334_add_trigger_before_update_groups_to_disallow_modifying_type.sql b/db/migrations/2410282334_add_trigger_before_update_groups_to_disallow_modifying_type.sql new file mode 100644 index 000000000..7986e3c13 --- /dev/null +++ b/db/migrations/2410282334_add_trigger_before_update_groups_to_disallow_modifying_type.sql @@ -0,0 +1,11 @@ +-- +migrate Up +-- +migrate StatementBegin +CREATE TRIGGER `before_update_groups` BEFORE UPDATE ON `groups` FOR EACH ROW BEGIN + IF OLD.`type` != NEW.`type` THEN + SIGNAL SQLSTATE '45000' SET MESSAGE_TEXT = 'Unable to change immutable groups.type'; + END IF; +END +-- +migrate StatementEnd + +-- +migrate Down +DROP TRIGGER `before_update_groups`; From 8962d5988c1d53a534a98045bc1eb1b3beeb7312 Mon Sep 17 00:00:00 2001 From: Dmitry Zenovich Date: Wed, 6 Nov 2024 16:15:56 +0300 Subject: [PATCH 06/12] add/modify tests for triggers, fix tests to support groups_groups.is_team_membership --- .../answers/create_answer.robustness.feature | 5 +- app/api/answers/submit.feature | 4 - app/api/answers/submit.robustness.feature | 3 - .../answers/update_current.robustness.feature | 4 + app/api/currentuser/get_dump.feature | 12 ++- app/api/currentuser/get_full_dump.feature | 15 ++-- .../get_group_activities.robustness.feature | 1 + .../get_group_skills.robustness.feature | 1 + .../currentuser/get_managed_groups.feature | 13 ++-- app/api/items/ask_hint.feature | 1 - app/api/items/ask_hint.robustness.feature | 4 - app/api/items/save_grade.feature | 1 - app/api/items/save_grade.robustness.feature | 4 - .../group_group_store_integration_test.go | 76 +++++++++++++++++-- app/database/group_store_integration_test.go | 13 ++++ .../item_item_store_integration_test.go | 1 + app/database/user_integration_test.go | 3 +- testhelpers/app_language_groups.go | 18 ++--- testhelpers/app_language_users.go | 11 +-- testhelpers/test_context.go | 14 ++++ 20 files changed, 145 insertions(+), 59 deletions(-) diff --git a/app/api/answers/create_answer.robustness.feature b/app/api/answers/create_answer.robustness.feature index 875011375..1a623536a 100644 --- a/app/api/answers/create_answer.robustness.feature +++ b/app/api/answers/create_answer.robustness.feature @@ -4,8 +4,9 @@ Feature: Create a 'saved' answer - robustness | login | group_id | | john | 101 | And the database table "groups" also has the following row: - | id | type | - | 13 | Team | + | id | type | + | 13 | Team | + | 22 | Class | And the database has the following table "groups_groups": | parent_group_id | child_group_id | | 22 | 13 | diff --git a/app/api/answers/submit.feature b/app/api/answers/submit.feature index dd2a16b42..4ab60a1e9 100644 --- a/app/api/answers/submit.feature +++ b/app/api/answers/submit.feature @@ -6,10 +6,6 @@ Feature: Submit a new answer And the database has the following table "groups": | id | name | type | | 201 | team | Team | - And the database has the following table "groups_groups": - | parent_group_id | child_group_id | - | 22 | 13 | - | 201 | 101 | And the groups ancestors are computed And the database has the following table "items": | id | default_language_tag | diff --git a/app/api/answers/submit.robustness.feature b/app/api/answers/submit.robustness.feature index 655e99d1e..8a50ffb95 100644 --- a/app/api/answers/submit.robustness.feature +++ b/app/api/answers/submit.robustness.feature @@ -3,9 +3,6 @@ Feature: Submit a new answer - robustness Given the database has the following users: | login | group_id | | john | 101 | - And the database has the following table "groups_groups": - | parent_group_id | child_group_id | - | 22 | 13 | And the groups ancestors are computed And the database has the following table "items": | id | read_only | default_language_tag | diff --git a/app/api/answers/update_current.robustness.feature b/app/api/answers/update_current.robustness.feature index 927929cbd..10d64c0c7 100644 --- a/app/api/answers/update_current.robustness.feature +++ b/app/api/answers/update_current.robustness.feature @@ -3,6 +3,10 @@ Feature: Update participant's current answer Given the database has the following users: | login | group_id | | john | 101 | + And the database has the following table "groups": + | id | + | 13 | + | 22 | And the database has the following table "groups_groups": | parent_group_id | child_group_id | | 22 | 13 | diff --git a/app/api/currentuser/get_dump.feature b/app/api/currentuser/get_dump.feature index f09b65de0..8807a1dc3 100644 --- a/app/api/currentuser/get_dump.feature +++ b/app/api/currentuser/get_dump.feature @@ -95,28 +95,32 @@ Feature: Export the short version of the current user's data "lock_membership_approved_at": null, "lock_membership_approved": 0, "personal_info_view_approved_at": null, "personal_info_view_approved": 0, "watch_approved_at": null, "watch_approved": 0, - "name": "Our Team", "expires_at": "9999-12-31T23:59:59Z" + "name": "Our Team", "expires_at": "9999-12-31T23:59:59Z", + "is_team_membership": 1 }, { "child_group_id": "11", "parent_group_id": "5", "lock_membership_approved_at": null, "lock_membership_approved": 0, "personal_info_view_approved_at": null, "personal_info_view_approved": 0, "watch_approved_at": null, "watch_approved": 0, - "name": "Other people", "expires_at": "9999-12-31T23:59:59Z" + "name": "Other people", "expires_at": "9999-12-31T23:59:59Z", + "is_team_membership": 0 }, { "child_group_id": "11", "parent_group_id": "6", "lock_membership_approved_at": null, "lock_membership_approved": 0, "personal_info_view_approved_at": null, "personal_info_view_approved": 0, "watch_approved_at": null, "watch_approved": 0, - "name": "Another Class", "expires_at": "9999-12-31T23:59:59Z" + "name": "Another Class", "expires_at": "9999-12-31T23:59:59Z", + "is_team_membership": 0 }, { "child_group_id": "11", "parent_group_id": "9", "lock_membership_approved_at": null, "lock_membership_approved": 0, "personal_info_view_approved_at": null, "personal_info_view_approved": 0, "watch_approved_at": null, "watch_approved": 0, - "name": "Some other friends", "expires_at": "9999-12-31T23:59:59Z" + "name": "Some other friends", "expires_at": "9999-12-31T23:59:59Z", + "is_team_membership": 0 } ], "group_managers": [ diff --git a/app/api/currentuser/get_full_dump.feature b/app/api/currentuser/get_full_dump.feature index d2d77eefc..c8c24e1fb 100644 --- a/app/api/currentuser/get_full_dump.feature +++ b/app/api/currentuser/get_full_dump.feature @@ -136,35 +136,40 @@ Feature: Export the current user's data "lock_membership_approved_at": null, "lock_membership_approved": 0, "personal_info_view_approved_at": null, "personal_info_view_approved": 0, "watch_approved_at": null, "watch_approved": 0, - "name": "Our Team", "expires_at": "9999-12-31T23:59:59Z" + "name": "Our Team", "expires_at": "9999-12-31T23:59:59Z", + "is_team_membership": 1 }, { "child_group_id": "11", "parent_group_id": "5", "lock_membership_approved_at": null, "lock_membership_approved": 0, "personal_info_view_approved_at": null, "personal_info_view_approved": 0, "watch_approved_at": null, "watch_approved": 0, - "name": "Other people", "expires_at": "9999-12-31T23:59:59Z" + "name": "Other people", "expires_at": "9999-12-31T23:59:59Z", + "is_team_membership": 0 }, { "child_group_id": "11", "parent_group_id": "6", "lock_membership_approved_at": null, "lock_membership_approved": 0, "personal_info_view_approved_at": null, "personal_info_view_approved": 0, "watch_approved_at": null, "watch_approved": 0, - "name": "Another Class", "expires_at": "9999-12-31T23:59:59Z" + "name": "Another Class", "expires_at": "9999-12-31T23:59:59Z", + "is_team_membership": 0 }, { "child_group_id": "11", "parent_group_id": "9", "lock_membership_approved_at": null, "lock_membership_approved": 0, "personal_info_view_approved_at": null, "personal_info_view_approved": 0, "watch_approved_at": null, "watch_approved": 0, - "name": "Some other friends", "expires_at": "9999-12-31T23:59:59Z" + "name": "Some other friends", "expires_at": "9999-12-31T23:59:59Z", + "is_team_membership": 0 }, { "child_group_id": "11", "parent_group_id": "10", "lock_membership_approved_at": null, "lock_membership_approved": 0, "personal_info_view_approved_at": null, "personal_info_view_approved": 0, "watch_approved_at": null, "watch_approved": 0, - "name": "Secret group", "expires_at": "9999-12-31T23:59:59Z" + "name": "Secret group", "expires_at": "9999-12-31T23:59:59Z", + "is_team_membership": 0 } ], "group_managers": [ diff --git a/app/api/currentuser/get_group_activities.robustness.feature b/app/api/currentuser/get_group_activities.robustness.feature index 85e2ecfa4..750c1f168 100644 --- a/app/api/currentuser/get_group_activities.robustness.feature +++ b/app/api/currentuser/get_group_activities.robustness.feature @@ -2,6 +2,7 @@ Feature: Get root activities for a participant group - robustness Background: Given the database has the following table "groups": | id | name | type | root_activity_id | created_at | + | 1 | Class | Class | null | 2019-01-30 08:26:46 | | 11 | jdoe | User | null | 2019-01-30 08:26:48 | | 13 | Group B | Team | 230 | 2019-01-30 08:26:46 | | 14 | Group C | Team | 230 | 2019-01-30 08:26:46 | diff --git a/app/api/currentuser/get_group_skills.robustness.feature b/app/api/currentuser/get_group_skills.robustness.feature index 629638546..50ce94f4d 100644 --- a/app/api/currentuser/get_group_skills.robustness.feature +++ b/app/api/currentuser/get_group_skills.robustness.feature @@ -2,6 +2,7 @@ Feature: Get root skills for a participant group - robustness Background: Given the database has the following table "groups": | id | name | type | root_activity_id | created_at | + | 1 | Class | Class | null | 2019-01-30 08:26:46 | | 13 | Group B | Team | 230 | 2019-01-30 08:26:46 | | 14 | Group C | Team | 230 | 2019-01-30 08:26:46 | And the database has the following table "languages": diff --git a/app/api/currentuser/get_managed_groups.feature b/app/api/currentuser/get_managed_groups.feature index 1ce9a5636..515dac9e2 100644 --- a/app/api/currentuser/get_managed_groups.feature +++ b/app/api/currentuser/get_managed_groups.feature @@ -1,11 +1,14 @@ Feature: List groups managed by the current user Background: Given the database has the following table "groups": - | id | name | type | description | - | 5 | Group | Class | null | - | 13 | Our Class | Class | null | - | 14 | Our Friends | Other | null | - | 15 | Another Group | Other | Super Group | + | id | name | type | description | + | 1 | Friends | Friends | null | + | 5 | Group | Class | null | + | 6 | Club | Club | null | + | 9 | Other | Other | null | + | 13 | Our Class | Class | null | + | 14 | Our Friends | Other | null | + | 15 | Another Group | Other | Super Group | And the database has the following users: | group_id | login | | 21 | owner | diff --git a/app/api/items/ask_hint.feature b/app/api/items/ask_hint.feature index 27fcd5f95..59382d44f 100644 --- a/app/api/items/ask_hint.feature +++ b/app/api/items/ask_hint.feature @@ -8,7 +8,6 @@ Feature: Ask for a hint | 201 | team | Team | And the database has the following table "groups_groups": | parent_group_id | child_group_id | - | 22 | 13 | | 201 | 101 | And the groups ancestors are computed And the database has the following table "platforms": diff --git a/app/api/items/ask_hint.robustness.feature b/app/api/items/ask_hint.robustness.feature index 12daaf3ce..2cfdf1c67 100644 --- a/app/api/items/ask_hint.robustness.feature +++ b/app/api/items/ask_hint.robustness.feature @@ -3,10 +3,6 @@ Feature: Ask for a hint - robustness Given the database has the following users: | login | group_id | | john | 101 | - And the database has the following table "groups_groups": - | parent_group_id | child_group_id | - | 22 | 13 | - And the groups ancestors are computed And the database has the following table "platforms": | id | regexp | public_key | priority | | 10 | https://platformwithkey | {{taskPlatformPublicKey}} | 0 | diff --git a/app/api/items/save_grade.feature b/app/api/items/save_grade.feature index e1a7f4b2c..248af89f1 100644 --- a/app/api/items/save_grade.feature +++ b/app/api/items/save_grade.feature @@ -8,7 +8,6 @@ Feature: Save grading result | 201 | team | Team | And the database has the following table "groups_groups": | parent_group_id | child_group_id | - | 22 | 13 | | 201 | 101 | And the groups ancestors are computed And the database has the following table "platforms": diff --git a/app/api/items/save_grade.robustness.feature b/app/api/items/save_grade.robustness.feature index f39b3d937..f8706b28a 100644 --- a/app/api/items/save_grade.robustness.feature +++ b/app/api/items/save_grade.robustness.feature @@ -3,10 +3,6 @@ Feature: Save grading result - robustness Given the database has the following users: | login | group_id | | john | 101 | - And the database has the following table "groups_groups": - | parent_group_id | child_group_id | - | 22 | 13 | - And the groups ancestors are computed And the database has the following table "platforms": | id | regexp | priority | public_key | | 10 | http://taskplatform.mblockelet.info/task.html\?.* | 2 | {{taskPlatformPublicKey}} | diff --git a/app/database/group_group_store_integration_test.go b/app/database/group_group_store_integration_test.go index 63bc1b4ef..0f7d5d640 100644 --- a/app/database/group_group_store_integration_test.go +++ b/app/database/group_group_store_integration_test.go @@ -7,6 +7,7 @@ import ( "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "github.com/France-ioi/AlgoreaBackend/v2/app/database" "github.com/France-ioi/AlgoreaBackend/v2/testhelpers" @@ -265,14 +266,16 @@ const ( - {ancestor_item_id: 1, child_item_id: 2} - {ancestor_item_id: 1, child_item_id: 3} - {ancestor_item_id: 2, child_item_id: 3} - groups: [{id: 101}, {id: 102}, {id: 103}, {id: 104}, {id: 105}, {id: 106}, {id: 107}, {id: 108}] + groups: [{id: 101}, {id: 102}, {id: 103}, {id: 104}, {id: 105}, {id: 106}, {id: 107}, {id: 108}, {id: 109, type: Team}] groups_groups: - {parent_group_id: 101, child_group_id: 102} - {parent_group_id: 101, child_group_id: 103} - {parent_group_id: 101, child_group_id: 104} - {parent_group_id: 101, child_group_id: 105} - {parent_group_id: 101, child_group_id: 106} + - {parent_group_id: 101, child_group_id: 109} - {parent_group_id: 102, child_group_id: 103} + - {parent_group_id: 102, child_group_id: 109} - {parent_group_id: 104, child_group_id: 105} - {parent_group_id: 107, child_group_id: 105} - {parent_group_id: 108, child_group_id: 104, expires_at: 2019-05-30 11:00:00} @@ -291,6 +294,7 @@ const ( - {id: 1, participant_id: 106} - {id: 1, participant_id: 107} - {id: 1, participant_id: 108} + - {id: 1, participant_id: 109} results: - {attempt_id: 1, participant_id: 101, item_id: 1} - {attempt_id: 1, participant_id: 102, item_id: 1} @@ -300,6 +304,7 @@ const ( - {attempt_id: 1, participant_id: 106, item_id: 1} - {attempt_id: 1, participant_id: 107, item_id: 1} - {attempt_id: 1, participant_id: 108, item_id: 1} + - {attempt_id: 1, participant_id: 109, item_id: 1} - {attempt_id: 1, participant_id: 101, item_id: 2} - {attempt_id: 1, participant_id: 102, item_id: 2} - {attempt_id: 1, participant_id: 103, item_id: 2} @@ -308,6 +313,7 @@ const ( - {attempt_id: 1, participant_id: 106, item_id: 2} - {attempt_id: 1, participant_id: 107, item_id: 2} - {attempt_id: 1, participant_id: 108, item_id: 2} + - {attempt_id: 1, participant_id: 109, item_id: 2} - {attempt_id: 1, participant_id: 101, item_id: 3} - {attempt_id: 1, participant_id: 102, item_id: 3} - {attempt_id: 1, participant_id: 103, item_id: 3} @@ -315,7 +321,8 @@ const ( - {attempt_id: 1, participant_id: 105, item_id: 3} - {attempt_id: 1, participant_id: 106, item_id: 3} - {attempt_id: 1, participant_id: 107, item_id: 3} - - {attempt_id: 1, participant_id: 108, item_id: 3}` + - {attempt_id: 1, participant_id: 108, item_id: 3} + - {attempt_id: 1, participant_id: 109, item_id: 3}` ) func TestGroupGroupStore_TriggerAfterInsert_MarksResultsAsChanged(t *testing.T) { @@ -345,6 +352,13 @@ func TestGroupGroupStore_TriggerAfterInsert_MarksResultsAsChanged(t *testing.T) expiresAt: "2019-05-30 11:00:00", expectedChanged: []resultPrimaryKey{}, }, + { + name: "group joins a group, but the relation is a team membership", + parentGroupID: 109, + childGroupID: 104, + expiresAt: "9999-12-31 23:59:59", + expectedChanged: []resultPrimaryKey{}, + }, { name: "group having no results joins another group", parentGroupID: 105, @@ -407,6 +421,13 @@ func TestGroupGroupStore_TriggerAfterUpdate_MarksResultsAsChanged(t *testing.T) {105, 1, 3}, }, }, + { + name: "restore an expired relation for a team membership", + parentGroupID: 109, + childGroupID: 104, + expiresAt: "9999-12-31 23:59:59", + expectedChanged: []resultPrimaryKey{}, + }, { name: "expire the relation", parentGroupID: 103, @@ -453,9 +474,9 @@ func TestGroupGroupStore_TriggerAfterUpdate_MarksResultsAsChanged(t *testing.T) expiresAt = maxDateTime } db := testhelpers.SetupDBWithFixtureString( + groupGroupMarksResultsAsChangedFixture, fmt.Sprintf("groups_groups: [{parent_group_id: %d, child_group_id: %d, expires_at: %s}]", - test.parentGroupID, test.childGroupID, expiresAt), - groupGroupMarksResultsAsChangedFixture) + test.parentGroupID, test.childGroupID, expiresAt)) defer func() { _ = db.Close() }() dataStore := database.NewDataStore(db) @@ -479,14 +500,15 @@ func TestGroupGroupStore_TriggerAfterUpdate_MarksResultsAsChanged(t *testing.T) } } -func TestGroupGroupStore_TriggerBeforeUpdate_RefusesToModifyParentGroupIDOrChildGroupID(t *testing.T) { +func TestGroupGroupStore_TriggerBeforeUpdate_RefusesToModifyParentGroupIDOrChildGroupIDOrIsTeamMembership(t *testing.T) { db := testhelpers.SetupDBWithFixtureString(` + groups: [{id: 1}, {id: 2}] groups_groups: [{parent_group_id: 1, child_group_id: 2}] `) defer func() { _ = db.Close() }() - const expectedErrorMessage = "Error 1644: Unable to change immutable " + - "groups_groups.parent_group_id and/or groups_groups.child_group_id" + const expectedErrorMessage = "Error 1644: Unable to change immutable columns of groups_groups " + + "(parent_group_id/child_group_id/is_team_membership)" groupGroupStore := database.NewDataStore(db).GroupGroups() result := groupGroupStore.Where("parent_group_id = 1 AND child_group_id = 2"). @@ -495,6 +517,9 @@ func TestGroupGroupStore_TriggerBeforeUpdate_RefusesToModifyParentGroupIDOrChild result = groupGroupStore.Where("parent_group_id = 1 AND child_group_id = 2"). UpdateColumn("child_group_id", 3) assert.EqualError(t, result.Error(), expectedErrorMessage) + result = groupGroupStore.Where("parent_group_id = 1 AND child_group_id = 2"). + UpdateColumn("is_team_membership", 1) + assert.EqualError(t, result.Error(), expectedErrorMessage) } type resultPrimaryKey struct { @@ -547,3 +572,40 @@ func queryResultsAndStatesForTests(t *testing.T, s *database.ResultStore, result Joins("RIGHT JOIN results_propagate USING(participant_id, attempt_id, item_id)")). Order("participant_id, attempt_id, item_id").Scan(result).Error()) } + +func TestGroupGroupStore_TriggerBeforeDelete(t *testing.T) { + testhelpers.SuppressOutputIfPasses(t) + + db := testhelpers.SetupDBWithFixtureString(` + groups: [{id: 1}, {id: 2}, {id: 3, type: Team}, {id: 4}] + groups_groups: [{parent_group_id: 1, child_group_id: 2}, {parent_group_id: 3, child_group_id: 4}]`) + + dataStore := database.NewDataStore(db) + require.NoError(t, dataStore.InTransaction(func(store *database.DataStore) error { + return store.GroupGroups().CreateNewAncestors() + })) + + found, err := dataStore.Table("groups_propagate").Where("ancestors_computation_state != 'done'").HasRows() + require.NoError(t, err) + require.False(t, found) + + require.NoError(t, dataStore.GroupGroups().Delete("parent_group_id = 1 AND child_group_id = 2").Error()) + + type id struct { + ID int64 + } + var marked []id + require.NoError(t, dataStore.Table("groups_propagate").Where("ancestors_computation_state = 'todo'").Scan(&marked).Error()) + assert.Equal(t, []id{{2}}, marked) + require.NoError(t, dataStore.Table("groups_propagate"). + Where("ancestors_computation_state = 'todo'"). + UpdateColumn("ancestors_computation_state", "done").Error()) + found, err = dataStore.Table("groups_propagate").Where("ancestors_computation_state != 'done'").HasRows() + require.NoError(t, err) + require.False(t, found) + + require.NoError(t, dataStore.GroupGroups().Delete("parent_group_id = 3 AND child_group_id = 4").Error()) + found, err = dataStore.Table("groups_propagate").Where("ancestors_computation_state != 'done'").HasRows() + require.NoError(t, err) + assert.False(t, found) +} diff --git a/app/database/group_store_integration_test.go b/app/database/group_store_integration_test.go index 0b3e57a68..542509c36 100644 --- a/app/database/group_store_integration_test.go +++ b/app/database/group_store_integration_test.go @@ -363,3 +363,16 @@ func Test_GroupStore_DeleteGroup(t *testing.T) { assert.NoError(t, groupStore.Table("groups_propagate").Pluck("id", &ids).Error()) assert.Empty(t, ids) } + +func TestGroupStore_TriggerBeforeUpdate_RefusesToModifyType(t *testing.T) { + testhelpers.SuppressOutputIfPasses(t) + + db := testhelpers.SetupDBWithFixtureString(`groups: [{id: 1}]`) + defer func() { _ = db.Close() }() + + const expectedErrorMessage = "Error 1644: Unable to change immutable groups.type" + + groupGroupStore := database.NewDataStore(db).Groups() + result := groupGroupStore.ByID(1).UpdateColumn("type", "Team") + assert.EqualError(t, result.Error(), expectedErrorMessage) +} diff --git a/app/database/item_item_store_integration_test.go b/app/database/item_item_store_integration_test.go index 9465f4431..7ab3d6448 100644 --- a/app/database/item_item_store_integration_test.go +++ b/app/database/item_item_store_integration_test.go @@ -29,5 +29,6 @@ func TestItemItemStore_TriggerAfterInsert_MarksResultsAsChanged(t *testing.T) { {106, 1, 2}, {107, 1, 2}, {108, 1, 2}, + {109, 1, 2}, }) } diff --git a/app/database/user_integration_test.go b/app/database/user_integration_test.go index 9dbdb08fb..d926d3787 100644 --- a/app/database/user_integration_test.go +++ b/app/database/user_integration_test.go @@ -75,7 +75,7 @@ func TestUser_CanSeeAnswer(t *testing.T) { testhelpers.SuppressOutputIfPasses(t) db := testhelpers.SetupDBWithFixtureString(` - groups: [{id: 101}, {id: 111}, {id: 121}] + groups: [{id: 101}, {id: 102}, {id: 111}, {id: 121}] users: - {login: "john", group_id: 101} - {login: "jane", group_id: 111} @@ -84,7 +84,6 @@ func TestUser_CanSeeAnswer(t *testing.T) { - {parent_group_id: 102, child_group_id: 101} groups_ancestors: - {ancestor_group_id: 102, child_group_id: 101} - - {ancestor_group_id: 102, child_group_id: 102} languages: [{tag: fr}] items: - {id: 10, default_language_tag: fr} diff --git a/testhelpers/app_language_groups.go b/testhelpers/app_language_groups.go index d8e5b07ea..19cdc271e 100644 --- a/testhelpers/app_language_groups.go +++ b/testhelpers/app_language_groups.go @@ -129,16 +129,16 @@ func (ctx *TestContext) ThereAreTheFollowingGroups(groups *godog.Table) error { } // ThereIsAGroup creates a new group (type=Class). -func (ctx *TestContext) ThereIsAGroup(group string) error { +func (ctx *TestContext) ThereIsAGroup(group string) (err error) { + defer recoverPanics(&err) ctx.addGroup(group, "Class") - return nil } // ThereIsATeam creates a new team. -func (ctx *TestContext) ThereIsATeam(group string) error { +func (ctx *TestContext) ThereIsATeam(group string) (err error) { + defer recoverPanics(&err) ctx.addGroup(group, "Team") - return nil } @@ -202,15 +202,13 @@ func (ctx *TestContext) UserIsAMemberOfTheGroupWhoHasApprovedAccessToHisPersonal } // AllUsersGroupIsDefinedAsTheGroup creates and sets the allUsersGroup. -func (ctx *TestContext) AllUsersGroupIsDefinedAsTheGroup(group string) error { - err := ctx.ThereIsAGroup(group) - if err != nil { - return err - } +func (ctx *TestContext) AllUsersGroupIsDefinedAsTheGroup(group string) (err error) { + defer recoverPanics(&err) + + ctx.addGroup(group, "Base") groupPrimaryKey := ctx.getGroupPrimaryKey(ctx.getIDOfReference(group)) ctx.setGroupFieldInDatabase(groupPrimaryKey, "name", "AllUsers") - ctx.setGroupFieldInDatabase(groupPrimaryKey, "type", "Base") err = ctx.TheApplicationConfigIs(&godog.DocString{ Content: ` diff --git a/testhelpers/app_language_users.go b/testhelpers/app_language_users.go index 8c1b1d849..d216416e3 100644 --- a/testhelpers/app_language_users.go +++ b/testhelpers/app_language_users.go @@ -81,14 +81,11 @@ func (ctx *TestContext) setUserFieldInDatabase(primaryKey map[string]string, fie } // ThereIsAUser create a user. -func (ctx *TestContext) ThereIsAUser(name string) error { - ctx.addUser(name) - - err := ctx.ThereIsAGroup(name) - mustNotBeError(err) +func (ctx *TestContext) ThereIsAUser(name string) (err error) { + defer recoverPanics(&err) - groupPrimaryKey := ctx.getGroupPrimaryKey(ctx.getIDOfReference(name)) - ctx.setGroupFieldInDatabase(groupPrimaryKey, "type", "User") + ctx.addUser(name) + ctx.addGroup(name, "User") return nil } diff --git a/testhelpers/test_context.go b/testhelpers/test_context.go index 7feff3fd0..441d2ab86 100644 --- a/testhelpers/test_context.go +++ b/testhelpers/test_context.go @@ -7,6 +7,7 @@ import ( "fmt" "net/http" "os" + "runtime" "sort" "bou.ke/monkey" @@ -237,3 +238,16 @@ func mustNotBeError(err error) { panic(err) } } + +func recoverPanics(returnErr *error) { + if p := recover(); p != nil { + switch e := p.(type) { + case runtime.Error: + panic(e) + case error: + *returnErr = e + default: + panic(p) + } + } +} From 8e9bcf21b2d41278e1505f5f8423bda460b89d3c Mon Sep 17 00:00:00 2001 From: Dmitry Zenovich Date: Wed, 6 Nov 2024 16:32:39 +0300 Subject: [PATCH 07/12] change the DB username for migration-tests to match the definer of triggers in the dump file --- .circleci/config.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index 982706168..bf6f708ec 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -89,7 +89,7 @@ jobs: - image: circleci/mysql:8.0.28 command: --default-authentication-plugin=mysql_native_password --max-allowed-packet=10485760 environment: - MYSQL_USER: franceioi + MYSQL_USER: algorea MYSQL_PASSWORD: dummy_password MYSQL_DATABASE: algorea_example MYSQL_ROOT_PASSWORD: root From 621ed6867bedb2c2cba523a38944d33945ccb703 Mon Sep 17 00:00:00 2001 From: Dmitry Zenovich Date: Wed, 6 Nov 2024 19:50:34 +0300 Subject: [PATCH 08/12] make groups&items propagations twice faster: 1) move the 'processing' state into a new temporary table, 2) always filter *_propagate tables by exact values of ancestors_propagation_state, 3) perform covering index lookups on groups_groups instead of index lookups (create an index for this) --- app/database/ancestors.go | 98 +++++++++++-------- app/database/group_group_store_test.go | 4 + ..._propagate_ancestors_computation_state.sql | 5 + ..._propagate_ancestors_computation_state.sql | 5 + ...t_group_id_expires_at_on_groups_groups.sql | 6 ++ 5 files changed, 76 insertions(+), 42 deletions(-) create mode 100644 db/migrations/2410292333_remove_unnecessary_enum_values_from_groups_propagate_ancestors_computation_state.sql create mode 100644 db/migrations/2410292340_remove_unnecessary_enum_values_from_items_propagate_ancestors_computation_state.sql create mode 100644 db/migrations/2411061620_create_index_child_group_id_is_team_membership_parent_group_id_expires_at_on_groups_groups.sql diff --git a/app/database/ancestors.go b/app/database/ancestors.go index 8b1771da8..381f8b5f1 100644 --- a/app/database/ancestors.go +++ b/app/database/ancestors.go @@ -13,7 +13,7 @@ const groups = "groups" // // Note: rows in *_propagate tables with `ancestors_computation_state`="todo" // are added in the database in SQL triggers: -// - after_insert_items/groups +// - after_insert_items // - after_update_groups_groups // - before_insert_items_items/groups_groups // - before_delete_items_items/groups_groups. @@ -34,39 +34,46 @@ func (s *DataStore) createNewAncestors(objectName, singleObjectName string) { /* mustNotBeError(s.db.Exec(query).Error) - hasChanges := true + createTemporaryTableQuery := "CREATE TEMPORARY TABLE " + objectName + "_propagate_processing (id BIGINT NOT NULL)" + dropTemporaryTableQuery := "DROP TEMPORARY TABLE IF EXISTS " + objectName + "_propagate_processing" + mustNotBeError(s.db.Exec(createTemporaryTableQuery).Error) + defer func() { + mustNotBeError(s.db.Exec(dropTemporaryTableQuery).Error) + }() relationsTable := objectName + "_" + objectName - var additionalJoin, additionalLocking string + additionalRelationCondition := "1" if objectName == groups { - additionalJoin = " JOIN `groups` AS parent ON parent.id = groups_groups.parent_group_id AND parent.type != 'Team' " - additionalLocking = " FOR SHARE OF parent " + additionalRelationCondition = "groups_groups.is_team_membership = 0" } // Next queries will be executed in the loop - // We mark as "processing" all objects that were marked as 'todo' and that have no parents not marked as 'done' + // We mark as processing all objects that were marked as 'todo' and that have no parents not marked as 'done'. // This way we prevent infinite looping as we never process objects that are descendants of themselves /* #nosec */ markAsProcessingQuery := ` - UPDATE ` + objectName + `_propagate AS children - SET children.ancestors_computation_state='processing' - WHERE children.ancestors_computation_state = 'todo' AND NOT EXISTS ( - SELECT 1 FROM ( + INSERT INTO ` + objectName + `_propagate_processing (id) + SELECT id + FROM ` + objectName + `_propagate AS children + WHERE children.ancestors_computation_state = 'todo' AND + NOT EXISTS ( SELECT 1 FROM ` + relationsTable + ` JOIN ` + objectName + `_propagate ON ` + objectName + `_propagate.id = ` + relationsTable + `.parent_` + singleObjectName + `_id AND - ` + objectName + `_propagate.ancestors_computation_state <> 'done' - ` + additionalJoin + ` - WHERE ` + relationsTable + `.child_` + singleObjectName + `_id = children.id + ` + objectName + `_propagate.ancestors_computation_state = 'todo' + WHERE ` + relationsTable + `.child_` + singleObjectName + `_id = children.id AND ` + additionalRelationCondition + ` + LIMIT 1 FOR SHARE OF ` + relationsTable + ` FOR UPDATE OF ` + objectName + `_propagate - ` + additionalLocking + ` - ) has_undone_parents - )` + ) + FOR UPDATE OF children` // #nosec + createTemporaryTable, err := s.db.CommonDB().Prepare(createTemporaryTableQuery) + mustNotBeError(err) + defer func() { mustNotBeError(createTemporaryTable.Close()) }() markAsProcessing, err := s.db.CommonDB().Prepare(markAsProcessingQuery) mustNotBeError(err) defer func() { mustNotBeError(markAsProcessing.Close()) }() @@ -77,18 +84,17 @@ func (s *DataStore) createNewAncestors(objectName, singleObjectName string) { /* if objectName == groups { expiresAtColumn = ", expires_at" - expiresAtValueJoin = ", LEAST(groups_ancestors_join.expires_at, groups_groups.expires_at)" + expiresAtValueJoin = ", MAX(LEAST(groups_ancestors_join.expires_at, groups_groups.expires_at)) AS max_expires_at" ignore = "" } - // For every object marked as 'processing', we compute all its ancestors + // For every object marked as processing, we compute all its ancestors recomputeQueries := make([]string, 0, 3) recomputeQueries = append(recomputeQueries, ` DELETE `+objectName+`_ancestors FROM `+objectName+`_ancestors - JOIN `+objectName+`_propagate - ON `+objectName+`_propagate.id = `+objectName+`_ancestors.child_`+singleObjectName+`_id - WHERE `+objectName+`_propagate.ancestors_computation_state = 'processing'`, ` + JOIN `+objectName+`_propagate_processing + ON `+objectName+`_propagate_processing.id = `+objectName+`_ancestors.child_`+singleObjectName+`_id`, ` INSERT `+ignore+` INTO `+objectName+`_ancestors ( ancestor_`+singleObjectName+`_id, @@ -100,39 +106,35 @@ func (s *DataStore) createNewAncestors(objectName, singleObjectName string) { /* `+relationsTable+`.child_`+singleObjectName+`_id `+expiresAtValueJoin+` FROM `+relationsTable+` AS `+relationsTable+` - `+additionalJoin+` JOIN `+objectName+`_ancestors AS `+objectName+`_ancestors_join ON ( `+objectName+`_ancestors_join.child_`+singleObjectName+`_id = `+relationsTable+`.parent_`+singleObjectName+`_id ) - JOIN `+objectName+`_propagate ON ( - `+relationsTable+`.child_`+singleObjectName+`_id = `+objectName+`_propagate.id + JOIN `+objectName+`_propagate_processing ON ( + `+relationsTable+`.child_`+singleObjectName+`_id = `+objectName+`_propagate_processing.id ) - WHERE - `+objectName+`_propagate.ancestors_computation_state = 'processing'`) // #nosec + WHERE `+additionalRelationCondition) // #nosec if objectName == groups { recomputeQueries[0] += ` AND groups_ancestors.ancestor_group_id != groups_ancestors.child_group_id` // do not delete group ancestors with is_self=1 recomputeQueries[1] += ` - AND NOW() < groups_groups.expires_at AND - NOW() < LEAST(groups_ancestors_join.expires_at, groups_groups.expires_at) - FOR UPDATE OF ` + objectName + `_propagate + AND NOW() < groups_groups.expires_at + AND NOW() < groups_ancestors_join.expires_at + GROUP BY groups_groups.child_group_id, groups_ancestors_join.ancestor_group_id + HAVING NOW() < max_expires_at + FOR UPDATE OF ` + objectName + `_propagate_processing FOR SHARE OF ` + objectName + `_ancestors_join - FOR SHARE OF ` + relationsTable + ` - FOR SHARE OF parent - ON DUPLICATE KEY UPDATE - expires_at = GREATEST(groups_ancestors.expires_at, LEAST(groups_ancestors_join.expires_at, groups_groups.expires_at))` + FOR SHARE OF ` + relationsTable } else { recomputeQueries[1] += ` - FOR UPDATE OF ` + objectName + `_propagate + FOR UPDATE OF ` + objectName + `_propagate_processing FOR SHARE OF ` + objectName + `_ancestors_join FOR SHARE OF ` + relationsTable recomputeQueries = append(recomputeQueries, ` INSERT IGNORE INTO items_ancestors (ancestor_item_id, child_item_id) SELECT items_items.parent_item_id, items_items.child_item_id FROM items_items - JOIN items_propagate ON items_items.child_item_id = items_propagate.id - WHERE items_propagate.ancestors_computation_state = 'processing' - FOR UPDATE OF items_propagate + JOIN items_propagate_processing ON items_items.child_item_id = items_propagate_processing.id + FOR UPDATE OF items_propagate_processing FOR SHARE OF items_items`) // #nosec } @@ -144,16 +146,20 @@ func (s *DataStore) createNewAncestors(objectName, singleObjectName string) { /* defer func(i int) { mustNotBeError(recomputeAncestors[i].Close()) }(i) } - // Objects marked as 'processing' are now marked as 'done' + // Objects marked as processing are now marked as 'done' markAsDoneQuery := ` UPDATE ` + objectName + `_propagate - SET ancestors_computation_state = 'done' - WHERE ancestors_computation_state = 'processing'` // #nosec + JOIN ` + objectName + `_propagate_processing + ON ` + objectName + `_propagate.id = ` + objectName + `_propagate_processing.id + SET ancestors_computation_state = 'done'` // #nosec markAsDone, err := s.db.CommonDB().Prepare(markAsDoneQuery) mustNotBeError(err) defer func() { mustNotBeError(markAsDone.Close()) }() + dropTemporaryTable, err := s.db.CommonDB().Prepare(dropTemporaryTableQuery) + mustNotBeError(err) + defer func() { mustNotBeError(dropTemporaryTable.Close()) }() - for hasChanges { + for { _, err = markAsProcessing.Exec() mustNotBeError(err) for i := 0; i < len(recomputeAncestors); i++ { @@ -167,6 +173,14 @@ func (s *DataStore) createNewAncestors(objectName, singleObjectName string) { /* var rowsAffected int64 rowsAffected, err = result.RowsAffected() mustNotBeError(err) - hasChanges = rowsAffected > 0 + if rowsAffected == 0 { + break + } + + _, err = dropTemporaryTable.Exec() + mustNotBeError(err) + + _, err = createTemporaryTable.Exec() + mustNotBeError(err) } } diff --git a/app/database/group_group_store_test.go b/app/database/group_group_store_test.go index 34bb4197a..d9dbe1fa6 100644 --- a/app/database/group_group_store_test.go +++ b/app/database/group_group_store_test.go @@ -169,10 +169,14 @@ func TestGroupGroupStore_CreateRelationsWithoutChecking(t *testing.T) { func setMockExpectationsForCreateNewAncestors(mock sqlmock.Sqlmock) { mock.ExpectExec("").WillReturnResult(sqlmock.NewResult(-1, 0)) + mock.ExpectExec("").WillReturnResult(sqlmock.NewResult(-1, 0)) + mock.ExpectPrepare("") mock.ExpectPrepare("") mock.ExpectPrepare("") mock.ExpectPrepare("") mock.ExpectPrepare("") + mock.ExpectPrepare("") + mock.ExpectExec("").WillReturnResult(sqlmock.NewResult(-1, 0)) mock.ExpectExec("").WillReturnResult(sqlmock.NewResult(-1, 0)) mock.ExpectExec("").WillReturnResult(sqlmock.NewResult(-1, 0)) mock.ExpectExec("").WillReturnResult(sqlmock.NewResult(-1, 0)) diff --git a/db/migrations/2410292333_remove_unnecessary_enum_values_from_groups_propagate_ancestors_computation_state.sql b/db/migrations/2410292333_remove_unnecessary_enum_values_from_groups_propagate_ancestors_computation_state.sql new file mode 100644 index 000000000..4bbef7191 --- /dev/null +++ b/db/migrations/2410292333_remove_unnecessary_enum_values_from_groups_propagate_ancestors_computation_state.sql @@ -0,0 +1,5 @@ +-- +migrate Up +ALTER TABLE `groups_propagate` MODIFY COLUMN `ancestors_computation_state` ENUM('todo', 'done') NOT NULL; + +-- +migrate Down +ALTER TABLE `groups_propagate` MODIFY COLUMN `ancestors_computation_state` ENUM('todo', 'done', 'processing', '') NOT NULL; diff --git a/db/migrations/2410292340_remove_unnecessary_enum_values_from_items_propagate_ancestors_computation_state.sql b/db/migrations/2410292340_remove_unnecessary_enum_values_from_items_propagate_ancestors_computation_state.sql new file mode 100644 index 000000000..925459fd7 --- /dev/null +++ b/db/migrations/2410292340_remove_unnecessary_enum_values_from_items_propagate_ancestors_computation_state.sql @@ -0,0 +1,5 @@ +-- +migrate Up +ALTER TABLE `items_propagate` MODIFY COLUMN `ancestors_computation_state` ENUM('todo', 'done') NOT NULL; + +-- +migrate Down +ALTER TABLE `items_propagate` MODIFY COLUMN `ancestors_computation_state` ENUM('todo', 'done', 'processing', '') NOT NULL; diff --git a/db/migrations/2411061620_create_index_child_group_id_is_team_membership_parent_group_id_expires_at_on_groups_groups.sql b/db/migrations/2411061620_create_index_child_group_id_is_team_membership_parent_group_id_expires_at_on_groups_groups.sql new file mode 100644 index 000000000..4ed648238 --- /dev/null +++ b/db/migrations/2411061620_create_index_child_group_id_is_team_membership_parent_group_id_expires_at_on_groups_groups.sql @@ -0,0 +1,6 @@ +-- +migrate Up +ALTER TABLE `groups_groups` ADD INDEX `child_group_id_is_team_membership_parent_group_id_expires_at` + (`child_group_id`,`is_team_membership`,`parent_group_id`,`expires_at`); + +-- +migrate Down +ALTER TABLE `groups_groups` DROP INDEX `child_group_id_is_team_membership_parent_group_id_expires_at`; From 89e27c83dc771db6fd500086986645935618ae7c Mon Sep 17 00:00:00 2001 From: Dmitry Zenovich Date: Thu, 7 Nov 2024 09:49:55 +0300 Subject: [PATCH 09/12] sort all the data returned by currentUserDataExport & currentUserFullDataExport to make tests stable --- app/api/currentuser/get_full_dump.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/app/api/currentuser/get_full_dump.go b/app/api/currentuser/get_full_dump.go index d74d52214..bb94a9fc1 100644 --- a/app/api/currentuser/get_full_dump.go +++ b/app/api/currentuser/get_full_dump.go @@ -85,6 +85,7 @@ func (srv *Service) getDumpCommon(r *http.Request, w http.ResponseWriter, full b service.MustNotBeError(store.Sessions(). Where("user_id = ?", user.GroupID). Select(columns + ", '***' AS refresh_token"). + Order("session_id"). ScanAndHandleMaps(streamerFunc(w)).Error()) }) @@ -95,6 +96,7 @@ func (srv *Service) getDumpCommon(r *http.Request, w http.ResponseWriter, full b Joins("JOIN sessions ON sessions.session_id = access_tokens.session_id"). Where("sessions.user_id = ?", user.GroupID). Select(columns + ", '***' AS token"). + Order("session_id"). ScanAndHandleMaps(streamerFunc(w)).Error()) }) } @@ -121,6 +123,7 @@ func (srv *Service) getDumpCommon(r *http.Request, w http.ResponseWriter, full b writeComma(w) writeJSONObjectArrayElement("answers", w, func(writer io.Writer) { service.MustNotBeError(store.Answers().Where("author_id = ?", user.GroupID). + Order("id"). ScanAndHandleMaps(streamerFunc(w)).Error()) }) @@ -144,6 +147,7 @@ func (srv *Service) getDumpCommon(r *http.Request, w http.ResponseWriter, full b Where("child_group_id = ?", user.GroupID). Joins("JOIN `groups` ON `groups`.id = parent_group_id"). Select(columns + ", `groups`.name"). + Order("parent_group_id"). ScanAndHandleMaps(streamerFunc(w)).Error()) }) @@ -154,6 +158,7 @@ func (srv *Service) getDumpCommon(r *http.Request, w http.ResponseWriter, full b Where("manager_id = ?", user.GroupID). Joins("JOIN `groups` ON `groups`.id = group_id"). Select(columns + ", `groups`.name"). + Order("group_id"). ScanAndHandleMaps(streamerFunc(w)).Error()) }) @@ -176,6 +181,7 @@ func (srv *Service) getDumpCommon(r *http.Request, w http.ResponseWriter, full b Where("member_id = ?", user.GroupID). Joins("JOIN `groups` ON `groups`.id = group_id"). Select(columns + ", `groups`.name"). + Order("group_id"). ScanAndHandleMaps(streamerFunc(w)).Error()) }) } From 373af709871421083c138127834e21db2091b9ba Mon Sep 17 00:00:00 2001 From: Dmitry Zenovich Date: Thu, 7 Nov 2024 10:06:30 +0300 Subject: [PATCH 10/12] add is_team_membership column to groups_groups_active view --- ..._membership_to_groups_groups_active_view.sql | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) create mode 100644 db/migrations/2411070615_add_column_is_team_membership_to_groups_groups_active_view.sql diff --git a/db/migrations/2411070615_add_column_is_team_membership_to_groups_groups_active_view.sql b/db/migrations/2411070615_add_column_is_team_membership_to_groups_groups_active_view.sql new file mode 100644 index 000000000..dcdd001bc --- /dev/null +++ b/db/migrations/2411070615_add_column_is_team_membership_to_groups_groups_active_view.sql @@ -0,0 +1,17 @@ +-- +migrate Up +DROP VIEW groups_groups_active; +CREATE VIEW groups_groups_active AS SELECT * FROM groups_groups WHERE NOW() < expires_at; + +-- +migrate Down +DROP VIEW groups_groups_active; +CREATE VIEW `groups_groups_active` AS + SELECT `groups_groups`.`parent_group_id` AS `parent_group_id`, + `groups_groups`.`child_group_id` AS `child_group_id`, + `groups_groups`.`expires_at` AS `expires_at`, + `groups_groups`.`personal_info_view_approved_at` AS `personal_info_view_approved_at`, + `groups_groups`.`personal_info_view_approved` AS `personal_info_view_approved`, + `groups_groups`.`lock_membership_approved_at` AS `lock_membership_approved_at`, + `groups_groups`.`lock_membership_approved` AS `lock_membership_approved`, + `groups_groups`.`watch_approved_at` AS `watch_approved_at`, + `groups_groups`.`watch_approved` AS `watch_approved` + FROM `groups_groups` WHERE (NOW() < `groups_groups`.`expires_at`); From c8c74c8402637521a2b4507a6048866dfb948de3 Mon Sep 17 00:00:00 2001 From: Dmitry Zenovich Date: Thu, 7 Nov 2024 10:20:12 +0300 Subject: [PATCH 11/12] do not join the groups table to detect team membership, use the groups_groups.is_team_membership column instead --- app/api/answers/get_answer.go | 6 +++--- app/api/currentuser/get_full_dump.go | 4 ++-- app/api/groups/create_invitations.go | 5 ++--- .../groups/get_current_user_team_by_item.go | 3 ++- app/api/groups/get_user_progress.go | 5 ++--- app/api/groups/get_user_progress_csv.go | 5 ++--- app/api/items/get_entry_state.go | 10 +++++---- app/database/group_group_store.go | 18 ++++++++++++++++ app/database/group_group_store_test.go | 20 ++++++++++++++++++ app/database/group_store.go | 18 ---------------- app/database/group_store_test.go | 21 ------------------- app/database/users.go | 5 ++--- 12 files changed, 59 insertions(+), 61 deletions(-) diff --git a/app/api/answers/get_answer.go b/app/api/answers/get_answer.go index ee4324cf3..08e9b4089 100644 --- a/app/api/answers/get_answer.go +++ b/app/api/answers/get_answer.go @@ -63,8 +63,8 @@ func (srv *Service) getAnswer(rw http.ResponseWriter, httpReq *http.Request) ser userAndHisTeamsQuery := store.Raw("SELECT id FROM ? `teams` UNION ALL SELECT ?", store.ActiveGroupGroups(). WhereUserIsMember(user). - Joins("JOIN `groups` ON groups.id = groups_groups_active.parent_group_id AND groups.type='Team'"). - Select("groups.id").SubQuery(), + Where("groups_groups_active.is_team_membership = 1"). + Select("groups_groups_active.parent_group_id AS id").SubQuery(), user.GroupID) // a participant should have at least 'content' access to the answers.item_id @@ -77,7 +77,7 @@ func (srv *Service) getAnswer(rw http.ResponseWriter, httpReq *http.Request) ser Joins("JOIN `groups_ancestors_active` ON groups_ancestors_active.ancestor_group_id = permissions.group_id"). Joins("JOIN `groups_groups_active` ON groups_groups_active.parent_group_id = groups_ancestors_active.child_group_id"). Where("groups_groups_active.child_group_id = ?", user.GroupID). - Joins("JOIN `groups` ON groups.id = groups_groups_active.parent_group_id AND groups.type='Team'"). + Where("groups_groups_active.is_team_membership = 1"). WherePermissionIsAtLeast("view", "content"). Where("permissions.item_id = answers.item_id"). Select("1").Limit(1).SubQuery() diff --git a/app/api/currentuser/get_full_dump.go b/app/api/currentuser/get_full_dump.go index bb94a9fc1..e6822c796 100644 --- a/app/api/currentuser/get_full_dump.go +++ b/app/api/currentuser/get_full_dump.go @@ -272,7 +272,7 @@ func buildQueryForGettingAttemptsOrResults(store *database.DataStore, user *data Select(columns). Where("participant_id IN (?)", store.GroupGroups().WhereUserIsMember(user). - Select("`groups`.id"). - Joins("JOIN `groups` ON `groups`.id = groups_groups.parent_group_id AND `groups`.type = 'Team'"). + Where("groups_groups.is_team_membership = 1"). + Select("groups_groups.parent_group_id AS id"). QueryExpr())) } diff --git a/app/api/groups/create_invitations.go b/app/api/groups/create_invitations.go index 2d5e1e018..2015e7add 100644 --- a/app/api/groups/create_invitations.go +++ b/app/api/groups/create_invitations.go @@ -211,15 +211,14 @@ func getOtherTeamsMembers(store *database.DataStore, parentGroupID int64, groups var otherTeamsMembers []int64 service.MustNotBeError(store.ActiveGroupGroups().Where("child_group_id IN (?)", groupsToCheck). - Joins("JOIN `groups` ON groups.id = groups_groups_active.parent_group_id"). Joins("JOIN (?) AS teams_contests", contestsQuery. // all the team's attempts (not only active ones) Select(` root_item_id AS item_id, MAX(NOW() < attempts.allows_submissions_until AND attempts.ended_at IS NULL) AS is_active`).QueryExpr()). Joins("JOIN items ON items.id = teams_contests.item_id"). - Joins("JOIN attempts ON attempts.participant_id = groups.id AND attempts.root_item_id = items.id"). - Where("groups.type = 'Team'"). + Joins("JOIN attempts ON attempts.participant_id = groups_groups_active.parent_group_id AND attempts.root_item_id = items.id"). + Where("groups_groups_active.is_team_membership = 1"). Where("parent_group_id != ?", parentGroupID). Where(` (teams_contests.is_active AND NOW() < attempts.allows_submissions_until AND attempts.ended_at IS NULL) OR diff --git a/app/api/groups/get_current_user_team_by_item.go b/app/api/groups/get_current_user_team_by_item.go index 2d79bc936..d6d347e97 100644 --- a/app/api/groups/get_current_user_team_by_item.go +++ b/app/api/groups/get_current_user_team_by_item.go @@ -54,7 +54,8 @@ func (srv *Service) getCurrentUserTeamByItem(w http.ResponseWriter, r *http.Requ var teamID int64 user := srv.GetUser(r) - err = srv.GetStore(r).Groups().TeamGroupForTeamItemAndUser(itemID, user).PluckFirst("groups.id", &teamID).Error() + err = srv.GetStore(r).ActiveGroupGroups().TeamGroupForTeamItemAndUser(itemID, user). + PluckFirst("groups_groups_active.parent_group_id", &teamID).Error() if gorm.IsRecordNotFoundError(err) { return service.ErrNotFound(errors.New("no team for this item")) } diff --git a/app/api/groups/get_user_progress.go b/app/api/groups/get_user_progress.go index c5bb158b1..2e0b39178 100644 --- a/app/api/groups/get_user_progress.go +++ b/app/api/groups/get_user_progress.go @@ -206,10 +206,9 @@ func joinUserProgressResults(db *database.DB, userID interface{}) *database.DB { return db. Joins(` LEFT JOIN LATERAL ( - SELECT STRAIGHT_JOIN groups.id + SELECT STRAIGHT_JOIN groups_groups_active.parent_group_id AS id FROM groups_groups_active - JOIN `+"`groups`"+` ON groups.id = groups_groups_active.parent_group_id - WHERE groups.type = 'Team' AND groups_groups_active.child_group_id = ? + WHERE groups_groups_active.is_team_membership = 1 AND groups_groups_active.child_group_id = ? ) teams ON 1`, userID). Joins(` LEFT JOIN LATERAL ( diff --git a/app/api/groups/get_user_progress_csv.go b/app/api/groups/get_user_progress_csv.go index f05791480..f72476eda 100644 --- a/app/api/groups/get_user_progress_csv.go +++ b/app/api/groups/get_user_progress_csv.go @@ -266,10 +266,9 @@ func joinUserProgressResultsForCSV(db *database.DB, userID interface{}) *databas return db. Joins(` LEFT JOIN LATERAL ( - SELECT STRAIGHT_JOIN groups.id + SELECT STRAIGHT_JOIN groups_groups_active.parent_group_id AS id FROM groups_groups_active - JOIN `+"`groups`"+` ON groups.id = groups_groups_active.parent_group_id - WHERE groups.type = 'Team' AND groups_groups_active.child_group_id = ? + WHERE groups_groups_active.is_team_membership = 1 AND groups_groups_active.child_group_id = ? ) teams ON 1`, userID). Joins(` LEFT JOIN LATERAL ( diff --git a/app/api/items/get_entry_state.go b/app/api/items/get_entry_state.go index f3fe87b01..61f8d787e 100644 --- a/app/api/items/get_entry_state.go +++ b/app/api/items/get_entry_state.go @@ -306,11 +306,13 @@ func getEntryStateInfo(groupID, itemID int64, user *database.User, store *databa membersCount = int32(len(otherMembers)) participatingSomewhereElseQuery := store.ActiveGroupGroups().Where("groups_groups_active.parent_group_id = ?", groupID). - Joins("JOIN groups_groups_active AS all_teams_relations ON all_teams_relations.child_group_id = groups_groups_active.child_group_id"). - Joins("JOIN `groups` AS groups_to_check ON groups_to_check.id = all_teams_relations.parent_group_id AND groups_to_check.type = 'Team'"). + Joins(` + JOIN groups_groups_active AS all_teams_relations + ON all_teams_relations.child_group_id = groups_groups_active.child_group_id AND + all_teams_relations.is_team_membership = 1`). Joins("JOIN items ON items.id = ?", itemID). - Joins("JOIN attempts ON attempts.participant_id = groups_to_check.id AND attempts.root_item_id = items.id"). - Where("groups_to_check.id != groups_groups_active.parent_group_id"). // except for this team + Joins("JOIN attempts ON attempts.participant_id = all_teams_relations.parent_group_id AND attempts.root_item_id = items.id"). + Where("all_teams_relations.parent_group_id != groups_groups_active.parent_group_id"). // except for this team Group("groups_groups_active.child_group_id"). Having("MAX(NOW() < attempts.allows_submissions_until) OR NOT MAX(items.allows_multiple_attempts)") if lock { diff --git a/app/database/group_group_store.go b/app/database/group_group_store.go index e23f402c7..139691e39 100644 --- a/app/database/group_group_store.go +++ b/app/database/group_group_store.go @@ -198,3 +198,21 @@ func (s *GroupGroupStore) CreateNewAncestors() (err error) { s.createNewAncestors() return nil } + +// TeamGroupForTeamItemAndUser returns a composable query for getting a team +// +// (as groups_groups_active.parent_group_id) that +// 1. the given user is a member of +// 2. has an unexpired attempt with root_item_id = `itemID`. +// +// If more than one team is found (which should be impossible), the one with the smallest `groups.id` is returned. +func (s *GroupGroupStore) TeamGroupForTeamItemAndUser(itemID int64, user *User) *DB { + return s. + Where("groups_groups_active.is_team_membership = 1"). + Where("groups_groups_active.child_group_id = ?", user.GroupID). + Joins(` + JOIN attempts ON attempts.participant_id = groups_groups_active.parent_group_id AND + attempts.root_item_id = ? AND NOW() < attempts.allows_submissions_until`, itemID). + Order("groups_groups_active.parent_group_id"). + Limit(1) // The current API doesn't allow users to join multiple teams working on the same item +} diff --git a/app/database/group_group_store_test.go b/app/database/group_group_store_test.go index d9dbe1fa6..7a9721ab9 100644 --- a/app/database/group_group_store_test.go +++ b/app/database/group_group_store_test.go @@ -257,3 +257,23 @@ func TestGroupGroupStore_CreateNewAncestors_HandlesErrorOfCreateNewAncestors(t * assert.NoError(t, dbMock.ExpectationsWereMet()) } + +func TestGroupGroupStore_TeamGroupForTeamItemAndUser(t *testing.T) { + db, mock := NewDBMock() + defer func() { _ = db.Close() }() + + mockUser := &User{GroupID: 2, DefaultLanguage: "fr"} + + mock.ExpectQuery(regexp.QuoteMeta("SELECT `groups_groups_active`.* FROM `groups_groups_active` "+ + "JOIN attempts ON attempts.participant_id = groups_groups_active.parent_group_id AND attempts.root_item_id = ? AND "+ + "NOW() < attempts.allows_submissions_until "+ + "WHERE (groups_groups_active.is_team_membership = 1) AND (groups_groups_active.child_group_id = ?) "+ + "ORDER BY groups_groups_active.parent_group_id LIMIT 1")). + WithArgs(1234, 2). + WillReturnRows(mock.NewRows([]string{"id"})) + + var result []interface{} + err := NewDataStore(db).ActiveGroupGroups().TeamGroupForTeamItemAndUser(1234, mockUser).Scan(&result).Error() + assert.NoError(t, err) + assert.NoError(t, mock.ExpectationsWereMet()) +} diff --git a/app/database/group_store.go b/app/database/group_store.go index c864314e4..731117265 100644 --- a/app/database/group_store.go +++ b/app/database/group_store.go @@ -28,24 +28,6 @@ func (s *GroupStore) ManagedBy(user *User) *DB { user_ancestors.child_group_id = ?`, user.GroupID) } -// TeamGroupForTeamItemAndUser returns a composable query for getting a team that -// 1. the given user is a member of -// 2. has an unexpired attempt with root_item_id = `itemID`. -// -// If more than one team is found (which should be impossible), the one with the smallest `groups.id` is returned. -func (s *GroupStore) TeamGroupForTeamItemAndUser(itemID int64, user *User) *DB { - return s. - Joins(`JOIN groups_groups_active - ON groups_groups_active.parent_group_id = groups.id AND - groups_groups_active.child_group_id = ?`, user.GroupID). - Joins(` - JOIN attempts ON attempts.participant_id = groups.id AND - attempts.root_item_id = ? AND NOW() < attempts.allows_submissions_until`, itemID). - Where("groups.type = 'Team'"). - Order("groups.id"). - Limit(1) // The current API doesn't allow users to join multiple teams working on the same item -} - // TeamGroupForUser returns a composable query for getting team group of the given user with given id. func (s *GroupStore) TeamGroupForUser(teamGroupID int64, user *User) *DB { return s. diff --git a/app/database/group_store_test.go b/app/database/group_store_test.go index 80abdd7b9..c8f2e8df5 100644 --- a/app/database/group_store_test.go +++ b/app/database/group_store_test.go @@ -29,27 +29,6 @@ func TestGroupStore_ManagedBy(t *testing.T) { assert.NoError(t, mock.ExpectationsWereMet()) } -func TestGroupStore_TeamGroupForTeamItemAndUser(t *testing.T) { - db, mock := NewDBMock() - defer func() { _ = db.Close() }() - - mockUser := &User{GroupID: 2, DefaultLanguage: "fr"} - - mock.ExpectQuery(regexp.QuoteMeta("SELECT `groups`.* FROM `groups` "+ - "JOIN groups_groups_active ON groups_groups_active.parent_group_id = groups.id AND "+ - "groups_groups_active.child_group_id = ? "+ - "JOIN attempts ON attempts.participant_id = groups.id AND attempts.root_item_id = ? AND "+ - "NOW() < attempts.allows_submissions_until "+ - "WHERE (groups.type = 'Team') ORDER BY `groups`.`id` LIMIT 1")). - WithArgs(2, 1234). - WillReturnRows(mock.NewRows([]string{"id"})) - - var result []interface{} - err := NewDataStore(db).Groups().TeamGroupForTeamItemAndUser(1234, mockUser).Scan(&result).Error() - assert.NoError(t, err) - assert.NoError(t, mock.ExpectationsWereMet()) -} - func TestGroupStore_TeamGroupForUser(t *testing.T) { db, mock := NewDBMock() defer func() { _ = db.Close() }() diff --git a/app/database/users.go b/app/database/users.go index 316dbd15e..921e7822f 100644 --- a/app/database/users.go +++ b/app/database/users.go @@ -20,12 +20,11 @@ func (s *DataStore) CheckIfTeamParticipationsConflictWithExistingUserMemberships } query := s.ActiveGroupGroups().Where("child_group_id = ?", userGroupID). - Joins("JOIN `groups` ON groups.id = groups_groups_active.parent_group_id"). + Where("is_team_membership = 1"). Joins("JOIN (?) AS teams_contests", contestsQuery. // all the team's attempts (not only active ones) Select("root_item_id AS item_id, MAX(NOW() < attempts.allows_submissions_until) AS is_active").QueryExpr()). Joins("JOIN items ON items.id = teams_contests.item_id"). - Joins("JOIN attempts ON attempts.participant_id = groups.id AND attempts.root_item_id = items.id"). - Where("groups.type = 'Team'"). + Joins("JOIN attempts ON attempts.participant_id = parent_group_id AND attempts.root_item_id = items.id"). Where("parent_group_id != ?", teamID). Where("(teams_contests.is_active AND NOW() < attempts.allows_submissions_until) OR NOT items.allows_multiple_attempts") if withLock { From add7e167983f16aa7c23df875c64c0475fa7361a Mon Sep 17 00:00:00 2001 From: Dmitry Zenovich Date: Thu, 7 Nov 2024 12:00:16 +0300 Subject: [PATCH 12/12] drop unnecessary trigger after_insert_items as there is no reason for marking a newly created item as "todo" for items propagation, only call the items propagation in itemCreate if new items_items rows are created --- app/api/items/create_item.go | 4 ++-- app/database/ancestors.go | 1 - .../item_item_store_ancestors_integration_test.go | 1 - .../2411070848_drop_trigger_after_insert_items.sql | 8 ++++++++ 4 files changed, 10 insertions(+), 4 deletions(-) create mode 100644 db/migrations/2411070848_drop_trigger_after_insert_items.sql diff --git a/app/api/items/create_item.go b/app/api/items/create_item.go index 3948483c8..d9293c36b 100644 --- a/app/api/items/create_item.go +++ b/app/api/items/create_item.go @@ -264,8 +264,6 @@ func validateAndInsertItem(srv *Service, r *http.Request) (itemID int64, apiErro setNewItemAsRootActivityOrSkill(store, formData, &input, itemID) - service.MustNotBeError(store.ItemItems().CreateNewAncestors()) - return nil }) if err == nil { @@ -617,6 +615,8 @@ func (srv *Service) insertItem(store *database.DataStore, user *database.User, f parentChildSpec = append(parentChildSpec, constructItemsItemsForChildren(newItemRequest.Children, itemID)...) insertItemItems(store, parentChildSpec) + + service.MustNotBeError(store.ItemItems().CreateNewAncestors()) } return itemID, service.NoError diff --git a/app/database/ancestors.go b/app/database/ancestors.go index 381f8b5f1..e0ec971fc 100644 --- a/app/database/ancestors.go +++ b/app/database/ancestors.go @@ -13,7 +13,6 @@ const groups = "groups" // // Note: rows in *_propagate tables with `ancestors_computation_state`="todo" // are added in the database in SQL triggers: -// - after_insert_items // - after_update_groups_groups // - before_insert_items_items/groups_groups // - before_delete_items_items/groups_groups. diff --git a/app/database/item_item_store_ancestors_integration_test.go b/app/database/item_item_store_ancestors_integration_test.go index 884eca928..87c25cb8f 100644 --- a/app/database/item_item_store_ancestors_integration_test.go +++ b/app/database/item_item_store_ancestors_integration_test.go @@ -51,7 +51,6 @@ func TestItemItemStore_CreateNewAncestors_Concurrent(t *testing.T) { var propagateResult []itemPropagateResultRow assert.NoError(t, itemItemStore.Table("items_propagate").Order("id").Scan(&propagateResult).Error()) assert.Equal(t, []itemPropagateResultRow{ - {ID: 1, AncestorsComputationState: "done"}, {ID: 2, AncestorsComputationState: "done"}, {ID: 3, AncestorsComputationState: "done"}, {ID: 4, AncestorsComputationState: "done"}, diff --git a/db/migrations/2411070848_drop_trigger_after_insert_items.sql b/db/migrations/2411070848_drop_trigger_after_insert_items.sql new file mode 100644 index 000000000..5d032925b --- /dev/null +++ b/db/migrations/2411070848_drop_trigger_after_insert_items.sql @@ -0,0 +1,8 @@ +-- +migrate Up +DROP TRIGGER `after_insert_items`; + +-- +migrate Down +DROP TRIGGER IF EXISTS `after_insert_items`; +-- +migrate StatementBegin +CREATE TRIGGER `after_insert_items` AFTER INSERT ON `items` FOR EACH ROW BEGIN INSERT IGNORE INTO `items_propagate` (`id`, `ancestors_computation_state`) VALUES (NEW.`id`, 'todo') ; END +-- +migrate StatementEnd