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 <simon@instructure.com>
QA-Review: Adam Stone <astone@instructure.com>
Product-Review: Benjamin Porter <bporter@instructure.com>
Tested-by: Jenkins

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])
 

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))
   }

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]

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={})

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

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})

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