From 8d770844e2b67b2239ba3fa008c9b671a93c2a0e Mon Sep 17 00:00:00 2001 From: Benjamin Porter Date: Wed, 1 Apr 2015 17:47:52 -0600 Subject: [PATCH] Announcements Index page - Order by post post date not position Ordering by position is normally identical to ordering by post date. The notable exception is when announcements are imported into a course. In this case, the order is by position which is wonky because positions are duplicated. This patch changes the index to explicitly order by posted_at, or delayed_post_at Fixes CNVS-13028 Test Plan: 1. Create a course and add an announcement or two 2. Create another course and add an announcement or two 3. Return to the first course and add an announcement or two 4. Return to the second course and add an announcement or two 5. Return to the first course and import the announcement from the second course (Hint: You will need delayed jobs running to complete the import). 6. Visit the announcements index page on the first course and ensure that the ordering of announcements is based on *creation time* or *posting time*, and not on position Change-Id: Ib1d1b155da197c1147fa87594152a722234190dd Reviewed-on: https://gerrit.instructure.com/51527 Reviewed-by: Simon Williams QA-Review: Adam Stone Product-Review: Benjamin Porter Tested-by: Jenkins --- .../discussion_topics_controller.rb | 8 +++- app/models/discussion_topic.rb | 2 + .../importers/discussion_topic_importer.rb | 8 ++-- spec/apis/v1/discussion_topics_api_spec.rb | 31 +++++++++++++++ .../discussion_topics_api_controller.rb | 1 - .../discussion_topics_controller_spec.rb | 2 + spec/models/discussion_topic_spec.rb | 39 +++++++++++++++++++ 7 files changed, 85 insertions(+), 6 deletions(-) diff --git a/app/controllers/discussion_topics_controller.rb b/app/controllers/discussion_topics_controller.rb index 6df13660657..4b6020141ec 100644 --- a/app/controllers/discussion_topics_controller.rb +++ b/app/controllers/discussion_topics_controller.rb @@ -262,7 +262,13 @@ class DiscussionTopicsController < ApplicationController @context.active_discussion_topics.only_discussion_topics end - scope = params[:order_by] == 'recent_activity' ? scope.by_last_reply_at : scope.by_position_legacy + scope = if params[:order_by] == 'recent_activity' + scope.by_last_reply_at + elsif params[:only_announcements] + scope.by_posted_at + else + scope.by_position_legacy + end scope = DiscussionTopic.search_by_attribute(scope, :title, params[:search_term]) diff --git a/app/models/discussion_topic.rb b/app/models/discussion_topic.rb index ee4f4da7cf3..ed3e92bec38 100644 --- a/app/models/discussion_topic.rb +++ b/app/models/discussion_topic.rb @@ -469,6 +469,8 @@ class DiscussionTopic < ActiveRecord::Base scope :by_position_legacy, -> { order("discussion_topics.position DESC, discussion_topics.created_at DESC, discussion_topics.id DESC") } scope :by_last_reply_at, -> { order("discussion_topics.last_reply_at DESC, discussion_topics.created_at DESC, discussion_topics.id DESC") } + scope :by_posted_at, -> { order("discussion_topics.posted_at DESC, discussion_topics.delayed_post_at DESC, discussion_topics.created_at DESC, discussion_topics.id DESC") } + scope :visible_to_students_in_course_with_da, lambda { |user_ids, course_ids| without_assignment_in_course(course_ids).union(joins_assignment_student_visibilities(user_ids, course_ids)) } diff --git a/app/models/importers/discussion_topic_importer.rb b/app/models/importers/discussion_topic_importer.rb index 0ffcdf78866..0d7be796edc 100644 --- a/app/models/importers/discussion_topic_importer.rb +++ b/app/models/importers/discussion_topic_importer.rb @@ -88,10 +88,10 @@ module Importers item.message = I18n.t('#discussion_topic.empty_message', 'No message') end - item.posted_at = Canvas::Migration::MigratorHelper.get_utc_time_from_timestamp(options[:posted_at]) - item.delayed_post_at = Canvas::Migration::MigratorHelper.get_utc_time_from_timestamp(options.delayed_post_at) - item.lock_at = Canvas::Migration::MigratorHelper.get_utc_time_from_timestamp(options[:lock_at]) - item.last_reply_at = nil if item.new_record? + item.posted_at = Canvas::Migration::MigratorHelper.get_utc_time_from_timestamp(options[:posted_at]) + item.delayed_post_at = Canvas::Migration::MigratorHelper.get_utc_time_from_timestamp(options.delayed_post_at) + item.lock_at = Canvas::Migration::MigratorHelper.get_utc_time_from_timestamp(options[:lock_at]) + item.last_reply_at = nil if item.new_record? if options[:workflow_state].present? item.workflow_state = options[:workflow_state] diff --git a/spec/apis/v1/discussion_topics_api_spec.rb b/spec/apis/v1/discussion_topics_api_spec.rb index 2908555cbc5..8fcaf651b9c 100644 --- a/spec/apis/v1/discussion_topics_api_spec.rb +++ b/spec/apis/v1/discussion_topics_api_spec.rb @@ -2413,6 +2413,37 @@ describe DiscussionTopicsController, type: :request do end end end + + it "should order Announcement items by posted_at rather than by position" do + course_with_teacher(:active_all => true) + account_admin_user(account: @course.account) # sets @admin + + ann_ids_ordered_by_posted_at = 10.times.map do |i| + ann = Announcement.create!({ + context: @course, + message: "Test Message", + }) + ann.posted_at = i.days.ago + ann.position = 1 + ann.save! + ann.id + end + + json = api_call( + :get, + "/api/v1/courses/#{@course.id}/discussion_topics?only_announcements=1", + { + controller: "discussion_topics", + action: "index", + format: "json", + course_id: @course.id.to_s, + only_announcements: 1, + }, + {} + ) + + expect(json.map{ |j| j["id"] }).to eq(ann_ids_ordered_by_posted_at) + end end def create_attachment(context, opts={}) diff --git a/spec/controllers/discussion_topics_api_controller.rb b/spec/controllers/discussion_topics_api_controller.rb index 889e71ff692..5a1fb842dcd 100644 --- a/spec/controllers/discussion_topics_api_controller.rb +++ b/spec/controllers/discussion_topics_api_controller.rb @@ -38,6 +38,5 @@ describe DiscussionTopicsApiController do expect(page_view.url).to match %r{^http://test\.host/api/v1/courses/\d+/discussion_topics} expect(page_view.participated).to be_truthy end - end end diff --git a/spec/controllers/discussion_topics_controller_spec.rb b/spec/controllers/discussion_topics_controller_spec.rb index 4ec3cca1b5b..a69d7e49792 100644 --- a/spec/controllers/discussion_topics_controller_spec.rb +++ b/spec/controllers/discussion_topics_controller_spec.rb @@ -42,6 +42,7 @@ describe DiscussionTopicsController do @topic.save @topic end + def topic_entry @entry = @topic.discussion_entries.create(:message => "some message", :user => @user) end @@ -89,6 +90,7 @@ describe DiscussionTopicsController do @assignment.only_visible_to_overrides = true @assignment.save! end + it "should return graded and visible group discussions properly" do cs = @student.enrollments.first.course_section create_section_override_for_assignment(@assignment, {course_section: cs}) diff --git a/spec/models/discussion_topic_spec.rb b/spec/models/discussion_topic_spec.rb index d7a38fe6346..bdc9c5efab6 100644 --- a/spec/models/discussion_topic_spec.rb +++ b/spec/models/discussion_topic_spec.rb @@ -1657,4 +1657,43 @@ describe DiscussionTopic do expect(rss.first.enclosure.url).to match(%r{download.mp4}) end end + + context "announcements" do + context "scopes" do + context "by_posted_at" do + let(:c) { Course.create! } + let(:new_ann) do + lambda do + Announcement.create!({ + context: c, + message: "Test Message", + }) + end + end + + it "sorts by the posted_at field descending" do + anns = 10.times.map do |i| + ann = new_ann.call + ann.posted_at = i.days.ago + ann.position = 1 + ann.save! + ann + end + expect(c.announcements.by_posted_at).to eq(anns) + end + + it "secondarily sorts by the delayed_post_at field descending" do + anns = 10.times.map do |i| + ann = new_ann.call + ann.delayed_post_at = i.days.ago + ann.posted_at = nil + ann.position = 1 + ann.save! + ann + end + expect(c.announcements.by_posted_at).to eq(anns) + end + end + end + end end