From 1b61f60e36dd6075715755d9802208f3704c89b0 Mon Sep 17 00:00:00 2001 From: Mircea Lungu Date: Mon, 31 Aug 2026 15:29:33 +0300 Subject: [PATCH 1/3] State the classroom visibility rules as a table The rules for what a student may see are spread across cohort_articles_for_user, _classroom_only and the article filters. No file states them, which is why they keep being got wrong: the language filter that emptied Jack's classroom, the off-language texts 54 students cannot open, and his question about whether two classes would collide were all the same gap in one form or another. Eight rows, each one world and what a student in it sees. Readable in a minute, and executed against the real model rather than a description of it. Writing them down immediately found one: _tiago_exercises dereferenced user.learned_language.code with no guard, and features_for_user runs on every /user_details -- so an account with no learned language would have taken down the whole call. No such account exists in production, and the model allows one; the classroom code already handles it, so the two disagreed. Guarded. Co-Authored-By: Claude Opus 5 --- .../test/test_classroom_visibility_rules.py | 123 ++++++++++++++++++ zeeguu/core/user_feature_toggles.py | 8 +- 2 files changed, 130 insertions(+), 1 deletion(-) create mode 100644 zeeguu/core/test/test_classroom_visibility_rules.py diff --git a/zeeguu/core/test/test_classroom_visibility_rules.py b/zeeguu/core/test/test_classroom_visibility_rules.py new file mode 100644 index 000000000..f3e3304d8 --- /dev/null +++ b/zeeguu/core/test/test_classroom_visibility_rules.py @@ -0,0 +1,123 @@ +"""What a student may see in their classroom, as a table. + +Every row is one world, and what a student in it sees. These rules are spread +across cohort_articles_for_user, _classroom_only and the article filters; no +one file states them, which is why they keep being got wrong. State them here +and let the real code answer. + +Read a row as: + + a student LEARNING this language, enrolled in these CLASSES, + sees SEES texts, and their app is in MODE. + +In CLASSES, the value is the language of each text in that class, so +"en en da" is a class holding two English texts and one Danish. A class name +ending in ! has "students see only the texts I share" switched on. +""" + +from unittest import TestCase + +import zeeguu.core +from zeeguu.core.model.cohort import Cohort +from zeeguu.core.model.cohort_article_map import CohortArticleMap +from zeeguu.core.model.teacher import Teacher +from zeeguu.core.test.model_test_mixin import ModelTestMixIn +from zeeguu.core.test.rules.article_rule import ArticleRule +from zeeguu.core.test.rules.language_rule import LanguageRule +from zeeguu.core.test.rules.user_rule import UserRule +from zeeguu.core.user_feature_toggles import is_feature_enabled_for_user + +db_session = zeeguu.core.model.db.session + +FULL_APP = "full app" +CLASSROOM_ONLY = "classroom only" + +# learning classes sees mode +RULES = [ + ("the ordinary case", + "da", {"Danish Class": "da da da"}, 3, FULL_APP), + + ("a class in another language is invisible, not empty", + "da", {"English Class": "en en"}, 0, FULL_APP), + + ("a class may hold texts the student cannot see", + "da", {"Mixed Class": "da en en"}, 1, FULL_APP), + + ("two classes merge into one list", + "da", {"A": "da", "B": "da"}, 2, FULL_APP), + + ("a text in two of the student's classes is listed once", + "da", {"A": "shared", "B": "shared"}, 1, FULL_APP), + + ("one restricted class restricts the whole app", + "da", {"A": "da da", "B!": "da"}, 3, CLASSROOM_ONLY), + + ("Jack's bug: restricted, and every text is in another language", + "de", {"CUT!": "en en en en en"}, 0, CLASSROOM_ONLY), + + ("a student with no language set sees nothing", + None, {"Danish Class": "da da"}, 0, FULL_APP), +] + + +class ClassroomVisibilityRulesTest(ModelTestMixIn, TestCase): + def _language(self, code): + return LanguageRule.get_or_create_language(code) + + _next_code = 0 + + def _world(self, learning, classes): + student = UserRule().user + if learning: + student.set_learned_language(learning, session=db_session) + else: + student.learned_language = None + + shared_article = None + for name, text_languages in classes.items(): + # invite codes are unique across the whole table, and class names + # repeat between rows + ClassroomVisibilityRulesTest._next_code += 1 + cohort = Cohort(f"code-{ClassroomVisibilityRulesTest._next_code}", + name.rstrip("!"), + self._language("da"), 10, + only_classroom_texts=name.endswith("!")) + db_session.add(cohort) + student.add_user_to_cohort(cohort, db_session) + + for code in text_languages.split(): + if code == "shared": + # the same article, deliberately, in both classes + if shared_article is None: + shared_article = ArticleRule().article + shared_article.language = self._language("da") + article = shared_article + else: + article = ArticleRule().article + article.language = self._language(code) + db_session.add(article) + db_session.add(CohortArticleMap(cohort, article, None)) + db_session.commit() + return student + + def test_every_rule(self): + for label, learning, classes, sees, mode in RULES: + with self.subTest(label): + student = self._world(learning, classes) + + self.assertEqual( + sees, len(student.cohort_articles_for_user()), + f"{label}: wrong number of texts visible") + + restricted = is_feature_enabled_for_user("classroom_only", student) + self.assertEqual( + mode, CLASSROOM_ONLY if restricted else FULL_APP, + f"{label}: wrong mode") + + def test_a_teacher_is_never_restricted(self): + # Stated separately because it is about who you are, not what you see. + teacher = self._world("da", {"A!": "da"}) + db_session.add(Teacher(teacher)) + db_session.commit() + + self.assertFalse(is_feature_enabled_for_user("classroom_only", teacher)) diff --git a/zeeguu/core/user_feature_toggles.py b/zeeguu/core/user_feature_toggles.py index 83b2c818a..4e6653f3a 100644 --- a/zeeguu/core/user_feature_toggles.py +++ b/zeeguu/core/user_feature_toggles.py @@ -73,7 +73,13 @@ def _new_topics(user): def _tiago_exercises(user): right_user = user.invitation_code == "Tiago" or user.id == 534 or user.id == 4022 - right_language = user.learned_language.code in ["da"] + # Guarded: features_for_user runs on every /user_details, so an account + # without a learned language would take the whole call down. No such + # account exists today, but the model allows one and the classroom code + # already handles it. + right_language = ( + user.learned_language is not None and user.learned_language.code in ["da"] + ) return right_user and right_language From d3c72af0a558b4bff7b536cb758597161bdd1730 Mon Sep 17 00:00:00 2001 From: Mircea Lungu Date: Wed, 2 Sep 2026 14:34:49 +0200 Subject: [PATCH 2/3] Put the classroom's rules in one file you can read The table in the previous commit checks that the rules hold. This makes the code that implements them say the same thing. zeeguu/core/classroom.py is the whole of the classroom's behaviour, in the order someone would ask about it: which classes is this student in, which texts are in them, which of those can the student read, and is the classroom the whole app for them. Each rule is one function and the sentence that justifies it: student_can_read the student's own language decides, not the class's classroom_of one merged list; a text in two classes appears once hidden_from the texts a class holds that nobody in it can open sees_only_class_texts strictest class wins; teachers are never restricted They used to be spread across User.cohort_articles_for_user, _classroom_only and the article filters, with no file stating any of them -- which is how a Greek learner in an English class ended up staring at a blank app, and how 54 students came to have texts in their class they cannot open. Both call sites are now adapters. cohort_articles_for_user turns the answer into article infos; _classroom_only forwards. No behaviour change, and the table is what says so: 298 tests and 20 subtests pass unchanged. Co-Authored-By: Claude Opus 5 --- zeeguu/core/classroom.py | 103 ++++++++++++++++++++++++++++ zeeguu/core/model/user.py | 55 +++++---------- zeeguu/core/user_feature_toggles.py | 22 ++---- 3 files changed, 126 insertions(+), 54 deletions(-) create mode 100644 zeeguu/core/classroom.py diff --git a/zeeguu/core/classroom.py b/zeeguu/core/classroom.py new file mode 100644 index 000000000..a1fbc08b1 --- /dev/null +++ b/zeeguu/core/classroom.py @@ -0,0 +1,103 @@ +"""Who sees which class texts. + +The whole of the classroom, in the order someone would ask about it: + + which classes is this student in? + which texts are in those classes? + which of those can the student actually read? + and is the classroom the whole app for them? + +Each rule below is one function and the sentence that justifies it. Everything +that is not a rule -- queries, caching, serialising to JSON -- lives elsewhere +and calls in here. The rules used to be spread across User, the feature +toggles and the article filters, which is why they kept being got wrong. + +test_classroom_visibility_rules.py states the same rules as a table of worlds +and expected outcomes; it is the check that this file still means what it says. +""" + + +def classes_of(user): + """The classes a user is enrolled in. + + Enrolment, not ownership: a teacher who owns a class is not in it unless + they joined it like a student. + """ + from zeeguu.core.model.cohort import Cohort + + return [Cohort.find(membership.cohort_id) for membership in user.cohorts] + + +def texts_of(cohort): + """Every text a teacher has shared with this class, in any language.""" + from zeeguu.core.model.cohort_article_map import CohortArticleMap + + return CohortArticleMap.get_articles_for_cohort(cohort) + + +def student_can_read(student, text): + """A student reads only in the language they are currently learning. + + The student's own setting decides, not the class's -- which is why a + Greek learner in an English class sees an empty classroom, and why a class + can hold texts that nobody in it can open. + """ + if student.learned_language is None: + return False + return text.language_id == student.learned_language_id + + +def classroom_of(student): + """The texts a student sees, each with the classes it reached them through. + + One merged list: a student in several classes has one classroom, not one + per class. A text shared with two of their classes appears once, naming + both. + + Returns [(Article, [Cohort])], in the order the classes were joined. + """ + reached_through = {} + order = [] + + for cohort in classes_of(student): + for text in texts_of(cohort): + if not student_can_read(student, text): + continue + if text.id not in reached_through: + order.append(text) + reached_through[text.id] = [] + reached_through[text.id].append(cohort) + + return [(text, reached_through[text.id]) for text in order] + + +def hidden_from(student): + """The texts in the student's classes that they cannot open. + + The other half of student_can_read, and the reason a class can look full + to its teacher and empty to half its students. + """ + return [ + text + for cohort in classes_of(student) + for text in texts_of(cohort) + if not student_can_read(student, text) + ] + + +def sees_only_class_texts(user): + """Whether the app is reduced to the classroom: no feed, no search, no inbox. + + A teacher sets it per class. A student in several classes is restricted if + any one of them asks for it -- the strictest class wins, and the way out is + to leave that class. + + Teachers are never restricted, even in a class that asks for it: they need + the feed and the search to find the texts they are going to share. This is + why a teacher clicking "Student Site" is shown the full app and cannot see + what their own students see. + """ + if user.isTeacher(): + return False + + return any(cohort.only_classroom_texts for cohort in classes_of(user)) diff --git a/zeeguu/core/model/user.py b/zeeguu/core/model/user.py index ecfdc416c..0a1a5e48a 100644 --- a/zeeguu/core/model/user.py +++ b/zeeguu/core/model/user.py @@ -551,48 +551,31 @@ def add_user_to_cohort(self, cohort, session): session.commit() def cohort_articles_for_user(self): - """The class texts this student can see, each tagged with its class. + """The classroom, as the API sends it. - A student in more than one class gets one merged list, so each text - says which class it came from -- otherwise the list is a pile with no - provenance. The tag is only useful to a student with several classes; - the client drops it for everyone else. + The rules for what a student sees live in zeeguu.core.classroom; this + only turns the answer into article infos and tags each one with the + classes it came from. """ - from zeeguu.core.model import Cohort, CohortArticleMap, UserArticle + from zeeguu.core.classroom import classroom_of + from zeeguu.core.model import UserArticle - all_articles = [] - classes_by_article = {} try: - # Filter articles by the user's learned language: a student only - # ever reads in the language they are currently learning, so a - # class taught in another one has nothing to show them. - user_language_id = ( - self.learned_language_id if self.learned_language else None - ) - - for c in self.cohorts: - cohort = Cohort.find(c.cohort_id) - for article in CohortArticleMap.get_articles_for_cohort(cohort): - if article.language_id != user_language_id: - continue - # A text shared with two of this student's classes appears - # once, tagged with both. - if article.id not in classes_by_article: - all_articles.append(article) - classes_by_article[article.id] = [] - classes_by_article[article.id].append( - {"id": cohort.id, "name": cohort.name} - ) - - infos = UserArticle.article_infos( - self, all_articles, select_appropriate=False - ) - for info in infos: - info["from_classes"] = classes_by_article.get(info["id"], []) - return infos - except NoResultFound as e: + visible = classroom_of(self) + except NoResultFound: return [] + infos = UserArticle.article_infos( + self, [text for text, _ in visible], select_appropriate=False + ) + classes_by_text = { + text.id: [{"id": c.id, "name": c.name} for c in cohorts] + for text, cohorts in visible + } + for info in infos: + info["from_classes"] = classes_by_text.get(info["id"], []) + return infos + def isTeacher(self): from zeeguu.core.model import Teacher diff --git a/zeeguu/core/user_feature_toggles.py b/zeeguu/core/user_feature_toggles.py index 4e6653f3a..63d1f6f44 100644 --- a/zeeguu/core/user_feature_toggles.py +++ b/zeeguu/core/user_feature_toggles.py @@ -128,26 +128,12 @@ def _always_open_externally(user): def _classroom_only(user): """The student sees only the texts their teacher shares with the class. - Set per class by the teacher (Cohort.only_classroom_texts). A student in - several classes is restricted if any one of them asks for it: the strictest - class wins, and the way out is to leave that class. - - Teachers are excluded even when they are members of such a cohort -- they - need the feed and search to find the texts they are going to share. + The rule itself is in zeeguu.core.classroom, next to the rest of the + classroom's behaviour. """ - if user.isTeacher(): - return False - - cohort_ids = [user_cohort.cohort_id for user_cohort in user.cohorts] - if not cohort_ids: - return False + from zeeguu.core.classroom import sees_only_class_texts - return ( - Cohort.query.filter(Cohort.id.in_(cohort_ids)) - .filter(Cohort.only_classroom_texts.is_(True)) - .count() - > 0 - ) + return sees_only_class_texts(user) def _verbal_flashcards(user): From 0f9ecb0296a19320d21d8e445cefe86e6077c553 Mon Sep 17 00:00:00 2001 From: Mircea Lungu Date: Wed, 2 Sep 2026 15:23:27 +0200 Subject: [PATCH 3/3] Separate the classroom's rules from the code that applies them hidden_from was in the same file as student_can_read, and it is not a rule -- it walks every class and every text and applies one. Mixing the two means the file is no longer readable in a minute, because you have to sort the policy from the traversal yourself. classroom/rules.py now holds only policies: functions that answer "is this allowed?" from values they are given, with no traversal, no query and no I/O. Two of them, 44 lines including the prose: student_can_read(student, text) app_is_reduced_to_classroom(user, classes) Note the second takes the classes rather than fetching them -- that is what keeps it a rule. classroom/queries.py finds things and applies those: classes_of, texts_of, classroom_of, hidden_from, sees_only_class_texts. Queries call rules; rules never call queries. No behaviour change: 298 tests and 20 subtests pass unchanged. Co-Authored-By: Claude Opus 5 --- zeeguu/core/classroom.py | 103 ------------------------------ zeeguu/core/classroom/__init__.py | 18 ++++++ zeeguu/core/classroom/queries.py | 68 ++++++++++++++++++++ zeeguu/core/classroom/rules.py | 44 +++++++++++++ 4 files changed, 130 insertions(+), 103 deletions(-) delete mode 100644 zeeguu/core/classroom.py create mode 100644 zeeguu/core/classroom/__init__.py create mode 100644 zeeguu/core/classroom/queries.py create mode 100644 zeeguu/core/classroom/rules.py diff --git a/zeeguu/core/classroom.py b/zeeguu/core/classroom.py deleted file mode 100644 index a1fbc08b1..000000000 --- a/zeeguu/core/classroom.py +++ /dev/null @@ -1,103 +0,0 @@ -"""Who sees which class texts. - -The whole of the classroom, in the order someone would ask about it: - - which classes is this student in? - which texts are in those classes? - which of those can the student actually read? - and is the classroom the whole app for them? - -Each rule below is one function and the sentence that justifies it. Everything -that is not a rule -- queries, caching, serialising to JSON -- lives elsewhere -and calls in here. The rules used to be spread across User, the feature -toggles and the article filters, which is why they kept being got wrong. - -test_classroom_visibility_rules.py states the same rules as a table of worlds -and expected outcomes; it is the check that this file still means what it says. -""" - - -def classes_of(user): - """The classes a user is enrolled in. - - Enrolment, not ownership: a teacher who owns a class is not in it unless - they joined it like a student. - """ - from zeeguu.core.model.cohort import Cohort - - return [Cohort.find(membership.cohort_id) for membership in user.cohorts] - - -def texts_of(cohort): - """Every text a teacher has shared with this class, in any language.""" - from zeeguu.core.model.cohort_article_map import CohortArticleMap - - return CohortArticleMap.get_articles_for_cohort(cohort) - - -def student_can_read(student, text): - """A student reads only in the language they are currently learning. - - The student's own setting decides, not the class's -- which is why a - Greek learner in an English class sees an empty classroom, and why a class - can hold texts that nobody in it can open. - """ - if student.learned_language is None: - return False - return text.language_id == student.learned_language_id - - -def classroom_of(student): - """The texts a student sees, each with the classes it reached them through. - - One merged list: a student in several classes has one classroom, not one - per class. A text shared with two of their classes appears once, naming - both. - - Returns [(Article, [Cohort])], in the order the classes were joined. - """ - reached_through = {} - order = [] - - for cohort in classes_of(student): - for text in texts_of(cohort): - if not student_can_read(student, text): - continue - if text.id not in reached_through: - order.append(text) - reached_through[text.id] = [] - reached_through[text.id].append(cohort) - - return [(text, reached_through[text.id]) for text in order] - - -def hidden_from(student): - """The texts in the student's classes that they cannot open. - - The other half of student_can_read, and the reason a class can look full - to its teacher and empty to half its students. - """ - return [ - text - for cohort in classes_of(student) - for text in texts_of(cohort) - if not student_can_read(student, text) - ] - - -def sees_only_class_texts(user): - """Whether the app is reduced to the classroom: no feed, no search, no inbox. - - A teacher sets it per class. A student in several classes is restricted if - any one of them asks for it -- the strictest class wins, and the way out is - to leave that class. - - Teachers are never restricted, even in a class that asks for it: they need - the feed and the search to find the texts they are going to share. This is - why a teacher clicking "Student Site" is shown the full app and cannot see - what their own students see. - """ - if user.isTeacher(): - return False - - return any(cohort.only_classroom_texts for cohort in classes_of(user)) diff --git a/zeeguu/core/classroom/__init__.py b/zeeguu/core/classroom/__init__.py new file mode 100644 index 000000000..f827a7705 --- /dev/null +++ b/zeeguu/core/classroom/__init__.py @@ -0,0 +1,18 @@ +from zeeguu.core.classroom.queries import ( + classes_of, + classroom_of, + hidden_from, + sees_only_class_texts, + texts_of, +) +from zeeguu.core.classroom.rules import app_is_reduced_to_classroom, student_can_read + +__all__ = [ + "app_is_reduced_to_classroom", + "classes_of", + "classroom_of", + "hidden_from", + "sees_only_class_texts", + "student_can_read", + "texts_of", +] diff --git a/zeeguu/core/classroom/queries.py b/zeeguu/core/classroom/queries.py new file mode 100644 index 000000000..2a6af68d1 --- /dev/null +++ b/zeeguu/core/classroom/queries.py @@ -0,0 +1,68 @@ +"""Finding things in the classroom. + +Everything here is implementation: it walks the model, applies the rules from +classroom/rules.py, and hands back the answer. No policy is decided in this +file -- a condition about what a student may see belongs next door. +""" + +from zeeguu.core.classroom.rules import app_is_reduced_to_classroom, student_can_read + + +def classes_of(user): + """The classes a user is enrolled in. + + Enrolment, not ownership: a teacher who owns a class is not in it unless + they joined it like a student. + """ + from zeeguu.core.model.cohort import Cohort + + return [Cohort.find(membership.cohort_id) for membership in user.cohorts] + + +def texts_of(cohort): + """Every text a teacher has shared with this class, in any language.""" + from zeeguu.core.model.cohort_article_map import CohortArticleMap + + return CohortArticleMap.get_articles_for_cohort(cohort) + + +def classroom_of(student): + """The texts a student sees, each with the classes it reached them through. + + One merged list: a student in several classes has one classroom, not one + per class, and a text shared with two of them appears once naming both. + + Returns [(Article, [Cohort])], in the order the classes were joined. + """ + reached_through = {} + order = [] + + for cohort in classes_of(student): + for text in texts_of(cohort): + if not student_can_read(student, text): + continue + if text.id not in reached_through: + order.append(text) + reached_through[text.id] = [] + reached_through[text.id].append(cohort) + + return [(text, reached_through[text.id]) for text in order] + + +def hidden_from(student): + """The texts in the student's classes that they cannot open. + + The reason a class can look full to its teacher and empty to half of its + students. + """ + return [ + text + for cohort in classes_of(student) + for text in texts_of(cohort) + if not student_can_read(student, text) + ] + + +def sees_only_class_texts(user): + """Whether the app is reduced to the classroom for this user.""" + return app_is_reduced_to_classroom(user, classes_of(user)) diff --git a/zeeguu/core/classroom/rules.py b/zeeguu/core/classroom/rules.py new file mode 100644 index 000000000..261b772bb --- /dev/null +++ b/zeeguu/core/classroom/rules.py @@ -0,0 +1,44 @@ +"""The classroom's rules. Nothing else lives here. + +Each function below is a policy: it answers "is this allowed?" from values it +is given, and touches no database. That is the point of the file -- you can +read it end to end in under a minute, and there is nowhere for behaviour to +hide, because there is no traversal, no query and no I/O. + +Anything that has to *find* things -- which classes a student is in, which +texts are in them -- is a query and lives in classroom/queries.py. Those call +these; these never call those. + +test_classroom_visibility_rules.py states the same rules as a table of worlds +and expected outcomes. +""" + + +def student_can_read(student, text): + """A student reads only in the language they are currently learning. + + The student's own setting decides, not the class's -- which is why a Greek + learner in an English class sees an empty classroom, and why a class can + hold texts that nobody in it can open. + """ + if student.learned_language is None: + return False + return text.language_id == student.learned_language_id + + +def app_is_reduced_to_classroom(user, classes): + """Whether this user gets only the classroom: no feed, no search, no inbox. + + A teacher sets it per class. A student in several classes is restricted if + any one of them asks for it -- the strictest class wins, and the way out is + to leave that class. + + Teachers are never restricted, even in a class that asks for it: they need + the feed and the search to find the texts they are going to share. This is + why a teacher clicking "Student Site" gets the full app, and cannot see + what their own students see. + """ + if user.isTeacher(): + return False + + return any(cohort.only_classroom_texts for cohort in classes)