Fix exercises staying locked after their unlock date

app/controllers/api/v8/courses/exercises_controller.rb

@@ -37,23 +37,14 @@ def index
           can_see_everything = current_user.administrator? || current_user.teacher?(course.organization) || current_user.assistant?(course)
           authorize! :read, course
 
-          exercises = course.exercises
-          unlocked_exercises = course.unlocks
-                                     .where(user_id: current_user.id)
-                                     .where(['valid_after IS NULL OR valid_after < ?', Time.now])
-                                     .pluck(:exercise_name)
-
-          unless can_see_everything
-            exercises = exercises.where(hidden: false, disabled_status: 0)
-            exercises = if unlocked_exercises.empty?
-              exercises.where(unlock_spec: nil)
-            else
-              exercises.where(["unlock_spec IS NULL OR exercises.name IN (#{unlocked_exercises.map { |_| '?' }.join(', ')})", *unlocked_exercises])
-            end
+          if can_see_everything
+            unlocked_ids = course.exercises.pluck(:id).to_set
+          else
+            exercises = course.exercises.where(hidden: false, disabled_status: 0).to_a
+            unlocked_names = course.unlocked_exercise_names_for(current_user, exercises)
+            unlocked_ids = exercises.select { |ex| unlocked_names.include?(ex.name) }.to_set(&:id)
           end
 
-          exercises = exercises.pluck(:id)
-
           all_exercises = course.exercises.includes(:available_points).where(disabled_status: 0)
           unless can_see_everything
             all_exercises = course.exercises
@@ -79,7 +70,7 @@ def index
               deadline: ex.deadline_for(current_user),
               soft_deadline: ex.soft_deadline_for(current_user),
               disabled: ex.disabled?,
-              unlocked: exercises.include?(ex.id)
+              unlocked: unlocked_ids.include?(ex.id)
             }
           end
 

app/controllers/courses_controller.rb

@@ -155,10 +155,14 @@ def manage_unlocks
   def save_unlocks
     authorize! :manage_unlocks, @course
 
-    groups = group_params
-    groups.each do |name, conditions|
-      array = conditions.values.reject(&:blank?)
-      @course.exercise_group_by_name(name).group_unlock_conditions = array.to_json
+    begin
+      group_params.each do |name, conditions|
+        array = conditions.values.reject(&:blank?)
+        group = @course.exercise_group_by_name(name)
+        Rails.logger.info "Unlock conditions of #{@course.name}/#{name} set by user #{current_user.id}: #{group.exercises(false).first&.unlock_spec || '[]'} -> #{array.to_json}"
+        group.group_unlock_conditions = array.to_json
+      end
+    ensure
       UncomputedUnlock.create_all_for_course(@course)
     end
 
@@ -196,7 +200,7 @@ def assign_show_view_vars
       @exercises = @exercises.select { |ex| ex.visible_to?(current_user) }
                              .natsort_by(&:name)
       @exercise_completion_status = ExerciseCompletionStatusGenerator.completion_status(current_user, @course)
-      @unlocks = current_user.unlocks.where(course: @course).where('valid_after IS NULL OR valid_after < ?', Time.zone.now).pluck(:exercise_name)
+      @unlocks = @course.unlocked_exercise_names_for(current_user, @exercises)
 
       if can?(:teach, @course)
         last_refresh = @course.course_template.course_template_refreshes.last

app/models/course.rb

@@ -356,6 +356,21 @@ def exercises_by_name_or_group(name, force_reload = false)
     exercises.to_a.select { |ex| ex.name == name || (group && ex.belongs_to_exercise_group?(group)) }
   end
 
+  # Set of names of the exercises in `exercises_to_check` the user may open, including those that need no unlock.
+  # Batched form of Exercise#unlocked_for? without resolving pending unlocks.
+  def unlocked_exercise_names_for(user, exercises_to_check)
+    from_rows = unlocks.where(user_id: user.id).where('valid_after IS NULL OR valid_after < ?', Time.now).pluck(:exercise_name).to_set
+    exercises_to_check.select do |ex|
+      if !ex.requires_unlock?
+        true
+      elsif ex.unlocked_by_date?
+        ex.unlock_time_by_date
+      else
+        from_rows.include?(ex.name)
+      end
+    end.to_set(&:name)
+  end
+
   def unlockable_exercises_for(user)
     UncomputedUnlock.resolve(self, user)
     unlocked = unlocks.where(user_id: user.id).pluck(:exercise_name)

app/models/exercise.rb

@@ -353,11 +353,26 @@ def requires_explicit_unlock?
     deadline_spec_obj.depends_on_unlock_time?
   end
 
+  # Date-only unlocks are decided by the clock, not by unlock rows: a user without rows must not stay locked.
+  # Explicit-unlock exercises are excluded because their unlock row starts the deadline timer.
+  def unlocked_by_date?
+    UnlockSpec.date_only?(unlock_spec) && !requires_explicit_unlock?
+  end
+
+  # Returns when the exercise unlocked for the user, or nil if it is locked or needs no unlock.
   def time_unlocked_for(user, resolve_unlocks = true)
+    return unlock_time_by_date if unlocked_by_date?
+
     UncomputedUnlock.resolve(course, user) if resolve_unlocks
     unlocks.where(user_id: user).find_by('valid_after IS NULL OR valid_after < ?', Time.now)&.created_at
   end
 
+  # The unlock date of a date-only exercise once it has passed; nil before it.
+  def unlock_time_by_date
+    unlock_date = unlock_spec_obj.valid_after
+    unlock_date <= Time.now ? unlock_date : nil
+  end
+
   def unlocked_for?(user, resolve_unlocks = true)
     res = !requires_unlock? || time_unlocked_for(user, resolve_unlocks)
     !!res

app/models/exercise_group.rb

@@ -81,11 +81,22 @@ def group_unlock_conditions
     exercises(false).map(&:unlock_conditions).first
   end
 
+  # The group's exercises keyed by raw unlock spec (nil for none); more than one key means
+  # `group_unlock_conditions` shows only the first exercise's conditions.
+  def exercises_by_unlock_spec
+    exercises(false).group_by(&:unlock_spec)
+  end
+
   def group_unlock_conditions=(unlock_conditions)
-    exercises(false).each do |e|
+    group_exercises = exercises(false)
+    # Rows granted under other conditions keep their date: fast refresh never revokes them, so moving it would open them early.
+    conditions = UnlockSpec.non_date_conditions(unlock_conditions).sort
+    rows_to_move = group_exercises.select { |e| UnlockSpec.non_date_conditions(e.unlock_spec).sort == conditions }.map(&:name)
+    group_exercises.each do |e|
       e.unlock_spec = unlock_conditions
       e.save!
     end
+    update_unlock_dates(group_exercises.first, rows_to_move) unless group_exercises.empty?
   end
 
   def available_point_names
@@ -107,6 +118,13 @@ def available_point_names
   end
 
   private
+    # Unlock rows copy the date at creation; keep existing rows in step when the date is edited.
+    def update_unlock_dates(exercise, exercise_names)
+      spec = exercise.unlock_spec_obj
+      return if spec.empty?
+      Unlock.where(course_id: course.id, exercise_name: exercise_names).update_all(valid_after: spec.valid_after)
+    end
+
     def group_deadline(method)
       exercises(false).map { |n| n.send(method) }.first
     end

app/models/uncomputed_unlock.rb

@@ -4,8 +4,8 @@
 
 # Keeps track of whether a (course, user)'s unlocks have yet to be computed, enabling lazy computation.
 # A course refresh populates this table and the UnlockComputerTask background task
-# consumes it by calling Unlock.refresh_unlocks on the entries. All code that accesses
-# unlocks must ensure that UncomputedUnlock.resolve is called for the (course, user) first.
+# consumes it by calling Unlock.refresh_unlocks on the entries. Code that needs
+# up-to-date unlocks should call UncomputedUnlock.resolve for the (course, user) first.
 class UncomputedUnlock < ApplicationRecord
   belongs_to :course
   belongs_to :user

app/models/unlock.rb

@@ -4,9 +4,11 @@
 # Stores whether and when an exercise has been unlocked for an user.
 #
 # An exercise is considered unlocked for a user if an unlock for
-# (user, course, exercise, valid_after < Time.now) exists.
+# (user, course, exercise, valid_after < Time.now) exists, except that exercises
+# whose only condition is a date are unlocked by the clock (Exercise#unlocked_by_date?)
+# and need no unlock.
 # The set of unlocks for a (course, user) is recomputed in the following cases:
-# - the course is refreshed (since the specs might change)
+# - the course is refreshed or unlock conditions are saved, for users with awarded points
 # - the user has a submission graded
 #
 # However, if the course has deadlines based on the unlock times of exercises,
@@ -84,6 +86,6 @@ def self.fast_refresh_unlocks_impl(course, user, user_unlocks_by_exercise_name)
           )
         end
       end
-      Unlock.import new_unlocks
+      Unlock.import new_unlocks, on_duplicate_key_ignore: true
     end
 end

app/models/unlock_spec.rb

@@ -31,6 +31,24 @@ def empty? # No unlock conditions - no Unlock object required to be unlocked
     @conditions.empty? && @valid_after.nil?
   end
 
+  # Whether the raw spec is only a date, so the clock alone decides the unlock.
+  # Reads the JSON without parsing the spec, since parsing a progress condition queries the course's exercises.
+  def self.date_only?(string)
+    conditions = raw_conditions(string)
+    conditions.any? && conditions.all? { |c| DateAndTimeUtils.looks_like_date_or_time(c) }
+  end
+
+  # The raw spec's conditions other than dates. Works on specs that no longer parse.
+  def self.non_date_conditions(string)
+    raw_conditions(string).reject { |c| DateAndTimeUtils.looks_like_date_or_time(c) }
+  end
+
+  def self.raw_conditions(string)
+    return [] if string.nil?
+    ActiveSupport::JSON.decode(string).map { |c| c.to_s.strip }.reject(&:empty?)
+  end
+  private_class_method :raw_conditions
+
   attr_reader :raw_spec
   attr_reader :valid_after
   attr_reader :universal_descriptions

app/views/courses/manage_unlocks.html.erb

@@ -75,6 +75,15 @@
                  class='btn btn-sm btn-link remove-condition-button'>remove</a><br/>
             <% end %>
           </div>
+          <% exercises_by_spec = group.exercises_by_unlock_spec %>
+          <% if exercises_by_spec.length > 1 %>
+            <p class="text-danger">
+              The exercises in this group have different unlock conditions; saving sets them all to the ones shown.
+              <% exercises_by_spec.each do |spec, exercises| %>
+                <br/><%= spec || 'none' %>: <%= exercises.map(&:name).join(', ') %>
+              <% end %>
+            </p>
+          <% end %>
           <a id="add-condition-<%= group.name %>"
              href="javascript:void(0);"
              onclick="add_unlock_condition('#unlock-conditions-<%= group.name %>', '<%= param_array %>')"

docker-compose.yml

@@ -1,7 +1,7 @@
 version: '2'
 services:
   db:
-    image: postgres:9.4
+    image: postgres:16
     environment: 
       POSTGRES_HOST_AUTH_METHOD: trust
     volumes:

lib/course_info.rb

@@ -15,10 +15,7 @@ def initialize(user, helpers)
   def course_data(organization, course, opts = {})
     exercises = course.exercises.includes(:course, :available_points).to_a.natsort_by(&:name)
 
-    @unlocked_exercises = course.unlocks
-                                .where(user_id: @user.id)
-                                .where(['valid_after IS NULL OR valid_after < ?', Time.now])
-                                .pluck(:exercise_name)
+    @unlocked_exercises = course.unlocked_exercise_names_for(@user, exercises)
 
     submissions_by_exercise = {}
     Submission.where(course_id: course.id, user_id: @user.id).each do |sub|
@@ -35,23 +32,16 @@ def course_data(organization, course, opts = {})
 
   def course_data_core_api(course)
     # UncomputedUnlock.resolve(course, @user)
-    @unlocked_exercises = course.unlocks
-                                .where(user_id: @user.id)
-                                .where(['valid_after IS NULL OR valid_after < ?', Time.now])
-                                .pluck(:exercise_name)
-
     exercises = course.exercises.includes(:course, :available_points)
-
-    unless @user.administrator? || @user.teacher?(course.organization) || @user.assistant?(course)
-      exercises = exercises.where(hidden: false, disabled_status: 0)
-      exercises = if @unlocked_exercises.empty?
-        exercises.where(unlock_spec: nil)
-      else
-        exercises.where(["unlock_spec IS NULL OR exercises.name IN (#{@unlocked_exercises.map { |_| '?' }.join(', ')})", *@unlocked_exercises])
-      end.select(&:_fast_visible?)
+    can_see_everything = @user.administrator? || @user.teacher?(course.organization) || @user.assistant?(course)
+    exercises = exercises.where(hidden: false, disabled_status: 0) unless can_see_everything
+    exercises = exercises.to_a
+    @unlocked_exercises = course.unlocked_exercise_names_for(@user, exercises)
+    unless can_see_everything
+      exercises = exercises.select { |ex| @unlocked_exercises.include?(ex.name) && ex._fast_visible? }
     end
 
-    exercises = exercises.to_a.natsort_by(&:name)
+    exercises = exercises.natsort_by(&:name)
 
     # Cache submissions so that we can calculate quickly whether the exercise
     # was completed or attampted and so on...
@@ -80,7 +70,7 @@ def exercise_data(exercise)
       return nil unless exercise.visible_to?(@user)
 
       # optimization: use @unlocked_exercises to avoid querying unlocks repeatedly
-      locked = exercise.requires_unlock? && !@unlocked_exercises.include?(exercise.name)
+      locked = !@unlocked_exercises.include?(exercise.name)
       show_points = !exercise.hide_submission_results? && !exercise.course.hide_submission_results?
       attempted = exercise.attempted_by?(@user)
 
@@ -118,7 +108,7 @@ def exercise_data(exercise)
 
     def exercise_data_core_api(exercise)
       # optimization: use @unlocked_exercises to avoid querying unlocks repeatedly
-      locked = exercise.requires_unlock? && !@unlocked_exercises.include?(exercise.name)
+      locked = !@unlocked_exercises.include?(exercise.name)
       show_points = !exercise.hide_submission_results? && !exercise.course.hide_submission_results?
       attempted = exercise.attempted_by?(@user)
 

lib/course_refresh_database_updater.rb

@@ -107,13 +107,27 @@ def delete_records_for_removed_exercises
       end
 
       def set_exercise_options
+        specs_by_group = @course.exercises.reject(&:new_record?).group_by(&:exercise_group_name).transform_values { |es| es.map(&:unlock_spec).uniq }
         @course.exercises.each do |e|
           e.has_tests = true # we don't yet detect whether an exercise includes tests
           e.options = {} # exercise metadata.yml reading support removed, set default options
+          inherit_group_unlock_spec(e, specs_by_group[e.exercise_group_name] || []) if e.new_record?
           e.disabled! if e.new_record? && e.course.refreshed? # disable new exercises
         end
       end
 
+      # Without this a new exercise in a locked group is open to everyone until a teacher saves the group's conditions again.
+      def inherit_group_unlock_spec(exercise, specs)
+        if specs.length > 1
+          @report.warnings << "Exercise #{exercise.name} was added to a group whose exercises have different unlock conditions; set them on the manage unlocks page"
+        elsif specs.first
+          exercise.unlock_spec = specs.first
+          @report.notices << "Exercise #{exercise.name} inherited the unlock conditions of its group"
+        end
+      rescue UnlockSpec::InvalidSyntaxError, RuntimeError => e
+        @report.warnings << "Exercise #{exercise.name} could not inherit the unlock conditions of its group: #{e.message}"
+      end
+
       def set_docker_image
         default_image = Exercise.new.docker_image
         @rust_data['exercises'].each do |exercise|

spec/controllers/api/v8/core/courses_controller_spec.rb

@@ -78,6 +78,22 @@ def show_course
         expect(exs[1]['completed']).to be_falsey
       end
 
+      describe 'with a date-only unlock and no unlock rows' do
+        it 'lists the exercise as unlocked after the date' do
+          course.exercises[0].update!(unlock_spec: ['1.1.2011'].to_json)
+
+          ex = show_course['course']['exercises'].find { |e| e['name'] == 'Exercise1' }
+          expect(ex['locked']).to be false
+        end
+
+        it 'does not list the exercise before the date' do
+          course.exercises[0].update!(unlock_spec: [(Time.zone.now + 1.day).strftime('%d.%m.%Y %H:%M')].to_json)
+
+          names = show_course['course']['exercises'].map { |e| e['name'] }
+          expect(names).not_to include('Exercise1')
+        end
+      end
+
       describe 'and as guest user' do
         it 'should respond with a 401' do
           controller.current_user = Guest.new

spec/controllers/api/v8/courses/exercises_controller_spec.rb

@@ -50,6 +50,25 @@
     end
   end
 
+  describe 'As a student without unlock rows' do
+    let(:token) { double resource_owner_id: user.id, acceptable?: true }
+
+    def unlocked_flags
+      get :index, params: { course_id: course.id }
+      JSON.parse(response.body).to_h { |ex| [ex['name'], ex['unlocked']] }
+    end
+
+    it 'marks a date-unlocked exercise as unlocked once the date has passed' do
+      exercise.update!(unlock_spec: ['1.1.2011'].to_json)
+      expect(unlocked_flags['testexercise']).to be true
+    end
+
+    it 'marks a date-unlocked exercise as locked before the date' do
+      exercise.update!(unlock_spec: [(Time.zone.now + 1.day).strftime('%d.%m.%Y %H:%M')].to_json)
+      expect(unlocked_flags['testexercise']).to be false
+    end
+  end
+
   describe 'As a guest' do
     let(:token) { nil }
     describe 'when searching for exercises' do