From 22b34e608895f7b495efeb51e49b82635ba5235f Mon Sep 17 00:00:00 2001 From: Florian Aucomte <33633200+faucomte97@users.noreply.github.com> Date: Thu, 6 Oct 2022 13:41:06 +0200 Subject: [PATCH] feat: Admin power for custom levels (#1371) * feat: Admin power for custom levels * fix command * Install common too * Revert ci * Update and fix test --- Pipfile | 1 + Pipfile.lock | 78 ++++++------ game/permissions.py | 54 +++------ game/static/game/css/level_share.css | 11 +- game/static/game/js/sharing.js | 32 +++-- game/templates/game/level_editor.html | 1 + game/templates/game/level_selection.html | 49 ++++++-- game/tests/test_level_editor.py | 67 +++++++---- game/tests/test_level_moderation.py | 26 +--- game/tests/test_level_selection.py | 147 +++++++++++++++++++++-- game/tests/utils/teacher.py | 3 +- game/views/level_editor.py | 146 ++++++++++------------ game/views/level_selection.py | 90 ++++++++------ 13 files changed, 432 insertions(+), 273 deletions(-) diff --git a/Pipfile b/Pipfile index 4ffefa349..2b08c15b2 100644 --- a/Pipfile +++ b/Pipfile @@ -15,6 +15,7 @@ pytest = "==7.*" pytest-xdist = "*" pytest-order = "*" codeforlife-portal = "*" +importlib-metadata = "<5" # Using version 5 causes an issue when trying to run pytest. Not sure why, linked to: https://stackoverflow.com/questions/73929564/entrypoints-object-has-no-attribute-get-digital-ocean [requires] python_version = "3.7" diff --git a/Pipfile.lock b/Pipfile.lock index df42e3083..e506341af 100644 --- a/Pipfile.lock +++ b/Pipfile.lock @@ -1,7 +1,7 @@ { "_meta": { "hash": { - "sha256": "244cb759d0039840580944138778fa38761616fa34e190813da48d58936af587" + "sha256": "f42507aafbc4e1a457822fa5d4bbca2998fbb241fd6dcddf14202a1ef822a114" }, "pipfile-spec": 6, "requires": { @@ -25,10 +25,10 @@ }, "cfl-common": { "hashes": [ - "sha256:d70aa4c9c3acd95f5c344cecd23b722fd206cf7a2d7ed088658d4e041b48cd5e", - "sha256:eee38784de70f3d040b127d7155ecd8b9ec269bac7b08afd52749165f54a1b4e" + "sha256:315a4ee833a45af4acd1b9ebbfe66ca01733c8dbbbb0c4fbf89efc22a6af5d7b", + "sha256:519493fb7b17dcf72d8343549b5050f448f246cff4bf25cf6f87271fea81d175" ], - "version": "==6.20.0" + "version": "==6.21.0" }, "django": { "hashes": [ @@ -53,10 +53,10 @@ }, "django-formtools": { "hashes": [ - "sha256:4699937e19ee041d803943714fe0c1c7ad4cab802600eb64bbf4cdd0a1bfe7d9", - "sha256:9663b6eca64777b68d6d4142efad8597fe9a685924673b25aa8a1dcff4db00c3" + "sha256:deb932be55b1d9419e37dc4d65dfbfeb8d307b71c8c11fd52f159aba5fc0deed", + "sha256:f5f32f62ec8192cd1bc55bd929ca7dff5a5f2addf9027db95a5906ecfaa64836" ], - "version": "==2.3" + "version": "==2.4" }, "django-js-reverse": { "hashes": [ @@ -137,10 +137,10 @@ }, "pytz": { "hashes": [ - "sha256:220f481bdafa09c3955dfbdddb7b57780e9a94f5127e35456a48589b9e0c0197", - "sha256:cea221417204f2d1a2aa03ddae3e867921971d0d76f14d87abb4414415bbdcf5" + "sha256:2c0784747071402c6e99f0bafdb7da0fa22645f06554c7ae06bf6358897e9c91", + "sha256:48ce799d83b6f8aab2020e369b627446696619e79645419610b9facd909b3174" ], - "version": "==2022.2.1" + "version": "==2022.4" }, "qrcode": { "hashes": [ @@ -161,17 +161,16 @@ }, "sqlparse": { "hashes": [ - "sha256:0c00730c74263a94e5a9919ade150dfc3b19c574389985446148402998287dae", - "sha256:48719e356bb8b42991bdbb1e8b83223757b93789c00910a616a071910ca4a64d" + "sha256:0323c0ec29cd52bceabc1b4d9d579e311f3e4961b98d174201d5622a23b85e34", + "sha256:69ca804846bb114d2ec380e4360a8a340db83f0ccf3afceeb1404df028f57268" ], - "version": "==0.4.2" + "version": "==0.4.3" }, "typing-extensions": { "hashes": [ "sha256:25642c956049920a5aa49edcdd6ab1e06d7e5d467fc00e0506c44ac86fbfca02", "sha256:e6d2677a32f47fc7eb2795db1dd15c1f34eff616bcaf2cfb5e997f854fa1c4a6" ], - "markers": "python_version < '3.8'", "version": "==4.3.0" } }, @@ -206,17 +205,17 @@ }, "certifi": { "hashes": [ - "sha256:43dadad18a7f168740e66944e4fa82c6611848ff9056ad910f8f7a3e46ab89e0", - "sha256:cffdcd380919da6137f76633531a5817e3a9f268575c128249fb637e4f9e73fb" + "sha256:0d9c601124e5a6ba9712dbc60d9c53c21e34f5f641fe83002317394311bdce14", + "sha256:90c1a32f1d68f940488354e36370f6cca89f0f106db09518524c88d6ed83f382" ], - "version": "==2022.6.15.1" + "version": "==2022.9.24" }, "cfl-common": { "hashes": [ - "sha256:d70aa4c9c3acd95f5c344cecd23b722fd206cf7a2d7ed088658d4e041b48cd5e", - "sha256:eee38784de70f3d040b127d7155ecd8b9ec269bac7b08afd52749165f54a1b4e" + "sha256:315a4ee833a45af4acd1b9ebbfe66ca01733c8dbbbb0c4fbf89efc22a6af5d7b", + "sha256:519493fb7b17dcf72d8343549b5050f448f246cff4bf25cf6f87271fea81d175" ], - "version": "==6.20.0" + "version": "==6.21.0" }, "chardet": { "hashes": [ @@ -227,11 +226,11 @@ }, "codeforlife-portal": { "hashes": [ - "sha256:7a368970f05c422acba7d706ec94f1136bbd0d611bfd58c21906b938ae38907f", - "sha256:d8c1048192ba3b95a9bec00d8b099f909f26825818a0dd3736c7861fce74deb0" + "sha256:df977edcbbdd376860f2d725d53707a385222c05c225477548554997f60b04aa", + "sha256:ff478cd03d48f2b44e6dc3feabf1b65fb97c931a4df7b1adf09bdd02c9007b7e" ], "index": "pypi", - "version": "==6.20.0" + "version": "==6.21.0" }, "django": { "hashes": [ @@ -263,10 +262,10 @@ }, "django-formtools": { "hashes": [ - "sha256:4699937e19ee041d803943714fe0c1c7ad4cab802600eb64bbf4cdd0a1bfe7d9", - "sha256:9663b6eca64777b68d6d4142efad8597fe9a685924673b25aa8a1dcff4db00c3" + "sha256:deb932be55b1d9419e37dc4d65dfbfeb8d307b71c8c11fd52f159aba5fc0deed", + "sha256:f5f32f62ec8192cd1bc55bd929ca7dff5a5f2addf9027db95a5906ecfaa64836" ], - "version": "==2.3" + "version": "==2.4" }, "django-js-reverse": { "hashes": [ @@ -387,10 +386,10 @@ }, "google-auth": { "hashes": [ - "sha256:be62acaae38d0049c21ca90f27a23847245c9f161ff54ede13af2cb6afecbac9", - "sha256:ed65ecf9f681832298e29328e1ef0a3676e3732b2e56f41532d45f70a22de0fb" + "sha256:98f601773978c969e1769f97265e732a81a8e598da3263895023958d456ee625", + "sha256:f12d86502ce0f2c0174e2e70ecc8d36c69593817e67e1d9c5e34489120422e4b" ], - "version": "==2.11.0" + "version": "==2.12.0" }, "greenlet": { "hashes": [ @@ -467,11 +466,12 @@ }, "importlib-metadata": { "hashes": [ - "sha256:637245b8bab2b6502fcbc752cc4b7a6f6243bb02b31c5c26156ad103d3d45670", - "sha256:7401a975809ea1fdc658c3aa4f78cc2195a0e019c5cbc4c06122884e9ae80c23" + "sha256:8a8a81bcf996e74fee46f0d16bd3eaa382a7eb20fd82445c3ad11f4090334116", + "sha256:dd0173e8f150d6815e098fd354f6414b0f079af4644ddfe90c71e2fc6174346d" ], + "index": "pypi", "markers": "python_version < '3.8'", - "version": "==4.12.0" + "version": "==4.13.0" }, "iniconfig": { "hashes": [ @@ -663,6 +663,7 @@ "sha256:4f365fec2dff9c1162f834d9f18af1ba13062db0c708bf7b946f8a5c76180c39" ], "index": "pypi", + "markers": "python_version < '3.10'", "version": "==7.1.3" }, "pytest-django": { @@ -705,10 +706,10 @@ }, "pytz": { "hashes": [ - "sha256:220f481bdafa09c3955dfbdddb7b57780e9a94f5127e35456a48589b9e0c0197", - "sha256:cea221417204f2d1a2aa03ddae3e867921971d0d76f14d87abb4414415bbdcf5" + "sha256:2c0784747071402c6e99f0bafdb7da0fa22645f06554c7ae06bf6358897e9c91", + "sha256:48ce799d83b6f8aab2020e369b627446696619e79645419610b9facd909b3174" ], - "version": "==2022.2.1" + "version": "==2022.4" }, "pyyaml": { "hashes": [ @@ -828,10 +829,10 @@ }, "sqlparse": { "hashes": [ - "sha256:0c00730c74263a94e5a9919ade150dfc3b19c574389985446148402998287dae", - "sha256:48719e356bb8b42991bdbb1e8b83223757b93789c00910a616a071910ca4a64d" + "sha256:0323c0ec29cd52bceabc1b4d9d579e311f3e4961b98d174201d5622a23b85e34", + "sha256:69ca804846bb114d2ec380e4360a8a340db83f0ccf3afceeb1404df028f57268" ], - "version": "==0.4.2" + "version": "==0.4.3" }, "tomli": { "hashes": [ @@ -845,7 +846,6 @@ "sha256:25642c956049920a5aa49edcdd6ab1e06d7e5d467fc00e0506c44ac86fbfca02", "sha256:e6d2677a32f47fc7eb2795db1dd15c1f34eff616bcaf2cfb5e997f854fa1c4a6" ], - "markers": "python_version < '3.8'", "version": "==4.3.0" }, "urllib3": { diff --git a/game/permissions.py b/game/permissions.py index 7966044c4..42e81e621 100644 --- a/game/permissions.py +++ b/game/permissions.py @@ -11,9 +11,7 @@ def _get_userprofile_school(userprofile): elif hasattr(userprofile, "student"): return userprofile.student.class_field.teacher.school else: - LOGGER.error( - f"Userprofile ID {userprofile.id} has no teacher or student attribute" - ) + LOGGER.error(f"Userprofile ID {userprofile.id} has no teacher or student attribute") return None @@ -37,6 +35,7 @@ def can_save_workspace(user, workspace): def can_delete_workspace(user, workspace): return not user.is_anonymous and workspace.owner == user.userprofile + ##################### # Level permissions # ##################### @@ -50,10 +49,7 @@ def can_play_or_delete_level(user, level): # If the teacher is an admin, they can play any student's level in the school, otherwise only student levels # from their own classes if user.userprofile.teacher.is_admin and hasattr(level.owner, "student"): - return ( - user.userprofile.teacher.school - == level.owner.student.class_field.teacher.school - ) + return user.userprofile.teacher.school == level.owner.student.class_field.teacher.school else: return user.userprofile.teacher.teaches(level.owner) @@ -87,9 +83,7 @@ def can_load_level(user, level): owner_school = _get_userprofile_school(level.owner) return user_school is not None and user_school == owner_school else: - return hasattr( - user.userprofile, "teacher" - ) and user.userprofile.teacher.teaches(level.owner) + return hasattr(user.userprofile, "teacher") and user.userprofile.teacher.teaches(level.owner) def can_save_level(user, level): @@ -125,16 +119,10 @@ def has_permission(self, request, view): def has_object_permission(self, request, view, obj): if request.user.is_anonymous: return False - elif ( - hasattr(request.user.userprofile, "student") - and request.user.userprofile.student.is_independent() - ): + elif hasattr(request.user.userprofile, "student") and request.user.userprofile.student.is_independent(): return False # if the user is a teacher and the level is shared with them - elif ( - hasattr(request.user.userprofile, "teacher") - and obj.shared_with.filter(id=request.user.id).exists() - ): + elif hasattr(request.user.userprofile, "teacher") and obj.shared_with.filter(id=request.user.id).exists(): return True else: return obj.owner == request.user.userprofile @@ -142,15 +130,14 @@ def has_object_permission(self, request, view, obj): class CanShareLevelWith(permissions.BasePermission): """ - Used to verify that an the user who is requesting to share their level is - authorised to share the level with a specific recipient. + Used to verify that the user who is requesting to share their level is authorised to share the level with a specific + recipient. The user is authorised if: - neither they nor the recipient are anonymous, - neither they nor the recipient are independent students, - - they are a student and the recipient is a student in the same class, or their - teacher - - they are a teacher and the recipient is a teacher in the same school, or their - student + - they are a student and the recipient is a student in the same class, or their teacher + - they are a teacher and the recipient is a teacher in the same school, or their student + - they are an admin teacher and the recipient is a student in the same school """ def has_permission(self, request, view): @@ -174,25 +161,18 @@ def can_share_level_with(self, recipient, sharer): and not (recipient_profile.student.is_independent()) ): # Are they in the same class? - return ( - sharer_profile.student.class_field - == recipient_profile.student.class_field - ) - elif hasattr(sharer_profile, "teacher") and sharer_profile.teacher.teaches( - recipient_profile - ): + return sharer_profile.student.class_field == recipient_profile.student.class_field + elif hasattr(sharer_profile, "teacher") and sharer_profile.teacher.teaches(recipient_profile): # Is the recipient taught by the sharer? return True - elif hasattr( - recipient_profile, "teacher" - ) and recipient_profile.teacher.teaches(sharer_profile): + elif hasattr(recipient_profile, "teacher") and recipient_profile.teacher.teaches(sharer_profile): # Is the sharer taught by the recipient? return True - elif hasattr(sharer_profile, "teacher") and hasattr( - recipient_profile, "teacher" - ): + elif hasattr(sharer_profile, "teacher") and hasattr(recipient_profile, "teacher"): # Are they in the same organisation? return recipient_profile.teacher.school == sharer_profile.teacher.school + elif hasattr(sharer_profile, "teacher") and sharer_profile.teacher.is_admin: + return recipient_profile.student.class_field.teacher.school == sharer_profile.teacher.school else: return False diff --git a/game/static/game/css/level_share.css b/game/static/game/css/level_share.css index dd22793a5..debfae6e3 100644 --- a/game/static/game/css/level_share.css +++ b/game/static/game/css/level_share.css @@ -11,15 +11,15 @@ } #share_type_select { - width: 180px; + width: 250px; margin-bottom: 0px; - margin-left: 35px; + margin-left: 15px; } #class_select { - width: 180px; + width: 250px; margin-bottom: 0px; - margin-left: 70px; + margin-left: 50px; } #classes_radio { @@ -34,6 +34,7 @@ tr[status='unshared'] .share_cell { color: red; } -tr[status='pending'] .share_cell { +tr[status="admin"] .share_name, +tr[status="admin"] .share_cell { color: gray; } diff --git a/game/static/game/js/sharing.js b/game/static/game/js/sharing.js index 6a0f2e5e9..ae2ba6a97 100644 --- a/game/static/game/js/sharing.js +++ b/game/static/game/js/sharing.js @@ -8,8 +8,8 @@ var ocargo = ocargo || {}; ocargo.Sharing = function (getLevelId, validationFunction) { this.text = []; this.text.shared = gettext("Yes"); + this.text.admin = gettext("Yes"); this.text.unshared = gettext("No"); - this.text.pending = "..."; this.classesTaught = []; this.fellowTeachers = []; @@ -89,35 +89,47 @@ ocargo.Sharing.prototype.populateSharingTable = function (recipients) { $("#shareLevelTable tr").off("click"); table.empty(); - // Order them alphabetically + // sort by admins first recipients.sort(function (a, b) { - if (a.name < b.name) { - return -1; - } else if (a.name > b.name) { - return 1; + if (a.admin && !b.admin) { + return -1 + } else if (b.admin && !a.admin) { + return 1 } return 0; }); this.allShared = true; - // Add a row to the table for each workspace saved in the database + // Add a row to the table for each recipient for (let i = 0; i < recipients.length; i++) { let recipient = recipients[i]; - let status = recipient.shared ? "shared" : "unshared"; + let status = recipient.shared ? "shared" || "admin" : "unshared"; if (recipient.shared) { status = "shared"; + + if (recipient.admin) { + status = "admin" + } + } else { status = "unshared"; this.allShared = false; } + + let name_text = recipient.name + + if (recipient.admin) { + name_text += (" (Admin)") + } + table.append( '' + - $("
").text(recipient.name).html() + + '">' + + $("
").text(name_text).html() + '' + this.text[status] + "" diff --git a/game/templates/game/level_editor.html b/game/templates/game/level_editor.html index a926f46ba..e14c633df 100644 --- a/game/templates/game/level_editor.html +++ b/game/templates/game/level_editor.html @@ -596,6 +596,7 @@

{% trans "Here you can share your level with your classes or your fellow teachers." %}

+

Admin teachers can already see levels created by you.

{% elif user|get_user_status == 'SCHOOL_STUDENT' %}

{% trans "Here you can share your level with your classmates. Try clicking in the Shared column!" %}

{% endif %} diff --git a/game/templates/game/level_selection.html b/game/templates/game/level_selection.html index 31e0e01f9..4e0166bb6 100644 --- a/game/templates/game/level_selection.html +++ b/game/templates/game/level_selection.html @@ -187,19 +187,46 @@

Created levels

- {% if shared_levels %} - {% for level in shared_levels %} -

- - {{level.title}} ({{level.owner|make_into_username}}) - {% if level.score != None %} - {{level.score|floatformat}}/10 - - {% endif %} -

+ {% if user|is_logged_in_as_admin_teacher %} +
Your classes
+ {% endif %} + {% if directly_shared_levels %} + {% for level in directly_shared_levels %} +

+ + {{ level.title }}, {{ level.owner|make_into_username }} + {% if level.class %} + ({{ level.class }}) + {% endif %} + + {% if level.score != None %} + {{ level.score|floatformat }}/10 + + {% endif %} +

{% endfor %} {% else %} -

No levels have been shared with you yet.

+

No levels.

+ {% endif %} + + {% if indirectly_shared_levels %} + {% for teacher, levels in indirectly_shared_levels.items %} +
Owned by {{ teacher|make_into_username }}
+ {% for level in levels %} +

+ + {{ level.title }}, {{level.owner|make_into_username}} + {% if level.class %} + ({{ level.class }}) + {% endif %} + + {% if level.score != None %} + {{level.score|floatformat}}/10 + + {% endif %} +

+ {% endfor %} + {% endfor %} {% endif %}
diff --git a/game/tests/test_level_editor.py b/game/tests/test_level_editor.py index 76f6cdc06..6b5beea3b 100644 --- a/game/tests/test_level_editor.py +++ b/game/tests/test_level_editor.py @@ -89,8 +89,8 @@ def test_level_saving_school_student(self): assert_that(response.status_code, equal_to(200)) sharing_info1 = json.loads(self.get_sharing_information(json.loads(response.content)["id"]).getvalue()) - assert_that(sharing_info1["teacher"]["shared"], equal_to(True)) - assert_that(len(mail.outbox), equal_to(1)) + assert sharing_info1["teacher"]["shared"] + assert len(mail.outbox) == 1 def test_anonymous_level_saving_school_student(self): email, password = signup_teacher_directly() @@ -119,10 +119,10 @@ def test_anonymous_level_saving_school_student(self): } response = self.client.post(url, {"data": json.dumps(data1)}) - assert_that(response.status_code, equal_to(200)) + assert response.status_code == 200 sharing_info1 = json.loads(self.get_sharing_information(json.loads(response.content)["id"]).getvalue()) - assert_that(sharing_info1["teacher"]["shared"], equal_to(True)) - assert_that(len(mail.outbox), equal_to(0)) + assert sharing_info1["teacher"]["shared"] + assert len(mail.outbox) == 0 def test_level_sharing_with_no_school(self): email1, password1 = signup_teacher_directly() @@ -131,7 +131,7 @@ def test_level_sharing_with_no_school(self): level = create_save_level(teacher1) sharing_info1 = json.loads(self.get_sharing_information(level.id).getvalue()) - assert_that(len(sharing_info1["teachers"]), equal_to(0)) + assert len(sharing_info1["teachers"]) == 0 def test_level_sharing_with_school(self): email1, password1 = signup_teacher_directly() @@ -148,7 +148,7 @@ def test_level_sharing_with_school(self): add_teacher_to_school(teacher2, school1) sharing_info1 = json.loads(self.get_sharing_information(level.id).getvalue()) - assert_that(len(sharing_info1["teachers"]), equal_to(1)) + assert len(sharing_info1["teachers"]) == 1 def test_level_sharing_with_empty_school(self): email1, password1 = signup_teacher_directly() @@ -161,7 +161,7 @@ def test_level_sharing_with_empty_school(self): add_teacher_to_school(teacher1, school1) sharing_info1 = json.loads(self.get_sharing_information(level.id).getvalue()) - assert_that(len(sharing_info1["teachers"]), equal_to(0)) + assert len(sharing_info1["teachers"]) == 0 def test_level_sharing_permissions(self): email1, password1 = signup_teacher_directly() @@ -175,18 +175,19 @@ def test_level_sharing_permissions(self): share_url = reverse("share_level_for_editor", args=[level.id]) school1 = create_school() - add_teacher_to_school(teacher1, school1) + add_teacher_to_school(teacher1, school1, is_admin=True) add_teacher_to_school(teacher2, school1) - # Create a class and student for the second teacher + # Create a class and 2 students for the second teacher _, class_name2, access_code2 = create_class_directly(email2) + student_name1, student_password1, student1 = create_school_student_directly(access_code2) student_name2, student_password2, student2 = create_school_student_directly(access_code2) self.logout() self.login(email2, password2) # Second teacher can't share the level as it's not been shared with them yet - response = self.client.post(share_url, {"recipientIDs[]": [student2.new_user.id], "action": ["share"]}) + response = self.client.post(share_url, {"recipientIDs[]": [student1.new_user.id], "action": ["share"]}) assert response.status_code == 403 # Log in as the first teacher again @@ -202,14 +203,34 @@ def test_level_sharing_permissions(self): self.login(email2, password2) # Now the second teacher should be able to share the level - response = self.client.post(share_url, {"recipientIDs[]": [student2.new_user.id], "action": ["share"]}) + response = self.client.post(share_url, {"recipientIDs[]": [student1.new_user.id], "action": ["share"]}) assert response.status_code == 200 # and load it load_level_url = reverse("load_level_for_editor", kwargs={"levelID": level.id}) response = self.client.get(load_level_url) assert response.status_code == 200 - # Login as a student + # Login as the first student + self.logout() + self.student_login(student_name1, access_code2, student_password1) + + # Check that the student cannot share the level + sharing_info = json.loads(self.get_sharing_information(level.id).getvalue()) + assert sharing_info["detail"] == "You do not have permission to perform this action." + + # Check the student can view the level + response = self.client.get(reverse("play_custom_level", args=[level.id])) + assert response.status_code == 200 + + # Login as first teacher again + self.logout() + self.login(email1, password1) + + # Share the level with the second student of teacher 2 + response = self.client.post(share_url, {"recipientIDs[]": [student2.new_user.id], "action": ["share"]}) + assert response.status_code == 200 + + # Login as the second student self.logout() self.student_login(student_name2, access_code2, student_password2) @@ -221,6 +242,8 @@ def test_level_sharing_permissions(self): response = self.client.get(reverse("play_custom_level", args=[level.id])) assert response.status_code == 200 + self.logout() + def test_level_can_only_be_edited_by_owner(self): email, password = signup_teacher_directly() create_organisation_directly(email) @@ -233,14 +256,14 @@ def test_level_can_only_be_edited_by_owner(self): self.student_login(hacker_name, access_code, hacker_password) resp = self.client.get(reverse("level_editor_chosen_level", kwargs={"levelId": new_level.id})) - self.assertNotIn("level", resp.context) + assert "level" not in resp.context self.logout() self.student_login(student_name, access_code, student_password) resp = self.client.get(reverse("level_editor_chosen_level", kwargs={"levelId": new_level.id})) - self.assertIn("level", resp.context) + assert "level" in resp.context def test_no_character_set_defaults_to_van(self): @@ -269,9 +292,9 @@ def test_no_character_set_defaults_to_van(self): } response = self.client.post(url, {"data": json.dumps(data_with_no_character)}) - assert_that(response.status_code, equal_to(200)) + assert response.status_code == 200 new_level = Level.objects.get(name="abc") - assert_that(new_level.character.name, equal_to("Van")) + assert new_level.character.name == "Van" def test_level_loading(self): email1, password1 = signup_teacher_directly() @@ -283,7 +306,7 @@ def test_level_loading(self): url = reverse("load_level_for_editor", kwargs={"levelID": level.id}) response = self.client.get(url) - assert_that(response.status_code, equal_to(200)) + assert response.status_code == 200 def test_level_of_anonymised_teacher_is_hidden(self): # Create 2 teacher accounts @@ -309,7 +332,7 @@ def test_level_of_anonymised_teacher_is_hidden(self): # Check `teacher1` can see `teacher2`'s shared level levels_url = reverse("levels") response = self.client.get(levels_url) - assert len(response.context["shared_levels"]) == 1 + assert len(response.context["directly_shared_levels"]) == 1 # Make teacher2 inactive teacher2.new_user.is_active = 0 @@ -317,7 +340,7 @@ def test_level_of_anonymised_teacher_is_hidden(self): # `teacher1` shouldn't see any shared levels now response = self.client.get(levels_url) - assert len(response.context["shared_levels"]) == 0 + assert len(response.context["directly_shared_levels"]) == 0 def test_level_of_anonymised_student_is_hidden(self): # Create a teacher and a student @@ -339,7 +362,7 @@ def test_level_of_anonymised_student_is_hidden(self): # Check teacher can see student's shared level levels_url = reverse("levels") response = self.client.get(levels_url) - assert len(response.context["shared_levels"]) == 1 + assert len(response.context["directly_shared_levels"]) == 1 # Make student inactive student.new_user.is_active = 0 @@ -347,4 +370,4 @@ def test_level_of_anonymised_student_is_hidden(self): # Teacher shouldn't see any shared levels now response = self.client.get(levels_url) - assert len(response.context["shared_levels"]) == 0 + assert len(response.context["directly_shared_levels"]) == 0 diff --git a/game/tests/test_level_moderation.py b/game/tests/test_level_moderation.py index 51ecb9afc..85781d7d4 100644 --- a/game/tests/test_level_moderation.py +++ b/game/tests/test_level_moderation.py @@ -1,17 +1,11 @@ -import json - from common.tests.utils.classes import create_class_directly +from common.tests.utils.organisation import create_organisation_directly, join_teacher_to_organisation from common.tests.utils.student import create_school_student_directly from common.tests.utils.teacher import signup_teacher_directly -from common.tests.utils.organisation import ( - create_organisation_directly, - join_teacher_to_organisation, -) from deploy import captcha from django.test.client import Client from django.test.testcases import TestCase from django.urls import reverse -from hamcrest import * from .utils.level import create_save_level @@ -114,8 +108,8 @@ def test_moderation_admin(self): # Create 2 teachers in the same school, one admin, one standard email1, password1 = signup_teacher_directly() email2, password2 = signup_teacher_directly() - name, postcode = create_organisation_directly(email1) - join_teacher_to_organisation(email2, name, postcode, is_admin=False) + school = create_organisation_directly(email1) + join_teacher_to_organisation(email2, school.name, school.postcode) # Create one class and student for each teacher _, class_name1, access_code1 = create_class_directly(email1) @@ -137,9 +131,7 @@ def test_moderation_admin(self): assert student1.new_user.first_name not in response.content.decode() assert student2.new_user.first_name in response.content.decode() # Try to delete level1, it shouldn't work - delete_level1_url = reverse( - "delete_level_for_editor", kwargs={"levelId": level1.id} - ) + delete_level1_url = reverse("delete_level_for_editor", kwargs={"levelId": level1.id}) response = self.client.get(delete_level1_url) assert response.status_code == 401 # Check level2 is still there @@ -157,9 +149,7 @@ def test_moderation_admin(self): assert student1.new_user.first_name in response.content.decode() assert student2.new_user.first_name in response.content.decode() # Delete level2 - delete_level2_url = reverse( - "delete_level_for_editor", kwargs={"levelId": level2.id} - ) + delete_level2_url = reverse("delete_level_for_editor", kwargs={"levelId": level2.id}) response = self.client.get(delete_level2_url) assert response.status_code == 200 # Check level1 is still there and level2 is not there anymore @@ -171,10 +161,6 @@ def test_moderation_admin(self): def teacher_login(self, email, password): self.client.post( reverse("teacher_login"), - { - "auth-username": email, - "auth-password": password, - "teacher_login_view-current_step": "auth", - }, + {"auth-username": email, "auth-password": password, "teacher_login_view-current_step": "auth"}, follow=True, ) diff --git a/game/tests/test_level_selection.py b/game/tests/test_level_selection.py index f0294151c..26beeb6c8 100644 --- a/game/tests/test_level_selection.py +++ b/game/tests/test_level_selection.py @@ -1,28 +1,149 @@ +import json + +from common.models import Teacher +from common.tests.utils.classes import create_class_directly +from common.tests.utils.student import create_school_student_directly +from common.tests.utils.teacher import signup_teacher_directly +from deploy import captcha from django.test.client import Client from django.test.testcases import TestCase from django.urls import reverse -from hamcrest import * + +from game.tests.utils.level import create_save_level +from game.tests.utils.teacher import add_teacher_to_school, create_school class LevelSelectionTestCase(TestCase): + @classmethod + def setUpClass(cls): + cls.orig_captcha_enabled = captcha.CAPTCHA_ENABLED + captcha.CAPTCHA_ENABLED = False + super(LevelSelectionTestCase, cls).setUpClass() + + @classmethod + def tearDownClass(cls): + captcha.CAPTCHA_ENABLED = cls.orig_captcha_enabled + super(LevelSelectionTestCase, cls).tearDownClass() + def setUp(self): self.client = Client() + def login(self, email, password): + self.client.post( + reverse("teacher_login"), + {"auth-username": email, "auth-password": password, "teacher_login_view-current_step": "auth"}, + follow=True, + ) + + def logout(self): + self.client.post(reverse("logout_view"), follow=True) + + def student_login(self, name, access_code, password): + self.client.post( + reverse("student_login", kwargs={"access_code": access_code}), + {"username": name, "password": password}, + follow=True, + ) + + def level_data(self, levelID): + data = { + "origin": '{"coordinate":[3,5],"direction":"S"}', + "pythonEnabled": False, + "decor": [], + "blocklyEnabled": True, + "blocks": [{"type": "move_forwards"}, {"type": "turn_left"}, {"type": "turn_right"}], + "max_fuel": "50", + "pythonViewEnabled": False, + "character": "3", + "name": f"level{levelID}", + "theme": 1, + "anonymous": False, + "cows": "[]", + "path": '[{"coordinate":[3,5],"connectedNodes":[1]},{"coordinate":[3,4],"connectedNodes":[0]}]', + "traffic_lights": "[]", + "destinations": "[[3,4]]", + } + return json.dumps(data) + def test_list_episodes(self): url = reverse("levels") response = self.client.get(url) - assert_that(response.status_code, equal_to(200)) - self._assert_that_response_contains_episode_name(response, "Getting Started") - self._assert_that_response_contains_level_with_title( - response, "Can you help the van get to the house?" - ) + assert response.status_code == 200 + assert response.context["blocklyEpisodes"][0]["name"] == "Getting Started" + assert response.context["blocklyEpisodes"][0]["levels"][0]["title"] == "Can you help the van get to the house?" - def _assert_that_response_contains_episode_name(self, response, expected): - assert_that(response.context["blocklyEpisodes"][0]["name"], equal_to(expected)) + def test_custom_levels_access(self): + email1, password1 = signup_teacher_directly() + email2, password2 = signup_teacher_directly() - def _assert_that_response_contains_level_with_title(self, response, expected): - assert_that( - response.context["blocklyEpisodes"][0]["levels"][0]["title"], - equal_to(expected), - ) + teacher1 = Teacher.objects.get(new_user__email=email1) + teacher2 = Teacher.objects.get(new_user__email=email2) + + school1 = create_school() + add_teacher_to_school(teacher1, school1, is_admin=True) + add_teacher_to_school(teacher2, school1) + + # Create a class and a student for each teacher + _, class_name1, access_code1 = create_class_directly(email1) + _, class_name2, access_code2 = create_class_directly(email2) + student_name1, student_password1, student1 = create_school_student_directly(access_code1) + student_name2, student_password2, student2 = create_school_student_directly(access_code2) + + save_url = "save_level_for_editor" + + # Login as the second teacher + self.login(email2, password2) + teacher2_level = create_save_level(teacher2) + save_level_url = reverse(save_url) + + response = self.client.post(save_level_url, {"data": self.level_data(teacher2_level.id)}) + + assert response.status_code == 200 + + # Login as the first student + self.logout() + self.student_login(student_name1, access_code1, student_password1) + + student1_level = create_save_level(student1) + + response = self.client.post(save_level_url, {"data": self.level_data(student1_level.id)}) + + assert response.status_code == 200 + + # Login as the second student + self.logout() + self.student_login(student_name2, access_code2, student_password2) + + student2_level = create_save_level(student2) + + response = self.client.post(save_level_url, {"data": self.level_data(student2_level.id)}) + + assert response.status_code == 200 + + # Login as first teacher again and check they have access to all the levels created above + self.logout() + self.login(email1, password1) + + url = reverse("levels") + response = self.client.get(url) + + assert response.status_code == 200 + assert len(response.context["directly_shared_levels"]) == 1 + assert response.context["directly_shared_levels"][0]["owner"] == student1.new_user + assert response.context["indirectly_shared_levels"][teacher2.new_user] + assert len(response.context["indirectly_shared_levels"][teacher2.new_user]) == 2 + assert response.context["indirectly_shared_levels"][teacher2.new_user][0]["owner"] == teacher2.new_user + assert response.context["indirectly_shared_levels"][teacher2.new_user][1]["owner"] == student2.new_user + + # Login as second teacher again and check they have access to only their student's level + self.logout() + self.login(email2, password2) + + url = reverse("levels") + response = self.client.get(url) + + assert response.status_code == 200 + assert len(response.context["directly_shared_levels"]) == 1 + assert response.context["directly_shared_levels"][0]["owner"] == student2.new_user + assert response.context["indirectly_shared_levels"] == {} diff --git a/game/tests/utils/teacher.py b/game/tests/utils/teacher.py index 6f2bf1752..6164cc9e5 100644 --- a/game/tests/utils/teacher.py +++ b/game/tests/utils/teacher.py @@ -14,6 +14,7 @@ def create_school() -> School: return school -def add_teacher_to_school(teacher: Teacher, school: School): +def add_teacher_to_school(teacher: Teacher, school: School, is_admin=False): teacher.user.teacher.school = school + teacher.is_admin = is_admin teacher.user.teacher.save() diff --git a/game/views/level_editor.py b/game/views/level_editor.py index a08b74588..888f6c754 100644 --- a/game/views/level_editor.py +++ b/game/views/level_editor.py @@ -5,12 +5,12 @@ from builtins import map from builtins import str -from common.models import Student, Class, Teacher +from common.models import Student, Teacher from django.contrib.auth.models import User -from django.urls import reverse from django.db import transaction from django.http import HttpResponse from django.shortcuts import get_object_or_404, render, redirect +from django.urls import reverse from django.utils.safestring import mark_safe from django.views.decorators.http import require_POST from portal.templatetags import app_tags @@ -51,9 +51,7 @@ def level_editor(request, levelId=None): "characters": get_all_character(), "themes": get_all_themes(), "cow_level_enabled": app_settings.COW_FEATURE_ENABLED, - "night_mode_feature_enabled": str( - app_settings.NIGHT_MODE_FEATURE_ENABLED - ).lower(), + "night_mode_feature_enabled": str(app_settings.NIGHT_MODE_FEATURE_ENABLED).lower(), } if levelId: level = Level.objects.get(id=levelId) @@ -69,15 +67,11 @@ def available_blocks(): if app_settings.COW_FEATURE_ENABLED: return Block.objects.all() else: - return Block.objects.all().exclude( - type__in=["declare_event", "puff_up", "sound_horn"] - ) + return Block.objects.all().exclude(type__in=["declare_event", "puff_up", "sound_horn"]) def play_anonymous_level(request, levelId, from_level_editor=True, random_level=False): - night_mode = ( - False if not app_settings.NIGHT_MODE_FEATURE_ENABLED else "night" in request.GET - ) + night_mode = False if not app_settings.NIGHT_MODE_FEATURE_ENABLED else "night" in request.GET level = Level.objects.filter(id=levelId) if not level.exists(): @@ -139,20 +133,14 @@ def play_anonymous_level(request, levelId, from_level_editor=True, random_level= "character_height": character_height, "wreckage_url": wreckage_url, "night_mode": night_mode, - "night_mode_feature_enabled": str( - app_settings.NIGHT_MODE_FEATURE_ENABLED - ).lower(), + "night_mode_feature_enabled": str(app_settings.NIGHT_MODE_FEATURE_ENABLED).lower(), "model_solution": model_solution, }, ) def level_data_for(level): - return { - "name": level.name, - "owner": app_tags.make_into_username(level.owner.user), - "id": level.id, - } + return {"name": level.name, "owner": app_tags.make_into_username(level.owner.user), "id": level.id} def levels_shared_with(user): @@ -226,26 +214,37 @@ def save_level_for_editor(request, levelId=None): if pattern.match(data["name"]): level_management.save_level(level, data) - # Add the teacher automatically if it is a new level and the student is not - # independent - if ( - (levelId is None) - and hasattr(level.owner, "student") - and not level.owner.student.is_independent() - ): - level.shared_with.add(level.owner.student.class_field.teacher.user.user) + + if levelId is None: + teacher = None + + # if level owner is a school student, share with teacher automatically if they aren't an admin + if hasattr(level.owner, "student") and not level.owner.student.is_independent(): + teacher = level.owner.student.class_field.teacher + if not teacher.is_admin: + level.shared_with.add(teacher.new_user) + + if not data["anonymous"]: + level_management.email_new_custom_level( + level.owner.student.class_field.teacher.new_user.email, + request.build_absolute_uri(reverse("level_moderation")), + request.build_absolute_uri(reverse("play_custom_level", kwargs={"levelId": level.id})), + request.build_absolute_uri(reverse("home")), + str(level.owner.student), + level.owner.student.class_field.name, + ) + elif hasattr(level.owner, "teacher"): + teacher = level.owner.teacher + + # share with all admins of the school + school_admins = teacher.school.admins() + + [ + level.shared_with.add(school_admin.new_user) + for school_admin in school_admins + if school_admin.new_user != request.user + ] level.save() - if not data["anonymous"]: - level_management.email_new_custom_level( - level.owner.student.class_field.teacher.new_user.email, - request.build_absolute_uri(reverse("level_moderation")), - request.build_absolute_uri( - reverse("play_custom_level", kwargs={"levelId": level.id}) - ), - request.build_absolute_uri(reverse("home")), - str(level.owner.student), - level.owner.student.class_field.name, - ) response = {"id": level.id} return HttpResponse(json.dumps(response), content_type="application/javascript") else: @@ -278,15 +277,13 @@ def generate_random_map_for_editor(request): traffic_lights = data["trafficLights"][0] == "true" scenery = data["scenery"][0] == "true" - data = random_road.generate_random_map_data( - size, branchiness, loopiness, curviness, traffic_lights, scenery, False - ) + data = random_road.generate_random_map_data(size, branchiness, loopiness, curviness, traffic_lights, scenery, False) return HttpResponse(json.dumps(data), content_type="application/javascript") class SharingInformationForEditor(APIView): - """Returns a information about who the level can be and is shared with. This uses + """Returns information about who the level can be and is shared with. This uses the CanShareLevel permission.""" authentication_classes = (SessionAuthentication,) @@ -315,15 +312,13 @@ def get(self, request, **kwargs): # First get all the student's classmates class_ = student.class_field - classmates = Student.objects.filter( - class_field=class_, new_user__is_active=True - ).exclude(id=student.id) + classmates = Student.objects.filter(class_field=class_, new_user__is_active=True).exclude(id=student.id) valid_recipients["classmates"] = [ { - "id": classmate.user.user.id, - "name": app_tags.make_into_username(classmate.user.user), + "id": classmate.new_user.id, + "name": app_tags.make_into_username(classmate.new_user), "shared": level.owner == classmate.user - or level.shared_with.filter(id=classmate.user.user.id).exists(), + or level.shared_with.filter(id=classmate.new_user.id).exists(), } for classmate in classmates ] @@ -331,10 +326,9 @@ def get(self, request, **kwargs): # Then add their teacher as well teacher = class_.teacher valid_recipients["teacher"] = { - "id": teacher.user.user.id, - "name": app_tags.make_into_username(teacher.user.user), - "shared": level.owner == teacher.user - or level.shared_with.filter(id=teacher.user.user.id).exists(), + "id": teacher.new_user.id, + "name": app_tags.make_into_username(teacher.new_user), + "shared": level.owner == teacher.user or level.shared_with.filter(id=teacher.new_user.id).exists(), } elif hasattr(userprofile, "teacher"): @@ -342,23 +336,24 @@ def get(self, request, **kwargs): # First get all the students they teach valid_recipients["classes"] = [] - classes_taught = Class.objects.filter(teacher=teacher) + if teacher.is_admin: + classes_taught = teacher.school.classes() + else: + classes_taught = teacher.class_teacher.all() for class_ in classes_taught: - students = Student.objects.filter( - class_field=class_, new_user__is_active=True - ) + students = Student.objects.filter(class_field=class_, new_user__is_active=True) valid_recipients["classes"].append( { - "name": class_.name, + "name": f"{class_.name} ({app_tags.make_into_username(class_.teacher.new_user)})" + if teacher.is_admin + else class_.name, "id": class_.id, "students": [ { - "id": student.user.user.id, - "name": app_tags.make_into_username(student.user.user), + "id": student.new_user.id, + "name": app_tags.make_into_username(student.new_user), "shared": level.owner == student.user - or level.shared_with.filter( - id=student.user.user.id - ).exists(), + or level.shared_with.filter(id=student.new_user.id).exists(), } for student in students ], @@ -371,20 +366,17 @@ def get(self, request, **kwargs): fellow_teachers = Teacher.objects.filter(school=teacher.school) valid_recipients["teachers"] = [ { - "id": fellow_teacher.user.user.id, - "name": app_tags.make_into_username(fellow_teacher.user.user), + "id": fellow_teacher.new_user.id, + "name": app_tags.make_into_username(fellow_teacher.new_user), + "admin": fellow_teacher.is_admin, "shared": level.owner == fellow_teacher.user - or level.shared_with.filter( - id=fellow_teacher.user.user.id - ).exists(), + or level.shared_with.filter(id=fellow_teacher.new_user.id).exists(), } for fellow_teacher in fellow_teachers if teacher != fellow_teacher ] - return HttpResponse( - json.dumps(valid_recipients), content_type="application/javascript" - ) + return HttpResponse(json.dumps(valid_recipients), content_type="application/javascript") class ShareLevelView(APIView): @@ -425,11 +417,7 @@ def _get_users_to_share_level_with(self, recipients): :return: A list of users which the requester is authorised to share the level with. """ - return [ - recipient.userprofile.user - for recipient in recipients - if self._can_share_level_with(recipient) - ] + return [recipient.userprofile.user for recipient in recipients if self._can_share_level_with(recipient)] def _can_share_level_with(self, recipient): """ @@ -438,13 +426,9 @@ def _can_share_level_with(self, recipient): :param recipient: User that the requester wants to share a level with. :return: A boolean of whether the requester has permission. """ - return permissions.CanShareLevelWith().has_object_permission( - self.request, self, recipient - ) + return permissions.CanShareLevelWith().has_object_permission(self.request, self, recipient) class HttpResponseUnauthorized(HttpResponse): def __init__(self): - super(HttpResponseUnauthorized, self).__init__( - content="Unauthorized", status=401 - ) + super(HttpResponseUnauthorized, self).__init__(content="Unauthorized", status=401) diff --git a/game/views/level_selection.py b/game/views/level_selection.py index bd37a5ed7..25cc9d19f 100644 --- a/game/views/level_selection.py +++ b/game/views/level_selection.py @@ -45,12 +45,7 @@ def fetch_episode_data_from_database(early_access, start, end): minName = level_name levels.append( - { - "id": level.id, - "name": level_name, - "maxScore": max_score(level), - "title": get_level_title(level_name), - } + {"id": level.id, "name": level_name, "maxScore": max_score(level), "title": get_level_title(level_name)} ) e = { @@ -85,10 +80,7 @@ def fetch_episode_data(early_access, start=1, end=12): dict( episode, name=messages.get_episode_title(episode["id"]), - levels=[ - dict(level, title=get_level_title(level["name"])) - for level in episode["levels"] - ], + levels=[dict(level, title=get_level_title(level["name"])) for level in episode["levels"]], ) for episode in data ] @@ -111,6 +103,14 @@ def is_student(user): return hasattr(user.userprofile, "student") +def is_teacher(user): + return hasattr(user.userprofile, "teacher") + + +def is_admin_teacher(user): + return hasattr(user.userprofile, "teacher") and user.userprofile.teacher.is_admin + + def levels(request): """Loads a page with all levels listed. @@ -136,46 +136,56 @@ def levels(request): else: attempts = {} - blockly_episodes = fetch_episode_data( - app_settings.EARLY_ACCESS_FUNCTION(request), 1, 9 - ) + blockly_episodes = fetch_episode_data(app_settings.EARLY_ACCESS_FUNCTION(request), 1, 9) for episode in blockly_episodes: for level in episode["levels"]: attach_attempts_to_level(attempts, level) - python_episodes = fetch_episode_data( - app_settings.EARLY_ACCESS_FUNCTION(request), 10, 11 - ) + python_episodes = fetch_episode_data(app_settings.EARLY_ACCESS_FUNCTION(request), 10, 11) for episode in python_episodes: for level in episode["levels"]: attach_attempts_to_level(attempts, level) owned_level_data = [] - shared_level_data = [] + directly_shared_levels = [] + indirectly_shared_levels = {} if not request.user.is_anonymous: owned_levels, shared_levels = level_management.get_loadable_levels(request.user) for level in owned_levels: owned_level_data.append( - { - "id": level.id, - "title": level.name, - "score": attempts.get(level.id), - "maxScore": 10, - } + {"id": level.id, "title": level.name, "score": attempts.get(level.id), "maxScore": 10} ) for level in shared_levels: - shared_level_data.append( - { - "id": level.id, - "title": level.name, - "owner": level.owner.user, - "score": attempts.get(level.id), - "maxScore": 10, - } - ) - + # if user is an admin teacher, sort levels by their own classes first (directly shared levels) and then by + # other classes in the school (indirectly shared levels). For each of those, get the levels owned by a + # teacher as well as those owned by a student. + if is_admin_teacher(user): + # get levels shared by fellow teachers + if is_teacher(level.owner.user): + teacher = level.owner.teacher.new_user + + if teacher not in indirectly_shared_levels: + indirectly_shared_levels[teacher] = [] + + indirectly_shared_levels[teacher].append(get_shared_level(level, attempts)) + else: + student_class = level.owner.student.class_field + class_teacher = student_class.teacher.new_user + + # get levels shared by students in the current user's classes + if class_teacher == user: + directly_shared_levels.append(get_shared_level(level, attempts, student_class)) + # get levels shared by students in the other teachers' classes + else: + if class_teacher not in indirectly_shared_levels: + indirectly_shared_levels[class_teacher] = [] + + indirectly_shared_levels[class_teacher].append(get_shared_level(level, attempts, student_class)) + # if user is a student or a standard teacher, just get levels shared with them directly. + else: + directly_shared_levels.append(get_shared_level(level, attempts)) return render( request, "game/level_selection.html", @@ -183,12 +193,24 @@ def levels(request): "blocklyEpisodes": blockly_episodes, "pythonEpisodes": python_episodes, "owned_levels": owned_level_data, - "shared_levels": shared_level_data, + "directly_shared_levels": directly_shared_levels, + "indirectly_shared_levels": indirectly_shared_levels, "scores": attempts, }, ) +def get_shared_level(level, attempts, student_class=None): + return { + "id": level.id, + "title": level.name, + "owner": level.owner.user, + "class": student_class, + "score": attempts.get(level.id), + "maxScore": 10, + } + + def random_level_for_episode(request, episodeID): """Generates a new random level based on the episodeID