cleanup and prevent duplicate external feeds

this commit does three things:
* runs a data migration to delete duplicate external feeds.  duplicates
  are defined as having the same context, url, verbosity, and header match
* prevents duplicates from being created on course copy, by searching
  for and re-using an existing feed if it exists when importing
* validating uniqueness of newly created/saved external feeds, and
  exposing validation errors in the UI

fixes CNVS-17523

test plan:
- before downloading this commit, create two courses, go to the
  announcments pages, and create several different external feeds, some
  of which only duplicate the url (but have differences for other values)
  and some of which are exact duplicates
- checkout this patchset, run migrations
- those courses should now only have unique feeds (the full duplicates
  will be deleted)
- now try creating a new full duplicate
- you should get an error message that says "taken" on the url field
- try importing the feeds from a course with a feed into another course
  that has the exact same external feed
- it should succeed, but not create a duplicate feed in the second course

Change-Id: If1655283102a74626c4579c24382cde92115e776
Reviewed-on: https://gerrit.instructure.com/45991
Tested-by: Jenkins
Reviewed-by: Rob Orton <rob@instructure.com>
Reviewed-by: Joel Hough <joel@instructure.com>
Reviewed-by: John Corrigan <jcorrigan@instructure.com>
QA-Review: Derek Hansen <dhansen@instructure.com>
Product-Review: Simon Williams <simon@instructure.com>

app/coffeescripts/models/ExternalFeed.coffee

@@ -1,6 +1,9 @@
 define [
   'Backbone'
-], (Backbone) ->
+  'compiled/backbone-ext/DefaultUrlMixin'
+], (Backbone, DefaultUrlMixin) ->
 
   class ExternalFeed  extends Backbone.Model
+    @mixin DefaultUrlMixin
     resourceName: 'external_feeds'
+    urlRoot: -> @_defaultUrl()

app/coffeescripts/views/ExternalFeeds/IndexView.coffee

@@ -1,13 +1,14 @@
 define [
-  'Backbone'
   'underscore'
+  'compiled/views/ValidatedFormView'
+  'compiled/models/ExternalFeed'
   'jst/ExternalFeeds/IndexView'
   'compiled/fn/preventDefault'
   'jquery'
   'jquery.toJSON'
-], (Backbone, _, template, preventDefault, $) ->
+], (_, ValidatedFormView, ExternalFeed, template, preventDefault, $) ->
 
-  class IndexView extends Backbone.View
+  class IndexView extends ValidatedFormView
 
     template: template
 
@@ -19,9 +20,19 @@ define [
 
     initialize: ->
       super
+      @createPendingModel()
       @collection.on 'all', @render, this
       @render()
 
+    createPendingModel: ->
+      @model = new ExternalFeed
+
+    toJSON: ->
+      json = @collection.toJSON()
+      json.cid = @cid
+      json.ENV = window.ENV if window.ENV?
+      json
+
     render: ->
       if @collection.length || @options.permissions.create
         $('body').addClass('with-right-side')
@@ -31,6 +42,10 @@ define [
       id = @$(event.target).data('deleteFeedId')
       @collection.get(id).destroy()
 
-    submit: preventDefault (event) ->
-      data = @$('#add_external_feed_form').toJSON()
-      @$el.disableWhileLoading @collection.create data, wait: true
+    getFormData: ->
+      @$('#add_external_feed_form').toJSON()
+
+    onSaveSuccess: =>
+      super
+      @collection.add(@model)
+      @createPendingModel()

app/models/external_feed.rb

@@ -21,6 +21,7 @@ class ExternalFeed < ActiveRecord::Base
   belongs_to :user
   belongs_to :context, :polymorphic => true
   validates_inclusion_of :context_type, :allow_nil => true, :in => ['Course', 'Group']
+
   has_many :external_feed_entries, :dependent => :destroy
   has_many :discussion_topics, dependent: :nullify
 
@@ -29,6 +30,7 @@ class ExternalFeed < ActiveRecord::Base
   include CustomValidations
   validates_presence_of :url, :context_id, :context_type
   validates_as_url :url
+  validates_uniqueness_of :url, scope: [:context_id, :context_type, :verbosity, :header_match]
 
   VERBOSITIES = %w(full link_only truncate)
   validates_inclusion_of :verbosity, :in => VERBOSITIES, :allow_nil => true

app/models/importers/external_feed_importer.rb

@@ -20,8 +20,7 @@ module Importers
     def self.import_from_migration(hash, context, migration=nil, item=nil)
       hash = hash.with_indifferent_access
       return nil if hash[:migration_id] && hash[:external_feeds_to_import] && !hash[:external_feeds_to_import][hash[:migration_id]]
-      item ||= ExternalFeed.where(context_id: context, context_type: context.class.to_s, migration_id: hash[:migration_id]).first if hash[:migration_id]
-      item ||= context.external_feeds.new
+      item ||= find_or_initialize_from_migration(hash, context)
       item.migration_id = hash[:migration_id]
       item.url = hash[:url]
       item.title = hash[:title]
@@ -32,5 +31,22 @@ module Importers
       migration.add_imported_item(item) if migration
       item
     end
+
+    def self.find_or_initialize_from_migration(hash, context)
+      item = ExternalFeed.where(
+        context_id: context,
+        context_type: context.class.to_s,
+        migration_id: hash[:migration_id]
+      ).first if hash[:migration_id]
+      item ||= ExternalFeed.where(
+        context_id: context,
+        context_type: context.class.to_s,
+        url: hash[:url],
+        header_match: hash[:header_match].presence,
+        verbosity: hash[:verbosity]
+      ).first if hash[:url]
+      item ||= context.external_feeds.new
+      item
+    end
   end
 end

app/views/jst/ExternalFeeds/IndexView.handlebars

@@ -42,7 +42,7 @@
          aria-label="{{#t "feed_url_label"}}Feed URL{{/t}}"
          placeholder="{{#t "feed_url_label"}}Feed URL{{/t}}">
   <div>
-    <select name="verbosity" class="input-block-level" aria-label="{{#t "options.content_to_post"}}Content to post{{/t}}">
+    <select id="external_feed_verbosity" name="verbosity" class="input-block-level" aria-label="{{#t "options.content_to_post"}}Content to post{{/t}}">
       <option>--{{#t "options.content_to_post"}}Content to post{{/t}}--</option>
       <option value="full">{{#t "options.full_article"}}Full article{{/t}}</option>
       <option value="truncate">{{#t "options.truncated"}}Truncated{{/t}}</option>

db/migrate/20141217222534_cleanup_duplicate_external_feeds.rb

@@ -0,0 +1,31 @@
+class CleanupDuplicateExternalFeeds < ActiveRecord::Migration
+  disable_ddl_transaction!
+  tag :postdeploy
+
+  def up
+    uniq_fields = %w(context_id context_type url header_match verbosity)
+    scope = ExternalFeed.group(uniq_fields).select(uniq_fields).having("COUNT(*) > 1")
+    scope.find_each do |baddie|
+      duplicate_scope = ExternalFeed.where(baddie.attributes.slice(*uniq_fields))
+      keeper = duplicate_scope.order(:id).first.id
+      duplicate_scope.where('id != ?', keeper).find_ids_in_batches do |id_batch|
+        Announcement.where(external_feed_id: id_batch).update_all(external_feed_id: keeper)
+        ExternalFeedEntry.where(external_feed_id: id_batch).delete_all
+        ExternalFeed.where(id: id_batch).delete_all
+      end
+    end
+
+    ExternalFeed.find_ids_in_ranges do |start_id, end_id|
+      ExternalFeed.where(id: start_id..end_id).where(verbosity: nil).
+        update_all(verbosity: 'full')
+    end
+
+    uniq_fields_without_header = uniq_fields - %w(header_match)
+    add_index :external_feeds, uniq_fields_without_header, unique: true, algorithm: :concurrently, where: "header_match IS NULL", name: 'index_external_feeds_uniquely_1'
+    add_index :external_feeds, uniq_fields, unique: true, algorithm: :concurrently, where: "header_match IS NOT NULL", name: 'index_external_feeds_uniquely_2'
+  end
+
+  def down
+    remove_index :external_feeds, name: 'index_external_feeds_uniquely'
+  end
+end

spec/apis/v1/external_feeds_api_spec.rb

@@ -58,14 +58,14 @@ describe 'ExternalFeedsController', type: :request do
       expect(json).to eq feed_json(feed)
 
       json = api_call_as_user(@allowed_user, :post, @url_base, @url_params.merge(:action => "create"),
-                              { :url => "http://www.example.com/feed", :header_match => '' })
+                              { :url => "http://www.example.com/feed2", :header_match => '' })
       feed = @context.external_feeds.find(json['id'])
       expect(feed.verbosity).to eq 'full'
       expect(feed.header_match).to eq nil
       expect(json).to eq feed_json(feed)
 
       json = api_call_as_user(@allowed_user, :post, @url_base, @url_params.merge(:action => "create"),
-                              { :url => "http://www.example.com/feed", :header_match => ' #mytag  ', :verbosity => 'truncate' })
+                              { :url => "http://www.example.com/feed3", :header_match => ' #mytag  ', :verbosity => 'truncate' })
       feed = @context.external_feeds.find(json['id'])
       expect(feed.verbosity).to eq 'truncate'
       expect(feed.header_match).to eq '#mytag'
@@ -73,7 +73,7 @@ describe 'ExternalFeedsController', type: :request do
 
       # bad verbosity value
       json = api_call_as_user(@allowed_user, :post, @url_base, @url_params.merge(:action => "create"),
-                              { :url => "http://www.example.com/feed", :verbosity => 'bogus' })
+                              { :url => "http://www.example.com/feed4", :verbosity => 'bogus' })
       feed = @context.external_feeds.find(json['id'])
       expect(feed.verbosity).to eq 'full'
 
@@ -85,11 +85,15 @@ describe 'ExternalFeedsController', type: :request do
       json = api_call_as_user(@allowed_user, :post, @url_base, @url_params.merge(:action => "create"),
                               { :verbosity => 'full' }, {}, :expected_status => 400)
 
+      # duplicate url
+      json = api_call_as_user(@allowed_user, :post, @url_base, @url_params.merge(:action => "create"),
+                              { :url => "http://www.example.com/feed" }, {}, :expected_status => 400)
+
       # url protocol is inferred
       json = api_call_as_user(@allowed_user, :post, @url_base, @url_params.merge(:action => "create"),
-                              { :url => "www.example.com/feed" })
+                              { :url => "www.example.com/feed5" })
       feed = @context.external_feeds.find(json['id'])
-      expect(feed.url).to eq "http://www.example.com/feed"
+      expect(feed.url).to eq "http://www.example.com/feed5"
       expect(json).to eq feed_json(feed)
     end
 

spec/coffeescripts/views/ExternalFeeds/IndexViewSpec.coffee

@@ -0,0 +1,72 @@
+define [
+  'jquery'
+  'compiled/collections/ExternalFeedCollection'
+  'compiled/models/ExternalFeed'
+  'compiled/views/ExternalFeeds/IndexView'
+  'helpers/fakeENV'
+], ($, ExternalFeedCollection, ExternalFeed, ExternalFeedsIndexView, fakeENV) ->
+
+  module 'IndexView',
+    setup: ->
+      fakeENV.setup(context_asset_string: 'courses_1')
+      ef = new ExternalFeed
+        id: 1
+        url: 'http://www.example.com/feed'
+        display_name: 'Example Feed'
+        verbosity: 'link_only'
+        header_match: null
+      efc = new ExternalFeedCollection [ef]
+      view = new ExternalFeedsIndexView
+        el: '#fixtures'
+        permissions: { create: true }
+        collection: efc
+      view.render()
+
+    teardown: ->
+      $('#fixtures').empty()
+      fakeENV.teardown()
+
+  submitForm = (url) ->
+    $('.add_external_feed_link').click()
+    $('#external_feed_url').val(url)
+    $('#external_feed_verbosity').val('link_only')
+    $('#add_external_feed_form button').click()
+
+  test 'renders the list of feeds', ->
+    equal $('li.external_feed').length, 1
+    ok $('li.external_feed').text().match('Example Feed')
+
+  test 'allows adding a new feed', ->
+    server = sinon.fakeServer.create()
+    server.respondWith('POST', '/api/v1/courses/1/external_feeds',
+      [200, { 'Content-Type': 'application/json' }, JSON.stringify({
+        id: 2
+        url: 'http://www.example.com/feed2'
+        display_name: 'Other Feed'
+        verbosity: 'link_only'
+        header_match: null
+      })])
+
+    submitForm('http://www.example.com/feed2')
+    server.respond()
+
+    equal $('li.external_feed').length, 2
+    ok $('li.external_feed').text().match('Other Feed')
+
+    server.restore()
+
+  test 'shows errors if save failed', ->
+    server = sinon.fakeServer.create()
+    server.respondWith('POST', '/api/v1/courses/1/external_feeds',
+      [400, { 'Content-Type': 'application/json' }, JSON.stringify({
+        errors: { url: [{ attribute: 'url', message: 'taken', type: 'taken' }] }
+      })])
+
+    submitForm('http://www.example.com/feed2')
+    server.respond()
+
+    equal $('li.external_feed').length, 1
+    ok $('.errorBox').text().match('taken')
+
+    $('.errorBox').remove()
+    server.restore()

spec/factories/external_feed_factory.rb

@@ -16,8 +16,8 @@
 # with this program. If not, see <http://www.gnu.org/licenses/>.
 #
 
-def external_feed_model(opts={})
-  factory_with_protected_attributes(ExternalFeed, valid_external_feed_attributes.merge(opts))
+def external_feed_model(opts={}, do_save=true)
+  factory_with_protected_attributes(ExternalFeed, valid_external_feed_attributes.merge(opts), do_save)
 end
 
 def valid_external_feed_attributes

spec/migrations/cleanup_duplicate_external_feeds_spec.rb

@@ -0,0 +1,85 @@
+#
+# Copyright (C) 2014 Instructure, Inc.
+#
+# This file is part of Canvas.
+#
+# Canvas is free software: you can redistribute it and/or modify it under
+# the terms of the GNU Affero General Public License as published by the Free
+# Software Foundation, version 3 of the License.
+#
+# Canvas is distributed in the hope that it will be useful, but WITHOUT ANY
+# WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR
+# A PARTICULAR PURPOSE. See the GNU Affero General Public License for more
+# details.
+#
+# You should have received a copy of the GNU Affero General Public License along
+# with this program. If not, see <http://www.gnu.org/licenses/>.
+#
+
+require File.expand_path(File.dirname(__FILE__) + '/../spec_helper.rb')
+require 'db/migrate/20141217222534_cleanup_duplicate_external_feeds'
+
+describe 'CleanupDuplicateExternalFeeds' do
+  before do
+    @migration = CleanupDuplicateExternalFeeds.new
+    @migration.down
+  end
+
+  it "should find duplicates" do
+    c1 = course_model
+    feeds = 3.times.map { external_feed_model({}, false) }
+    feeds.each{ |f| f.save(validate: false) }
+    feeds[2].update_attribute(:url, "http://another-non-default-place.com")
+
+    c2 = course_model
+    feeds << external_feed_model
+
+    expect(ExternalFeed.where(id: feeds).count).to eq 4
+
+    @migration.up
+
+    expect(ExternalFeed.where(id: [feeds[0], feeds[2], feeds[3]]).count).to eq 3
+    expect(ExternalFeed.where(id: feeds[1]).count).to eq 0
+  end
+
+  it "should cleanup associated entries and announcements of duplicates" do
+    course_with_teacher
+    @context = @course
+
+    feeds = 2.times.map { external_feed_model({}, false) }
+    feeds.each{ |f| f.save(validate: false) }
+    entries = feeds.map do |feed|
+      feed.external_feed_entries.create!(
+        :user => @teacher,
+        :title => 'blah',
+        :message => 'blah',
+        :workflow_state => :active
+      )
+    end
+    announcements = feeds.map do |feed|
+      a = announcement_model
+      a.update_attribute(:external_feed_id, feed.id)
+      a
+    end
+
+    @migration.up
+
+    expect(ExternalFeed.where(id: feeds[0]).count).to eq 1
+    expect(ExternalFeedEntry.where(id: entries[0]).count).to eq 1
+    expect(announcements[0].reload.external_feed_id).to eq feeds[0].id
+
+    expect(ExternalFeed.where(id: feeds[1]).count).to eq 0
+    expect(ExternalFeedEntry.where(id: entries[1]).count).to eq 0
+    expect(announcements[1].reload.external_feed_id).to eq feeds[0].id
+  end
+
+  it "sets a default for any NULL verbosity field" do
+    course = course_model
+    feed = external_feed_model
+    ExternalFeed.where(id: feed).update_all(verbosity: nil)
+
+    @migration.up
+
+    expect(feed.reload.verbosity).to eq 'full'
+  end
+end

spec/models/importers/external_feed_importer_spec.rb

@@ -0,0 +1,74 @@
+#
+# Copyright (C) 2011 Instructure, Inc.
+#
+# This file is part of Canvas.
+#
+# Canvas is free software: you can redistribute it and/or modify it under
+# the terms of the GNU Affero General Public License as published by the Free
+# Software Foundation, version 3 of the License.
+#
+# Canvas is distributed in the hope that it will be useful, but WITHOUT ANY
+# WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR
+# A PARTICULAR PURPOSE. See the GNU Affero General Public License for more
+# details.
+#
+# You should have received a copy of the GNU Affero General Public License along
+# with this program. If not, see <http://www.gnu.org/licenses/>.
+#
+
+require File.expand_path(File.dirname(__FILE__) + '../../../import_helper')
+
+describe Importers::ExternalFeedImporter do
+
+  context ".import_from_migration" do
+    it "creates a feed from the provided hash" do
+      @course = course
+      data = {
+        url: 'http://www.example.com/feed',
+        title: 'test feed',
+        verbosity: 'link_only',
+        header_match: ''
+      }
+      feed = Importers::ExternalFeedImporter.import_from_migration(data, @course)
+      expect(feed.url).to eq data[:url]
+      expect(feed.title).to eq data[:title]
+      expect(feed.verbosity).to eq data[:verbosity]
+      expect(feed.header_match).to be_nil
+    end
+  end
+
+  context ".find_or_initialize_from_migration" do
+    before(:once) do
+      @course = course
+      @feed = external_feed_model(migration_id: '12345')
+    end
+
+    it "finds a feed by migration id" do
+      found = Importers::ExternalFeedImporter.find_or_initialize_from_migration({
+        migration_id: '12345'
+      }, @course)
+      expect(found.id).to eq @feed.id
+    end
+
+    it "finds by uniq attrs" do
+      found = Importers::ExternalFeedImporter.find_or_initialize_from_migration({
+        migration_id: 'xyzyx',
+        url: @feed.url,
+        header_match: @feed.header_match,
+        verbosity: @feed.verbosity
+      }, @course)
+      expect(found.id).to eq @feed.id
+    end
+
+    it "initializes if none found" do
+      found = Importers::ExternalFeedImporter.find_or_initialize_from_migration({
+        migration_id: 'xyzyx',
+        url: @feed.url + "xxx",
+        header_match: @feed.header_match,
+        verbosity: @feed.verbosity
+      }, @course)
+      expect(found).to be_new_record
+    end
+  end
+
+end