diff --git a/.circleci/config.yml b/.circleci/config.yml index 7aa9a9ab610..343d5901a77 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -288,12 +288,12 @@ jobs: - build_and_run_auth_server # Replace both archive and security repositories - # https://support.circleci.com/hc/en-us/articles/37474192881179-Resolving-Unable-to-connect-to-archive-ubuntu-com-Error-in-CircleCI + # https://discuss.circleci.com/t/connection-issues-with-apt-get-from-archive-ubuntu-com/48094/16 - run: name: Change Ubuntu archive mirrors command: | - sudo sed -i 's|http://archive.ubuntu.com|http://mirrors.rit.edu|g' /etc/apt/sources.list - sudo sed -i 's|http://security.ubuntu.com|http://mirrors.rit.edu|g' /etc/apt/sources.list + sudo sed -i 's|http://archive.ubuntu.com|https://mirror.math.princeton.edu/pub|g' /etc/apt/sources.list + sudo sed -i 's|http://security.ubuntu.com|https://mirror.math.princeton.edu/pub|g' /etc/apt/sources.list - run: name: Install Playwright dependencies diff --git a/Gemfile b/Gemfile index 89506d4836f..fcf9eb3b1b2 100644 --- a/Gemfile +++ b/Gemfile @@ -11,7 +11,7 @@ gem 'csv' # For Windows devs gem 'tzinfo-data', platforms: [:mswin, :mswin64] -gem 'rails', '~> 8.0.0' +gem 'rails', '~> 8.1.0' # Use PostgreSQL for the backend gem 'pg' @@ -32,9 +32,10 @@ gem 'workflow-activerecord', '>= 4.1', '< 7.0' # Add creator_id and updater_id attributes to models gem 'activerecord-userstamp', git: 'https://github.com/Coursemology/activerecord-userstamp.git' # Allow declaring the calculated attributes of a record -# Upstream v1.1.1 (supports Rails 7.0/7.1/7.2/8.0); retires the Coursemology fork. -# TODO: bump to upstream master (v1.2.0) for Rails 8.1 during that upgrade — v1.1.1 ships no 8.1 patch. -gem 'calculated_attributes', git: 'https://github.com/aha-app/calculated_attributes.git', ref: 'ecaf6c9' +# Upstream v1.2.0 — first ref shipping rails_8_1_patches.rb +# NOTE: v1.2.0 also carries a broken Relation#calculate override — neutralised +# in config/initializers/calculated_attributes_patch.rb; read that before bumping this ref again. +gem 'calculated_attributes', git: 'https://github.com/aha-app/calculated_attributes.git', ref: '992fdd9' # For multiple table inheritance # TODO: Figure out breaking changes in v2 as polymorphism is not working correctly. gem 'active_record-acts_as', git: 'https://github.com/Coursemology/active_record-acts_as.git' diff --git a/Gemfile.lock b/Gemfile.lock index 11c81f506a1..0fafda86d04 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -35,11 +35,11 @@ GIT GIT remote: https://github.com/aha-app/calculated_attributes.git - revision: ecaf6c95c8d097e58e2660e84e131df4aed16600 - ref: ecaf6c9 + revision: 992fdd9785d201e8e9ad7d6c2f22659ba1cc1c81 + ref: 992fdd9 specs: - calculated_attributes (1.1.1) - activerecord (>= 7.0.0, < 9) + calculated_attributes (1.2.0) + activerecord (>= 8.0.0, < 9) GIT remote: https://github.com/jesjos/active_record_upsert @@ -62,29 +62,31 @@ GEM remote: https://rubygems.org/ specs: Ascii85 (2.0.1) - actioncable (8.0.5.1) - actionpack (= 8.0.5.1) - activesupport (= 8.0.5.1) + action_text-trix (2.1.19) + railties + actioncable (8.1.3.1) + actionpack (= 8.1.3.1) + activesupport (= 8.1.3.1) nio4r (~> 2.0) websocket-driver (>= 0.6.1) zeitwerk (~> 2.6) - actionmailbox (8.0.5.1) - actionpack (= 8.0.5.1) - activejob (= 8.0.5.1) - activerecord (= 8.0.5.1) - activestorage (= 8.0.5.1) - activesupport (= 8.0.5.1) + actionmailbox (8.1.3.1) + actionpack (= 8.1.3.1) + activejob (= 8.1.3.1) + activerecord (= 8.1.3.1) + activestorage (= 8.1.3.1) + activesupport (= 8.1.3.1) mail (>= 2.8.0) - actionmailer (8.0.5.1) - actionpack (= 8.0.5.1) - actionview (= 8.0.5.1) - activejob (= 8.0.5.1) - activesupport (= 8.0.5.1) + actionmailer (8.1.3.1) + actionpack (= 8.1.3.1) + actionview (= 8.1.3.1) + activejob (= 8.1.3.1) + activesupport (= 8.1.3.1) mail (>= 2.8.0) rails-dom-testing (~> 2.2) - actionpack (8.0.5.1) - actionview (= 8.0.5.1) - activesupport (= 8.0.5.1) + actionpack (8.1.3.1) + actionview (= 8.1.3.1) + activesupport (= 8.1.3.1) nokogiri (>= 1.8.5) rack (>= 2.2.4) rack-session (>= 1.0.1) @@ -92,44 +94,45 @@ GEM rails-dom-testing (~> 2.2) rails-html-sanitizer (~> 1.6) useragent (~> 0.16) - actiontext (8.0.5.1) - actionpack (= 8.0.5.1) - activerecord (= 8.0.5.1) - activestorage (= 8.0.5.1) - activesupport (= 8.0.5.1) + actiontext (8.1.3.1) + action_text-trix (~> 2.1.15) + actionpack (= 8.1.3.1) + activerecord (= 8.1.3.1) + activestorage (= 8.1.3.1) + activesupport (= 8.1.3.1) globalid (>= 0.6.0) nokogiri (>= 1.8.5) - actionview (8.0.5.1) - activesupport (= 8.0.5.1) + actionview (8.1.3.1) + activesupport (= 8.1.3.1) builder (~> 3.1) erubi (~> 1.11) rails-dom-testing (~> 2.2) rails-html-sanitizer (~> 1.6) - activejob (8.0.5.1) - activesupport (= 8.0.5.1) + activejob (8.1.3.1) + activesupport (= 8.1.3.1) globalid (>= 0.3.6) - activemodel (8.0.5.1) - activesupport (= 8.0.5.1) - activerecord (8.0.5.1) - activemodel (= 8.0.5.1) - activesupport (= 8.0.5.1) + activemodel (8.1.3.1) + activesupport (= 8.1.3.1) + activerecord (8.1.3.1) + activemodel (= 8.1.3.1) + activesupport (= 8.1.3.1) timeout (>= 0.4.0) activerecord-import (2.2.0) activerecord (>= 4.2) - activestorage (8.0.5.1) - actionpack (= 8.0.5.1) - activejob (= 8.0.5.1) - activerecord (= 8.0.5.1) - activesupport (= 8.0.5.1) + activestorage (8.1.3.1) + actionpack (= 8.1.3.1) + activejob (= 8.1.3.1) + activerecord (= 8.1.3.1) + activesupport (= 8.1.3.1) marcel (~> 1.0) - activesupport (8.0.5.1) + activesupport (8.1.3.1) base64 - benchmark (>= 0.3) bigdecimal concurrent-ruby (~> 1.0, >= 1.3.1) connection_pool (>= 2.2.5) drb i18n (>= 1.6, < 2) + json logger (>= 1.4.2) minitest (>= 5.1) securerandom (>= 0.3) @@ -166,7 +169,6 @@ GEM baran (0.1.12) base64 (0.3.0) bcrypt (3.1.22) - benchmark (0.5.0) bigdecimal (3.3.1) builder (3.3.0) bullet (8.1.3) @@ -235,7 +237,7 @@ GEM htmlentities (~> 4.3) launchy (>= 2.1, < 4.0) mail (~> 2.7) - erb (6.0.6) + erb (6.0.7) erubi (1.13.1) et-orbi (1.2.11) tzinfo @@ -311,7 +313,7 @@ GEM image_processing (1.14.0) mini_magick (>= 4.9.5, < 6) ruby-vips (>= 2.0.17, < 3) - io-console (0.8.2) + io-console (0.9.2) irb (1.18.0) pp (>= 0.6.0) prism (>= 1.3.0) @@ -428,7 +430,7 @@ GEM nio4r (~> 2.0) raabro (1.4.0) racc (1.8.1) - rack (3.2.6) + rack (3.2.7) rack-cors (3.0.0) logger rack (>= 3.0.14) @@ -441,20 +443,20 @@ GEM rack (>= 1.3) rackup (2.3.1) rack (>= 3) - rails (8.0.5.1) - actioncable (= 8.0.5.1) - actionmailbox (= 8.0.5.1) - actionmailer (= 8.0.5.1) - actionpack (= 8.0.5.1) - actiontext (= 8.0.5.1) - actionview (= 8.0.5.1) - activejob (= 8.0.5.1) - activemodel (= 8.0.5.1) - activerecord (= 8.0.5.1) - activestorage (= 8.0.5.1) - activesupport (= 8.0.5.1) + rails (8.1.3.1) + actioncable (= 8.1.3.1) + actionmailbox (= 8.1.3.1) + actionmailer (= 8.1.3.1) + actionpack (= 8.1.3.1) + actiontext (= 8.1.3.1) + actionview (= 8.1.3.1) + activejob (= 8.1.3.1) + activemodel (= 8.1.3.1) + activerecord (= 8.1.3.1) + activestorage (= 8.1.3.1) + activesupport (= 8.1.3.1) bundler (>= 1.15.0) - railties (= 8.0.5.1) + railties (= 8.1.3.1) rails-controller-testing (1.0.5) actionpack (>= 5.0.1.rc1) actionview (>= 5.0.1.rc1) @@ -469,9 +471,9 @@ GEM rails-i18n (8.1.0) i18n (>= 0.7, < 2) railties (>= 8.0.0, < 9) - railties (8.0.5.1) - actionpack (= 8.0.5.1) - activesupport (= 8.0.5.1) + railties (8.1.3.1) + actionpack (= 8.1.3.1) + activesupport (= 8.1.3.1) irb (~> 1.13) rackup (>= 1.0.0) rake (>= 12.2) @@ -483,7 +485,7 @@ GEM rb-fsevent (0.11.2) rb-inotify (0.11.1) ffi (~> 1.0) - rbs (4.1.1) + rbs (4.2.0) logger prism (>= 1.6.0) tsort @@ -500,7 +502,7 @@ GEM redis-client (0.28.0) connection_pool regexp_parser (2.12.0) - reline (0.6.3) + reline (0.7.0) io-console (~> 0.5) request_store (1.7.0) rack (>= 1.4) @@ -642,7 +644,7 @@ GEM nokogiri (~> 1.8) yajl-ruby (1.4.3) yard (0.9.45) - zeitwerk (2.8.2) + zeitwerk (2.8.3) PLATFORMS ruby @@ -706,7 +708,7 @@ DEPENDENCIES puma (~> 7.2) rack-cors rack-mini-profiler - rails (~> 8.0.0) + rails (~> 8.1.0) rails-controller-testing rails-html-sanitizer (>= 1.7.1) recaptcha diff --git a/app/controllers/user/registrations_controller.rb b/app/controllers/user/registrations_controller.rb index 6212db552d0..38c9e0fee58 100644 --- a/app/controllers/user/registrations_controller.rb +++ b/app/controllers/user/registrations_controller.rb @@ -43,16 +43,27 @@ def create return end + # Validate the enrolment target *before* +super+. Devise's +create+ renders via +respond_with+, + # and since Rails 8.1 +head+ raises +AbstractController::DoubleRenderError+ when a response body + # has already been set, so these guards can no longer run after it. + # + # Checking first is also correct on its own terms: a non-local +return+ out of a transaction + # block commits the transaction (Rails 7.1+), so the previous ordering created the user account + # and *then* returned 404/403, leaving an orphaned registration behind. + enrol_course = nil + enrol_course_id = invitation_params[:enrol_course_id].presence + if enrol_course_id + enrol_course = Course.find_by(id: enrol_course_id) + return head :not_found unless enrol_course + + # this endpoint is accessible to unauthenticated users, so authorize! isn't used + return head :forbidden unless enrol_course.published && enrol_course.enrollable + end + User.transaction do super - if resource.persisted? && invitation_params[:enrol_course_id] - enrol_course = Course.find_by(id: invitation_params[:enrol_course_id]) - head :not_found and return unless enrol_course - - # this endpoint is accessible to unauthenticated users, so authorize! isn't used - head :forbidden and return unless enrol_course.published && enrol_course.enrollable - + if resource.persisted? && enrol_course @enrol_request = Course::EnrolRequest.create!( user: @user, course_id: invitation_params[:enrol_course_id], diff --git a/app/models/application_record.rb b/app/models/application_record.rb index ebcf5973ea6..1397bb857ea 100644 --- a/app/models/application_record.rb +++ b/app/models/application_record.rb @@ -4,4 +4,58 @@ class ApplicationRecord < ActiveRecord::Base include ApplicationUserstampConcern include ApplicationActsAsConcern + + # Builds the raw SQL expression behind a `calculated_attributes` attribute, for use in `order`. + # + # Ordering by the *alias* (`order('average_achievement_count DESC')`) only works while the + # aliased projection survives in the relation's select list. That is not guaranteed: + # `calculated_attributes` v1.2.0 wipes `select_values` in place inside `Relation#calculate`, so + # any `count` on the relation (`each_cons` and Bullet both trigger one) leaves an ORDER BY + # referencing a column that is no longer selected — `PG::UndefinedColumn`. + # + # Ordering by the expression instead is self-contained, and costs nothing: PostgreSQL emits an + # identical plan either way, reusing the same `SubPlan` for the select-list and sort-key copies + # (verified with EXPLAIN — byte-identical plans). + # + # Derives the expression from the same lambda that defines the projection, so the two cannot + # drift apart. + # + # Mirrors the resolution and normalisation that `calculated_attributes` itself performs in + # `ActiveRecord::Relation#calculated` (`model_methods.rb`), so the ORDER BY expression and the + # SELECT projection are always built the same way: + # - falls back to `base_class` for MTI/STI models, as the gem does; + # - forwards `*args`, since attributes may be parameterised + # (e.g. `calculated :topic_unread_count, ->(user) { ... }` in `Course::Forum`); + # - accepts every shape a lambda may return — a relation, a raw SQL string, a + # `sanitize_sql` array, or a bare Arel node. + # + # @param [Symbol] attribute The calculated attribute name. + # @param [Array] args Arguments for a parameterised attribute, forwarded to its lambda. + # @return [Arel::Nodes::Node] An expression suitable for `order`. + # @raise [ArgumentError] If the attribute is not defined on this model or its base class. + def self.calculated_expression(attribute, *) + calculated_expression_to_arel(calculated_attribute_lambda(attribute).call(*)) + end + + # Resolves a calculated attribute's lambda, falling back to +base_class+ for MTI/STI models the + # way the gem does. Fails loudly rather than letting a typo surface as +NoMethodError+ on +nil+. + def self.calculated_attribute_lambda(attribute) + calculated.calculated[attribute] || base_class.calculated.calculated[attribute] || + raise(ArgumentError, "#{name} has no calculated attribute #{attribute.inspect}") + end + private_class_method :calculated_attribute_lambda + + # Normalises whatever a calculated lambda returns into an Arel expression, matching the gem's own + # handling: a +sanitize_sql+ array, a raw SQL string, a relation, or an Arel node. + def self.calculated_expression_to_arel(sql) + # NB: the array is passed whole, not splatted. The gem's own copy of this does + # `send(:sanitize_sql, *sql)`, which hands `['... ?', value]` to a one-argument method and + # raises. `sanitize_sql(condition)` expects the array itself. + sql = send(:sanitize_sql, sql) if sql.is_a?(Array) + return Arel.sql("(#{sql})") if sql.is_a?(String) + return Arel.sql("(#{sql.to_sql})") if sql.respond_to?(:to_sql) + + sql + end + private_class_method :calculated_expression_to_arel end diff --git a/app/models/course/group.rb b/app/models/course/group.rb index cbefd595e75..b1afcd7e1bd 100644 --- a/app/models/course/group.rb +++ b/app/models/course/group.rb @@ -61,7 +61,8 @@ class Course::Group < ApplicationRecord end) scope :ordered_by_experience_points, (lambda do - all.calculated(:average_experience_points).order('average_experience_points DESC') + all.calculated(:average_experience_points). + order(calculated_expression(:average_experience_points).desc) end) # Order course_users by achievement count for use in the group leaderboard. @@ -69,7 +70,8 @@ class Course::Group < ApplicationRecord # obtained the current achievement count first. scope :ordered_by_average_achievement_count, (lambda do all.calculated(:average_achievement_count, :last_obtained_achievement). - order('average_achievement_count DESC, last_obtained_achievement ASC') + order(calculated_expression(:average_achievement_count).desc, + calculated_expression(:last_obtained_achievement).asc) end) scope :ordered_by_name, -> { order(name: :asc) } diff --git a/app/models/course_user.rb b/app/models/course_user.rb index 6682fb1fac4..ae1642798ab 100644 --- a/app/models/course_user.rb +++ b/app/models/course_user.rb @@ -147,7 +147,8 @@ class CourseUser < ApplicationRecord # obtained the current experience points first. scope :ordered_by_experience_points, (lambda do all.calculated(:experience_points, :last_experience_points_record). - order('experience_points DESC, last_experience_points_record ASC') + order(calculated_expression(:experience_points).desc, + calculated_expression(:last_experience_points_record).asc) end) # Order course_users by achievement count for use in the course leaderboard. @@ -155,7 +156,8 @@ class CourseUser < ApplicationRecord # obtained the current achievement count first. scope :ordered_by_achievement_count, (lambda do all.calculated(:achievement_count, :last_obtained_achievement). - order('achievement_count DESC, last_obtained_achievement ASC') + order(calculated_expression(:achievement_count).desc, + calculated_expression(:last_obtained_achievement).asc) end) scope :order_alphabetically, ->(direction = :asc) { order(name: direction) } diff --git a/config/application.rb b/config/application.rb index 5e9fd98dcd3..e93ff25dfda 100644 --- a/config/application.rb +++ b/config/application.rb @@ -16,7 +16,7 @@ module Application # rubocop:disable Style/ClassAndModuleChildren class Application < Rails::Application # Initialize configuration defaults for originally generated Rails version. - config.load_defaults 8.0 + config.load_defaults 8.1 # Settings in config/environments/* take precedence over those specified here. # Application configuration can go into files in config/initializers diff --git a/config/initializers/calculated_attributes_patch.rb b/config/initializers/calculated_attributes_patch.rb new file mode 100644 index 00000000000..b375f24d55d --- /dev/null +++ b/config/initializers/calculated_attributes_patch.rb @@ -0,0 +1,67 @@ +# frozen_string_literal: true +# Neutralises an upstream regression in `calculated_attributes` v1.2.0. +# +# ## Why this exists +# +# The Rails 8.1 hop required bumping `calculated_attributes` to v1.2.0 (`992fdd9`) — the first ref +# shipping `rails_8_1_patches.rb`. The gem requires its patch file by exact Rails version +# (`rails_#{MAJOR}_#{MINOR}_patches`), so any older ref raises `LoadError` at boot on 8.1. +# +# That bump also brings commit 9cfea61 ("Support .count on relations that have previously had +# `calculated` applied", 2026-05-14), which adds an override of `ActiveRecord::Relation#calculate`: +# +# module ActiveRecord +# class Relation +# def calculated(*args) ... end +# +# private # <- (1) +# +# def calculate(operation, column_name) +# self.select_values = [] if select_values.any? { |p| ... p.calculated_attr? } # <- (2) +# super +# end +# end +# end +# +# It breaks us in two independent ways: +# +# 1. **Wrong visibility → infinite recursion.** `calculate` is public API on Relation, and +# `ActiveRecord::Querying` delegates it to the relation (`delegate :calculate, ..., to: :all`). +# Demoting it to private means the delegation cannot dispatch, so the call falls through to +# `Relation#method_missing`, which delegates straight back to the class: +# +# Querying#calculate -> all.calculate -> method_missing -> klass.public_send(:calculate) +# -> Querying#calculate -> ... => SystemStackError +# +# 2. **Clearing `select_values` breaks `ORDER BY `.** Even with the visibility +# fixed, wiping the select list drops the calculated projection while the ORDER BY still +# references its alias, producing `PG::UndefinedColumn`. The wipe is done *in place* +# (`self.select_values = []`), so it corrupts the relation for later use — the query that +# actually failed was not the `count` but the record load that followed it. +# +# **This second break is no longer load-bearing here.** The four leaderboard scopes in +# `Course::Group` / `CourseUser` now order by the underlying SQL expression, via +# `ApplicationRecord.calculated_expression`, instead of by the alias. That is self-contained and +# survives a wiped select list; verified by running those specs with this file disabled. Removing +# the override still matters for keeping the calculated values *selected* (otherwise each record +# lazily re-queries its own attribute, an N+1), and break (1) is fatal regardless. +# +# Neither problem is Rails-8.1-specific; they simply arrived with the ref bump. There is no +# upstream ref that has the 8.1 patches without them — the regression (9cfea61, 2026-05-14) +# predates the 8.1 support commit (464fd11, 2026-05-29). +# +# ## What this does +# +# Removes the gem's override so `ActiveRecord::Calculations#calculate` is used again. That restores +# the exact behaviour of v1.1.1 (`ecaf6c9`), the ref we ran in production, while keeping v1.2.0's +# `rails_8_1_patches.rb`, which is the only reason we bumped. The only thing given up is the +# upstream `.count`-after-`calculated` support added in 9cfea61 — which we never had. +# +# The guard makes this a no-op if a future version drops or relocates the override, so a later gem +# bump degrades to "does nothing" rather than silently removing a fixed implementation. Revisit +# this file on any `calculated_attributes` bump; delete it once upstream is fixed. +ActiveSupport.on_load(:active_record) do + definition = ActiveRecord::Relation.instance_method(:calculate).source_location&.first + + ActiveRecord::Relation.send(:remove_method, :calculate) if definition&.include?('calculated_attributes') +end diff --git a/spec/controllers/course/assessment/question/rubric_based_responses_controller_spec.rb b/spec/controllers/course/assessment/question/rubric_based_responses_controller_spec.rb index 224aea4e44f..95c56ebd679 100644 --- a/spec/controllers/course/assessment/question/rubric_based_responses_controller_spec.rb +++ b/spec/controllers/course/assessment/question/rubric_based_responses_controller_spec.rb @@ -47,11 +47,11 @@ criterions_attributes: { '0' => { grade: 0, - explanation: nil + explanation: '' }, '1' => { grade: 1, - explanation: nil + explanation: '' } } } diff --git a/spec/models/application_record_spec.rb b/spec/models/application_record_spec.rb new file mode 100644 index 00000000000..dbe90cff652 --- /dev/null +++ b/spec/models/application_record_spec.rb @@ -0,0 +1,110 @@ +# frozen_string_literal: true +require 'rails_helper' + +RSpec.describe ApplicationRecord, type: :model do + describe '.calculated_expression' do + let!(:instance) { create(:instance) } + with_tenant(:instance) do + let(:course) { create(:course) } + + context 'with a plain (no-argument) calculated attribute' do + subject { CourseUser.calculated_expression(:experience_points) } + + it 'returns a parenthesised Arel expression wrapping the lambda\'s SQL' do + expect(subject).to be_a(Arel::Nodes::SqlLiteral) + expect(subject.to_s).to start_with('(').and end_with(')') + expect(subject.to_s).to include('SUM(points_awarded)') + end + + it 'matches the SQL the gem builds for the SELECT projection, so the two cannot drift' do + projection = CourseUser.all.calculated(:experience_points).arel.projections. + find { |p| p.respond_to?(:right) && p.right.to_s == 'experience_points' } + + expect(projection.left.to_s).to eq(subject.to_s) + end + end + + context 'with a parameterised calculated attribute' do + # Course::Forum defines `calculated :topic_unread_count, ->(user) { ... }`. + let(:user) { create(:user) } + + it 'forwards arguments to the lambda' do + expression = Course::Forum.calculated_expression(:topic_unread_count, user) + + expect(expression).to be_a(Arel::Nodes::SqlLiteral) + expect(expression.to_s).to include('course_forum_topics') + end + + it 'raises ArgumentError when the required argument is missing' do + expect { Course::Forum.calculated_expression(:topic_unread_count) }. + to raise_error(ArgumentError) + end + end + + context 'when the attribute is not defined' do + it 'raises a descriptive ArgumentError naming the model and the attribute' do + expect { CourseUser.calculated_expression(:not_a_real_attribute) }. + to raise_error(ArgumentError, /CourseUser has no calculated attribute :not_a_real_attribute/) + end + end + + describe 'normalisation of the lambda return value' do + def stub_calculated(model, attribute, callable) + allow(model).to receive(:calculated).and_return(double(calculated: { attribute => callable })) + end + + it 'wraps a raw SQL string' do + stub_calculated(CourseUser, :probe, -> { 'SELECT 1' }) + + expect(CourseUser.calculated_expression(:probe).to_s).to eq('(SELECT 1)') + end + + it 'wraps a relation via #to_sql' do + stub_calculated(CourseUser, :probe, -> { CourseUser.select(:id) }) + + expect(CourseUser.calculated_expression(:probe).to_s).to include('SELECT "course_users"."id"') + end + + it 'sanitises an array, so bound values cannot be interpolated raw' do + stub_calculated(CourseUser, :probe, -> { ['SELECT ?', "O'Brien"] }) + + expect(CourseUser.calculated_expression(:probe).to_s).to eq("(SELECT 'O''Brien')") + end + + it 'passes an Arel node through untouched' do + node = CourseUser.arel_table[:id] + stub_calculated(CourseUser, :probe, -> { node }) + + expect(CourseUser.calculated_expression(:probe)).to eq(node) + end + end + + describe 'the ordering it exists to support' do + # The helper's whole purpose: order by the expression rather than the alias, so ordering + # survives a relation whose select list has been cleared. + let!(:earlier) { create(:course_student, course: course) } + let!(:later) { create(:course_student, course: course) } + + before do + create(:course_experience_points_record, course_user: earlier, points_awarded: 100, + awarded_at: 2.days.ago) + create(:course_experience_points_record, course_user: later, points_awarded: 100, + awarded_at: 1.day.ago) + end + + it 'breaks a tie by who reached the total first' do + ranked = course.course_users.students.ordered_by_experience_points.to_a + + expect(ranked.index(earlier)).to be < ranked.index(later) + end + + it 'still orders correctly after a count has cleared the select list' do + relation = course.course_users.students.ordered_by_experience_points + relation.count # the operation that used to corrupt the relation + + expect(relation.to_a.index(earlier)).to be < relation.to_a.index(later) + end + end + end + end +end diff --git a/spec/services/course/assessment/answer/ai_generated_post_service_spec.rb b/spec/services/course/assessment/answer/ai_generated_post_service_spec.rb index 6121fa5f6b5..fe150367971 100644 --- a/spec/services/course/assessment/answer/ai_generated_post_service_spec.rb +++ b/spec/services/course/assessment/answer/ai_generated_post_service_spec.rb @@ -124,7 +124,7 @@ expect do service.create_ai_generated_draft_post end.to change { Course::Discussion::Post.count }.by(1) - post = Course::Discussion::Post.last + post = submission_question.posts.reload.last expect(post.text).to eq('draft post') expect(post.is_ai_generated).to be true expect(post.workflow_state).to eq('draft') @@ -190,7 +190,7 @@ rating = grading_evaluation.ratings.last expect(rating.rating).to be_nil expect(rating.original_feedback).to eq('generated feedback') - expect(rating.post).to eq(Course::Discussion::Post.last) + expect(rating.post).to eq(submission_question.posts.reload.last) expect(rating.creator).to eq(User.system) end end