From ca6acddf9a8ae473bdbc32ab9484d1d0680eeb40 Mon Sep 17 00:00:00 2001 From: Rico Hermans Date: Thu, 9 May 2024 20:03:54 +0200 Subject: [PATCH 1/7] =?UTF-8?q?=F0=9F=AA=B2=20Fix=20running=20programs=20w?= =?UTF-8?q?hen=20logged=20in?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When logged in, we automatically save a program as the user runs it. In a recent change, we update the program counter only when a program is modified from the example code. But we wrote the entire 'program' record back to the database in an `update()` call, which includes the `id` field; Dynamo does not allow including the `id` field in an `update()` call. Instead, only update the `is_modified` field, and update the Dynamo abstraction layer to check for this error condition. --- website/dynamo.py | 4 ++++ website/programs.py | 5 +++-- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/website/dynamo.py b/website/dynamo.py index 3452b603418..c37b02f8b90 100644 --- a/website/dynamo.py +++ b/website/dynamo.py @@ -369,6 +369,10 @@ def update(self, key, updates): self._validate_indexable_fields(updates, True) self._validate_key(key) + updating_keys = set(updates.keys()) & set(self.key_schema.key_names) + if updating_keys: + raise RuntimeError(f'update() may not include a key field in the \'updates\' field ({updating_keys} may not be part of {updates}; did you accidentally pass an entire record to update()?)') + return self.storage.update(self.table_name, key, updates) @querylog.timed_as("db_del") diff --git a/website/programs.py b/website/programs.py index ecdb46ddb76..ecce3da14f2 100644 --- a/website/programs.py +++ b/website/programs.py @@ -77,6 +77,7 @@ def store_user_program(self, updates['public'] = 1 if set_public else 0 if program_id: + # Updates an existing program # FIXME: This should turn into a conditional update current_prog = self.db.program_by_id(program_id) if not current_prog: @@ -86,6 +87,7 @@ def store_user_program(self, program = self.db.update_program(program_id, updates) else: + # Creates a new program updates['id'] = uuid.uuid4().hex program = self.db.store_program(updates) @@ -101,8 +103,7 @@ def store_user_program(self, # and if it was already modified and now is so again, count should not increase. if is_modified and not program.get('is_modified'): self.db.increase_user_program_count(user["username"]) - program['is_modified'] = is_modified - program = self.db.update_program(program['id'], program) + self.db.update_program(program['id'], { 'is_modified': True }) querylog.log_value(program_id=program['id'], adventure_name=adventure_name, error=error, code_lines=len(code.split('\n'))) From 07c751f01f5749fa588e1462bfff3041e25aa64a Mon Sep 17 00:00:00 2001 From: Rico Hermans Date: Thu, 9 May 2024 20:16:17 +0200 Subject: [PATCH 2/7] Return the latest version of 'program' --- website/programs.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/website/programs.py b/website/programs.py index ecce3da14f2..2bc64a4b97d 100644 --- a/website/programs.py +++ b/website/programs.py @@ -103,7 +103,7 @@ def store_user_program(self, # and if it was already modified and now is so again, count should not increase. if is_modified and not program.get('is_modified'): self.db.increase_user_program_count(user["username"]) - self.db.update_program(program['id'], { 'is_modified': True }) + program = self.db.update_program(program['id'], { 'is_modified': True }) querylog.log_value(program_id=program['id'], adventure_name=adventure_name, error=error, code_lines=len(code.split('\n'))) From 0a1f71e0ae149d4b53ca0703d140d750afae967d Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Thu, 9 May 2024 18:17:34 +0000 Subject: [PATCH 3/7] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- website/dynamo.py | 3 ++- website/programs.py | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/website/dynamo.py b/website/dynamo.py index c37b02f8b90..1dfeee05b5b 100644 --- a/website/dynamo.py +++ b/website/dynamo.py @@ -371,7 +371,8 @@ def update(self, key, updates): updating_keys = set(updates.keys()) & set(self.key_schema.key_names) if updating_keys: - raise RuntimeError(f'update() may not include a key field in the \'updates\' field ({updating_keys} may not be part of {updates}; did you accidentally pass an entire record to update()?)') + raise RuntimeError(f'update() may not include a key field in the \'updates\' field ({updating_keys} may not be part of { + updates}; did you accidentally pass an entire record to update()?)') return self.storage.update(self.table_name, key, updates) diff --git a/website/programs.py b/website/programs.py index 2bc64a4b97d..c0a70cac11a 100644 --- a/website/programs.py +++ b/website/programs.py @@ -103,7 +103,7 @@ def store_user_program(self, # and if it was already modified and now is so again, count should not increase. if is_modified and not program.get('is_modified'): self.db.increase_user_program_count(user["username"]) - program = self.db.update_program(program['id'], { 'is_modified': True }) + program = self.db.update_program(program['id'], {'is_modified': True}) querylog.log_value(program_id=program['id'], adventure_name=adventure_name, error=error, code_lines=len(code.split('\n'))) From 7f695a72fb2a5cec79f444dee90a55d554632e07 Mon Sep 17 00:00:00 2001 From: Rico Hermans Date: Thu, 9 May 2024 20:22:09 +0200 Subject: [PATCH 4/7] Try and avoid linter complaints --- website/dynamo.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/website/dynamo.py b/website/dynamo.py index 1dfeee05b5b..09c971e1b7f 100644 --- a/website/dynamo.py +++ b/website/dynamo.py @@ -371,8 +371,10 @@ def update(self, key, updates): updating_keys = set(updates.keys()) & set(self.key_schema.key_names) if updating_keys: - raise RuntimeError(f'update() may not include a key field in the \'updates\' field ({updating_keys} may not be part of { - updates}; did you accidentally pass an entire record to update()?)') + raise RuntimeError(' '.join([ + 'update() may not include a key field in the \'updates\' field', + f'({updating_keys} may not be part of {updates};', + 'did you accidentally pass an entire record to update()?)'])) return self.storage.update(self.table_name, key, updates) From 876268ac043cce37fe574eff4b6d0a96cb4d85e4 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Fri, 10 May 2024 10:35:09 +0200 Subject: [PATCH 5/7] Another case of putting an entire record into `update()` --- website/database.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/website/database.py b/website/database.py index 4281be9bac7..411720f9935 100644 --- a/website/database.py +++ b/website/database.py @@ -899,8 +899,7 @@ def set_favourite_program(self, username, program_id, set_favourite): # We can only set a favourite program is there is already a public profile data = PUBLIC_PROFILES.get({"username": username}) if data: - data["favourite_program"] = program_id if set_favourite else '' - self.update_public_profile(username, data) + self.update_public_profile(username, { "favourite_program": program_id if set_favourite else '' }) return True return False From 83b50e8c2ad715a728a51dac4f2c69d60aab5f36 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Fri, 10 May 2024 08:36:32 +0000 Subject: [PATCH 6/7] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- website/database.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/website/database.py b/website/database.py index 411720f9935..4339dadc04e 100644 --- a/website/database.py +++ b/website/database.py @@ -899,7 +899,7 @@ def set_favourite_program(self, username, program_id, set_favourite): # We can only set a favourite program is there is already a public profile data = PUBLIC_PROFILES.get({"username": username}) if data: - self.update_public_profile(username, { "favourite_program": program_id if set_favourite else '' }) + self.update_public_profile(username, {"favourite_program": program_id if set_favourite else ''}) return True return False From c440375b62c9ad3b51702a0d3e4630bf3ef8593c Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Fri, 10 May 2024 10:50:38 +0200 Subject: [PATCH 7/7] Make Cypress test slightly more reliable --- tests/cypress/e2e/public_programs/programs_page.cy.js | 4 ++-- tests/cypress/e2e/tools/profile/profile.js | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/cypress/e2e/public_programs/programs_page.cy.js b/tests/cypress/e2e/public_programs/programs_page.cy.js index ba53635d5d1..10461f94fd4 100644 --- a/tests/cypress/e2e/public_programs/programs_page.cy.js +++ b/tests/cypress/e2e/public_programs/programs_page.cy.js @@ -9,7 +9,7 @@ describe("General tests for my programs page (with both custom teacher and built const adventure = 'story' beforeEach(() => { loginForTeacher(); - }) + }) it("create adventure, run its code, and see it in my programs", () => { createAdventure(programName); @@ -31,7 +31,7 @@ describe("General tests for my programs page (with both custom teacher and built // make sure to navigate to the wanted program tab. cy.get(`[data-cy="${adventure}"]`) .click(); - // Paste example code + // Paste example code cy.get(`[data-cy="paste-example-code-${adventure}"]`).click(); cy.get('#runit').click(); cy.wait(500); diff --git a/tests/cypress/e2e/tools/profile/profile.js b/tests/cypress/e2e/tools/profile/profile.js index ad08b6afe27..9fff3b0d5cf 100644 --- a/tests/cypress/e2e/tools/profile/profile.js +++ b/tests/cypress/e2e/tools/profile/profile.js @@ -4,6 +4,6 @@ export function makeProfilePublic() { goToProfilePage(); cy.get('#public_profile_button').click(); cy.get('#personal_text').type('updating profile to be public'); - cy.get('#agree_terms').click(); + cy.get('#agree_terms').check(); // May start out checked, in which case 'click()' would undo the check! cy.get('#submit_public_profile').click(); } \ No newline at end of file