[FIX] Fix loading teacher adventures (hedyorg#4173)

This PR fixes two issues around teacher adventure customizations:

- Class customizations had the `teacher_adventures` field, which indicate what adventures are available in a particular class. This list of adventures had been replaced with the `adventure_order` set, and the UI that would fill the `teacher_adventures` field has been removed, so the field wasn't being set anymore. However, the `load_customized_adventures` function was still assuming that it only needed to load teacher adventures from the `teacher_adventures` field (and hence, since that field would always be empty, no adventures would be loaded).

- Second, on the class customization page, adventures wouldn't show up in the "available adventures" dropdown right away. That's because the code that would add options to the dropdown wasn't being called on page load, but only when the user twiddled the other parts of the user interface.

This PR addresses both of these issues.

**How to test**

Create an adventure, create a class, add a student to the class, add an adventure to the class, log in as the student, observe the adventure.

app.py

@@ -189,41 +189,40 @@ def load_customized_adventures(level, customizations, into_adventures):
     a bit expensive and since we've already done that in the caller, we pass
     it in here.
 
-    Mutates the 'into_adventures' list in-place
+    Mutates the 'into_adventures' list in-place. On entry, it is the list of
+    default `Adventure` objects in the default order. On exit, it may have been
+    reordered and enhanced with teacher-written adventures.
     """
-    # Add teacher adventures
-    # [ <str>id ]
-    teacher_adventures = customizations.get('teacher_adventures', [])
-    for adv_id in teacher_adventures:
-        # FIXME: This should be a batch-get on the collection
-        adventure = DATABASE.get_adventure(adv_id)
-        try:
-            adventure['content'] = safe_format(adventure['content'],
-                                               **hedy_content.KEYWORDS.get(g.keyword_lang))
-        except BaseException:
-            # We don't want teacher being able to break the student UI -> pass this adventure
-            pass
-
-        into_adventures.append(Adventure(
-            short_name=adv_id,
-            name=adventure['name'],
-            save_name=adventure['name'] + ' ' + str(level),
-            start_code='',  # Teacher adventures don't seem to have this
-            text=adventure['content'],
-            is_teacher_adventure=True))
-
-    # Select a subset of available adventures, in the right order.
+    # First find the list of all teacher adventures for the current level,
+    # batch-get all of them, then put them into the adventures array in the correct
+    # location.
 
     # { <str>level -> [ { <str>name, <bool>from_teacher ] }
     # 'name' is either a shortname or an ID into the teacher table
     sorted_adventures = customizations.get('sorted_adventures', {})
     order_for_this_level = sorted_adventures.get(str(level), [])
-    if order_for_this_level:
-        adventure_map = {a['short_name']: a for a in into_adventures}
+    if not order_for_this_level:
+        return  # Nothing to do
+
+    adventure_ids = {a['name'] for a in order_for_this_level if a['from_teacher']}
+    teacher_adventure_map = DATABASE.batch_get_adventures(adventure_ids)
+    builtin_adventure_map = {a.short_name: a for a in into_adventures}
+    print(teacher_adventure_map)
+
+    # Replace `into_adventures`
+    into_adventures[:] = []
+    for a in order_for_this_level:
+        if a['from_teacher'] and (db_row := teacher_adventure_map.get(a['name'])):
+            try:
+                db_row['content'] = safe_format(db_row['content'],
+                                                **hedy_content.KEYWORDS.get(g.keyword_lang))
+            except Exception:
+                # We don't want teacher being able to break the student UI -> pass this adventure
+                pass
 
-        # Replace entire list
-        into_adventures[:] = [adventure_map[select['name']]
-                              for select in order_for_this_level if select['name'] in adventure_map]
+            into_adventures.append(Adventure.from_teacher_adventure_database_row(db_row))
+        if not a['from_teacher']:
+            into_adventures.append(builtin_adventure_map[a['name']])
 
 
 @babel.localeselector
@@ -1051,8 +1050,6 @@ def index(level, program_id):
 
         loaded_program = Program.from_database_row(result)
 
-    # In case of a "forced keyword language" -> load that one, otherwise: load
-    # the one stored in the g object
     adventures = load_adventures_for_level(level)
 
     # Initially all levels are available -> strip those for which conditions

static/js/teachers.ts

@@ -203,12 +203,6 @@ function update_db_adventure(adventure_id: string) {
    const level = $('#custom_adventure_level').val();
    const content = DOMPurify.sanitize(<string>$('#custom_adventure_content').val());
    const agree_public = $('#agree_public').prop('checked');
-   // Get all checked checkboxes of the class 'customize_adventure_class_checkbox' and map their values
-   // The values in this case are the class id's for which we need to update the class customizations
-   let classes = new Array();
-   $(".customize_adventure_class_checkbox:checked").each(function () {
-     classes.push($(this).val());
-   });
 
     $.ajax({
       type: 'POST',
@@ -218,7 +212,6 @@ function update_db_adventure(adventure_id: string) {
         name: adventure_name,
         level: level,
         content: content,
-        classes: classes,
         public: agree_public
       }),
       contentType: 'application/json',
@@ -497,9 +490,9 @@ export function enable_level(level: string) {
             start: function() {
               $(this).addClass('flex');
               $(this).removeClass('hidden');
-            }          
-          });         
-        }        
+            }
+          });
+        }
     } else {
         $('.adventure_level_' + level).each(function () {
             $(this).prop("checked", false);
@@ -767,7 +760,7 @@ export function initializeCustomizeClassPage(options: InitializeCustomizeClassPa
 
       drag_list(document.getElementById("sortadventures"));
 
-      $('#adventures').on('change', function(){
+      $('#levels-dropdown').on('change', function(){
           var level = $(this).val() as string;
           $("div.adventures-tab").addClass('hidden').removeClass('flex').removeAttr('style');
           if ($("#disabled").hasClass('flex')) {
@@ -779,12 +772,9 @@ export function initializeCustomizeClassPage(options: InitializeCustomizeClassPa
                   $(this).removeClass('hidden');
               }
           });
-          $('#available').empty();
-          $('#available').append(`<option value="none" selected>${available_adventures_level_translation} ${level}</option>`);
-          const adventures = available_adventures[level];
-          for(let i = 0; i < adventures.length; i++) {
-            $('#available').append(`<option id="remove-${adventures[i]['name']}" value="${adventures[i]['name']}-${level}-${adventures[i]['from_teacher']}">${adventure_names[adventures[i]['name']]}</option>`);
-          }
+
+          addAllAvailableAdventures(level);
+
           drag_list(document.getElementById("level-"+level));
       });
 
@@ -820,7 +810,23 @@ export function initializeCustomizeClassPage(options: InitializeCustomizeClassPa
           drag_list(document.getElementById("level-"+level));
           markUnsavedChanges();
       });
+
+      addAllAvailableAdventures($('#levels-dropdown').val() as string);
   });
+
+  /**
+   * Put all available adventures into the dropdown
+   *
+   * They get removed from this list later on in some way that I don't quite get.
+   */
+  function addAllAvailableAdventures(level: string) {
+    $('#available').empty();
+    $('#available').append(`<option value="none" selected>${available_adventures_level_translation} ${level}</option>`);
+    const adventures = available_adventures[level];
+    for(let i = 0; i < adventures.length; i++) {
+      $('#available').append(`<option id="remove-${adventures[i]['name']}" value="${adventures[i]['name']}-${level}-${adventures[i]['from_teacher']}">${adventure_names[adventures[i]['name']]}</option>`);
+    }
+  }
 }
 
 /**

templates/customize-adventure.html

@@ -46,21 +46,6 @@ <h3 class="text-center mt-0 mb-4">{{ _('adventure') }}</h3>
                         <input type="checkbox" name="agree_public" id="agree_public" class="mr-4" {% if adventure.public %}checked{% endif %}>
                         <label for="agree_public" class="text-sm italic w-full">{{_('adventure_terms')}}</label>
                     </div>
-                    {% if class_data %}
-                        <!-- <div class="flex flex-col items-center">
-                            <h3 class="text-center">{{_('directly_add_adventure_to_classes')}}</h3>
-                            <div class="border-gray-500 border px-4 py-2 rounded-lg">
-                                <div class="flex flex-col items-center gap-2">
-                                    {% for class in class_data %}
-                                        <div class="flex w-full flex-row items-center gap-4 justify-between">
-                                            <label>{{ class.name }}</label>
-                                            <input class="customize_adventure_class_checkbox" type="checkbox" value="{{class.id}}" {% if class.checked %}checked{% endif %}>
-                                        </div>
-                                    {% endfor %}
-                                </div>
-                            </div>
-                        </div> -->
-                    {% endif %}
                 </div>
             </div>
             <div class="flex flex-row justify-end gap-2 my-4">

templates/customize-class.html

@@ -31,7 +31,7 @@ <h3 class="px-4 w-3/12">{{_('select_levels')}}</h3>
         <h3 class="px-4">{{_('select_adventures')}}</h3>
         <div class="flex flex-row w-full mb-4">
             <div class="flex flex-row items-center justify-center mr-2 w-1/12 ml-4">
-                <select class="py-2 px-1 mt-2 h-10 text-center w-full" name="levels" id="adventures" data-cy="adventures">                   
+                <select class="py-2 px-1 mt-2 h-10 text-center w-full" name="levels" id="levels-dropdown" data-cy="adventures">
                     {% for i in range(1, max_level + 1) %}
                         <option id="select-{{i}}" {% if customizations and i not in customizations['levels'] %}
                                                     class="hidden"
@@ -68,7 +68,7 @@ <h3 class="px-4">{{_('select_adventures')}}</h3>
                     {% for level, adventures in adventures_default_order.items() %}
                         <div id="level-{{ level }}" data-cy="level-{{ level }}" {% if loop.index == 1 %}
                                                                                     class="adventures-tab flex"
-                                                                                {% else %}    
+                                                                                {% else %}
                                                                                     class="adventures-tab hidden"
                                                                                 {% endif %}
                                                                                 >

website/database.py

@@ -30,10 +30,45 @@
     dynamo.Index('teacher'),
     dynamo.Index('link'),
 ])
+
+# A custom teacher adventure
+# - id (str): id of the adventure
+# - content (str): adventure text
+# - creator (str): username (of a teacher account, hopefully)
+# - date (int): timestamp of last update
+# - level (int | str): level number, sometimes as an int, sometimes as a str
+# - name (str): adventure name
+# - public (bool): whether it can be shared
 ADVENTURES = dynamo.Table(storage, "adventures", "id", indexes=[dynamo.Index("creator")])
 INVITATIONS = dynamo.Table(
     storage, "class_invitations", partition_key="username", indexes=[dynamo.Index("class_id")]
 )
+
+# Class customizations
+#
+# Various columns with different meanings:
+#
+# These I'm quite sure about:
+#
+# - id (str): the identifier of the class this customization set applies to
+# - levels (int[]): the levels available in this class
+# - opening_dates ({ str -> str }): key is level nr as string, value is an ISO date
+# - other_settings (str[]): string list with values like "hide_quiz", "hide_parsons"
+# - sorted_adventures ({ str -> { from_teacher: bool, name: str }[] }):
+#     for every level (key as string) the adventures to show, in order. If from_teacher
+#     is False, the name of a built-in adventure. If from_teacher is true, name is the
+#     id of a adventure in the ADVENTURES table. The id may refer to an adventure that
+#     has been deleted. In that case, it should be ignored.
+#
+# These not so much:
+#
+# - level_thresholds ({ "quiz" -> int }): TODO don't know what this does
+# - adventures: ({ str -> int[] }): probably a map indicating, for each adventure, what
+#      levels it should be available in. It's sort of redundant with sorted_adventures,
+#      so I'm not sure why it exists.
+# - teacher_adventures (str[]): a list of all ids of the adventures that have been made
+#      available to this class. This list is deprecated, all adventures a teacher created
+#      are now automatically available to all of their classes.
 CUSTOMIZATIONS = dynamo.Table(storage, "class_customizations", partition_key="id")
 ACHIEVEMENTS = dynamo.Table(storage, "achievements", partition_key="username")
 PUBLIC_PROFILES = dynamo.Table(storage, "public_profiles", partition_key="username")
@@ -506,15 +541,13 @@ def get_teacher_students(self, username):
     def get_adventure(self, adventure_id):
         return ADVENTURES.get({"id": adventure_id})
 
+    def batch_get_adventures(self, adventure_ids):
+        """From a list of adventure ids, return a map of { id -> adventure }."""
+        keys = {id: {"id": id} for id in adventure_ids}
+        return ADVENTURES.batch_get(keys) if keys else {}
+
     def delete_adventure(self, adventure_id):
-        # If we delete an adventure -> also delete is from possible class customizations
-        teacher = self.get_adventure(adventure_id).get("creator", "")
         ADVENTURES.delete({"id": adventure_id})
-        for Class in self.get_teacher_classes(teacher, True):
-            customizations = self.get_class_customizations(Class.get("id"))
-            if customizations and adventure_id in customizations.get("teacher_adventures", []):
-                customizations["teacher_adventures"].remove(adventure_id)
-                self.update_class_customizations(customizations)
 
     def store_adventure(self, adventure):
         """Store an adventure."""
@@ -590,26 +623,6 @@ def all_classes(self):
     def delete_class_customizations(self, class_id):
         CUSTOMIZATIONS.delete({"id": class_id})
 
-    def add_adventure_to_class_customizations(self, class_id, adventure_id):
-        customizations = self.get_class_customizations(class_id)
-        if not customizations:
-            customizations = {"id": class_id, "teacher_adventures": [adventure_id]}
-        elif adventure_id not in customizations.get("teacher_adventures", []):
-            customizations["teacher_adventures"] = customizations.get("teacher_adventures", []) + [adventure_id]
-        # If both cases don't return valid the adventure is already in the customizations -> save a PUT operation
-        else:
-            return None
-        CUSTOMIZATIONS.put(customizations)
-
-    def remove_adventure_from_class_customizations(self, class_id, adventure_id):
-        customizations = self.get_class_customizations(class_id)
-        # If there are no customizations, leave as it is -> only perform an action if it is already stored on the class
-        if not customizations:
-            return None
-        elif adventure_id in customizations.get("teacher_adventures", []):
-            customizations["teacher_adventures"].remove(adventure_id)
-            CUSTOMIZATIONS.put(customizations)
-
     def update_class_customizations(self, customizations):
         CUSTOMIZATIONS.put(customizations)
 

website/dynamo.py

@@ -1159,3 +1159,18 @@ def _do_fetch(self):
                                    reverse=self.reverse,
                                    limit=self.limit,
                                    pagination_token=self.pagination_token)
+
+
+class ScanIterator(QueryIterator):
+    """Iterate over a table scan, automatically proceeding to the next result page if necessary.
+
+    Wrapper around scan that automatically paginates.
+    """
+
+    def __init__(self, table, limit=None, pagination_token=None):
+        self.table = table
+        self.limit = limit
+        super().__init__(pagination_token)
+
+    def _do_fetch(self):
+        return self.table.scan(limit=self.limit, pagination_token=self.pagination_token)

website/for_teachers.py

@@ -233,10 +233,6 @@ def get_class_info(self, user, class_id):
         for adventure in teacher_adventures:
             adventure_names[adventure['id']] = adventure['name']
 
-        teacher_adventures_formatted = []
-        for adventure in teacher_adventures:
-            teacher_adventures_formatted.append({"id": adventure['id'], "level": adventure['level']})
-
         available_adventures = {i: [] for i in range(1, hedy.HEDY_MAX_LEVEL+1)}
 
         if customizations:
@@ -273,7 +269,7 @@ def get_class_info(self, user, class_id):
             javascript_page_options=dict(
                 page='customize-class',
                 available_adventures_level_translation=gettext('available_adventures_level'),
-                teacher_adventures=teacher_adventures,
+                teacher_adventures=teacher_adventures_formatted,
                 adventures_default_order=hedy_content.ADVENTURE_ORDER_PER_LEVEL,
                 adventure_names=adventure_names,
                 available_adventures=available_adventures,
@@ -499,8 +495,6 @@ def update_adventure(self, user):
             return gettext("adventure_length"), 400
         if not isinstance(body.get("public"), bool):
             return gettext("public_invalid"), 400
-        if not isinstance(body.get("classes"), list):
-            return gettext("classes_invalid"), 400
 
         current_adventure = self.db.get_adventure(body["id"])
         if not current_adventure or current_adventure["creator"] != user["username"]:
@@ -530,17 +524,6 @@ def update_adventure(self, user):
 
         self.db.update_adventure(body["id"], adventure)
 
-        # Once the adventure is correctly stored we have to update all class customizations
-        # This is once again an expensive operation, we have to retrieve all teacher customizations
-        # Then check if something is changed with the current situation, if so -> update in database
-        Classes = self.db.get_teacher_classes(user["username"])
-        for Class in Classes:
-            # If so, the adventure should be in the class
-            if Class.get("id") in body.get("classes", []):
-                self.db.add_adventure_to_class_customizations(Class.get("id"), body.get("id"))
-            else:
-                self.db.remove_adventure_from_class_customizations(Class.get("id"), body.get("id"))
-
         return {"success": gettext("adventure_updated")}, 200
 
     @route("/customize-adventure/<adventure_id>", methods=["DELETE"])

website/types.py

@@ -85,3 +85,13 @@ class Adventure:
 
     def __getitem__(self, key):
         return getattr(self, key)
+
+    @staticmethod
+    def from_teacher_adventure_database_row(row):
+        return Adventure(
+            short_name=row['id'],
+            name=row['name'],
+            save_name=row['name'],
+            start_code='',  # Teacher adventures don't seem to have this
+            text=row['content'],
+            is_teacher_adventure=True)