From b7926b6926b0e0998e3922d2a07248cc8b494cc8 Mon Sep 17 00:00:00 2001 From: Patrick Peak Date: Fri, 20 Apr 2012 14:37:48 -0400 Subject: [PATCH] Can now delete attachments via the attachment manager. --- .../javascripts/cms/attachment_manager.js.erb | 25 ++++++++++++++----- app/controllers/cms/attachments_controller.rb | 2 +- app/models/cms/attachment.rb | 2 ++ config/routes.rb | 3 +-- lib/cms/behaviors/attaching.rb | 20 +++++++++++---- test/assumptions_test.rb | 15 +++++++++++ test/factories/attachables.rb | 14 +++++++++-- test/unit/behaviors/attaching_test.rb | 17 ++++++++++++- test/unit/models/attachment_test.rb | 25 +++++++++++++++++++ test/unit/models/user_test.rb | 3 --- todo_list.markdown | 2 +- 11 files changed, 107 insertions(+), 21 deletions(-) create mode 100644 test/assumptions_test.rb diff --git a/app/assets/javascripts/cms/attachment_manager.js.erb b/app/assets/javascripts/cms/attachment_manager.js.erb index 5d67e058e..c07cc5673 100644 --- a/app/assets/javascripts/cms/attachment_manager.js.erb +++ b/app/assets/javascripts/cms/attachment_manager.js.erb @@ -1,4 +1,13 @@ +// Allows users to upload files via AJAX as attachments for a given block. +// $(function () { + + // Return the authenticity token for any JS function that needs to do AJAX. + // Since AJAX posts will fail if you don't attach this as defined here: http://michaelhendrickx.com/201012_jquery-ajax-with-rails-authenticity-token.html + $.cms.auth_token = function () { + return $('meta[name=csrf-token]').attr('content'); + }; + $.cms.AttachmentManager = { upload:function (file) { var assetName = $('#asset_types').val() @@ -34,13 +43,17 @@ $(function () { }, + // @param [Integer] id The id of the attachment to delete. destroy:function (id) { - if (confirm("Are you sure?")) { - $.post('/cms/attachments/' + id, {_method:'delete'}, null, 'script'); - $("#asset_" + id).hide(); - if ($("#assets_table > table tr:visible").length <= 2) { - $("#assets_table > table").hide(); - } + if (confirm("Are you sure want to delete this attachment?")) { + $.post('/cms/attachments/' + id, {_method:'delete', authenticity_token:$.cms.auth_token()}, function (attachment_id) { + console.log(attachment_id); + $("#attachment_" + attachment_id).hide(); + if ($("#assets_table > table tr:visible").length <= 2) { + $("#assets_table > table").hide(); + } + }, 'script'); + } return false; } diff --git a/app/controllers/cms/attachments_controller.rb b/app/controllers/cms/attachments_controller.rb index 0495887b7..b89f68d49 100644 --- a/app/controllers/cms/attachments_controller.rb +++ b/app/controllers/cms/attachments_controller.rb @@ -24,7 +24,7 @@ def create def destroy @attachment = Attachment.find(params[:id]) @attachment.destroy - render :nothing => true + render :json => @attachment.id end end end \ No newline at end of file diff --git a/app/models/cms/attachment.rb b/app/models/cms/attachment.rb index 950136c72..ca71abef6 100644 --- a/app/models/cms/attachment.rb +++ b/app/models/cms/attachment.rb @@ -20,6 +20,8 @@ class Attachment < ActiveRecord::Base before_create :set_cardinality belongs_to :attachable, :polymorphic => true + validates :attachment_name, :attachable_type, :presence => true + include Cms::Addressable include Cms::Addressable::DeprecatedPageAccessors has_one :section_node, :as => :node, :class_name => 'Cms::SectionNode' diff --git a/config/routes.rb b/config/routes.rb index 7fc24a4a9..337455b28 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -60,8 +60,7 @@ put :move_to_root end end - match '/attachments/:id', :to => 'attachments#show', :as=>'attachment' - resources :attachments, :only=>[:create, :destroy] + resources :attachments, :only=>[:show, :create, :destroy] match '/content_library', :to=>'html_blocks#index', :as=>'content_library' content_blocks :html_blocks diff --git a/lib/cms/behaviors/attaching.rb b/lib/cms/behaviors/attaching.rb index 5173f7544..158626df9 100644 --- a/lib/cms/behaviors/attaching.rb +++ b/lib/cms/behaviors/attaching.rb @@ -170,18 +170,27 @@ def has_many_attachments(name, options = {}) end end - # Find all attachments as of the given version for the specified block. + # Find all attachments as of the given version for the specified block. Excludes attachments that were + # deleted as of a version. # # @param [Integer] version_number # @param [Attaching] attachable The object with attachments # @return [Array] def attachments_as_of_version(version_number, attachable) - found_versions = Cms::Attachment::Version.where(:attachable_id => attachable.id).where(:attachable_type => attachable.attachable_type).where(:attachable_version => version_number).all - found_attachments = [] + found_versions = Cms::Attachment::Version.where(:attachable_id => attachable.id). + where(:attachable_type => attachable.attachable_type). + where(:attachable_version => version_number). + order(:version).all + found_attachments = {} + + # Compress multiple versions into a single record and exclude deleted records. found_versions.each do |av| - found_attachments << av.build_object_from_version + record = av.build_object_from_version + found_attachments[record.id] = record end - found_attachments + found_attachments.delete_if { |_, value| value.deleted? } + + found_attachments.values end end @@ -241,6 +250,7 @@ def after_build_new_version(new_version) def after_as_of_version() @attachments_as_of = self.class.attachments_as_of_version(version, self) + # Override #attachments to return the original attachments for the current version. metaclass = class << self; self; diff --git a/test/assumptions_test.rb b/test/assumptions_test.rb new file mode 100644 index 000000000..0eb26672c --- /dev/null +++ b/test/assumptions_test.rb @@ -0,0 +1,15 @@ +require "test_helper" + +module Cms + + # To avoid hard to debug errors associated with testing, + # we verify some assumptions here about the state of the database and other things. + class AssumptionsTest < ActiveSupport::TestCase + + test "tests assume there is no data in the database before starting" do + assert_equal 0, Section.count + assert_equal 0, Page.count + assert_equal 0, Permission.count + end + end +end \ No newline at end of file diff --git a/test/factories/attachables.rb b/test/factories/attachables.rb index c80153542..8f7e441c8 100644 --- a/test/factories/attachables.rb +++ b/test/factories/attachables.rb @@ -15,11 +15,21 @@ class VersionedAttachable < ActiveRecord::Base end m.sequence(:name) { |n| "VersionedAttachable#{n}" } m.after_build { |f, evaluator| - opts = {:data => evaluator.attachment_file, :attachment_name => 'document' } - opts[:parent] = evaluator.parent if evaluator.parent # Handle :parent=>nil + opts = {:data => evaluator.attachment_file, :attachment_name => 'document'} + opts[:parent] = evaluator.parent if evaluator.parent # Handle :parent=>nil opts[:data_file_path] = evaluator.attachment_file_path if evaluator.attachment_file_path f.attachments.build(opts) } m.publish_on_save true end + + factory :attachment_document, :class => Cms::Attachment do |m| + m.attachment_name "document" + m.attachable_type "VersionedAttachable" + m.data { mock_file } + m.parent { find_or_create_root_section } + m.data_file_path "/" + m.publish_on_save true + end + end diff --git a/test/unit/behaviors/attaching_test.rb b/test/unit/behaviors/attaching_test.rb index 288c16f5f..187c2d27d 100644 --- a/test/unit/behaviors/attaching_test.rb +++ b/test/unit/behaviors/attaching_test.rb @@ -357,10 +357,25 @@ def setup assert_equal file_contents(file2.path), file_contents(@attachable.as_of_version(2).attachments[0].full_file_location) end - test "" do + test "delete an attachment should not be found when fetching the draft version of blocks" do + doc = @attachable.attachments.first + doc.destroy + current = @attachable.as_of_draft_version() + assert_equal 0, current.attachments.size end + test "deleted attachments shouldn't be found'" do + @attachable.attachments << create(:attachment_document) + @attachable.save! + + assert_equal 2, @attachable.attachments.size + doc = @attachable.attachments.first + doc.destroy + + current = @attachable.as_of_draft_version() + assert_equal 1, current.attachments.size + end private def update_attachable_to_version2(new_path) diff --git a/test/unit/models/attachment_test.rb b/test/unit/models/attachment_test.rb index b1c4fc6cf..b42131515 100644 --- a/test/unit/models/attachment_test.rb +++ b/test/unit/models/attachment_test.rb @@ -156,3 +156,28 @@ def attachment @attachment ||= Cms::Attachment.new end end + +class AttachmentsValidation < ActiveSupport::TestCase + + def setup + @valid_attachment = Cms::Attachment.new + @valid_attachment.attachment_name = "anything" + @valid_attachment.attachable_type = "VersionedAttachable" + end + + test "Valid" do + assert @valid_attachment.valid? + end + + test "Must have an attachment_name" do + @valid_attachment.attachment_name = nil + refute @valid_attachment.valid? + end + + test "Must have content_block_class" do + @valid_attachment.attachable_type = nil + refute @valid_attachment.valid? + end + + +end diff --git a/test/unit/models/user_test.rb b/test/unit/models/user_test.rb index 434ce7abd..819308e83 100644 --- a/test/unit/models/user_test.rb +++ b/test/unit/models/user_test.rb @@ -141,11 +141,8 @@ class UserAbleToViewTest < ActiveSupport::TestCase user = Cms::User.new assert(!user.cms_access?, "") end - - end - class PageEdittingPermissions < ActiveSupport::TestCase def setup @content_editor = create(:content_editor) # Create first, so it will have permission to edit root section diff --git a/todo_list.markdown b/todo_list.markdown index 26a367f14..c235e847f 100644 --- a/todo_list.markdown +++ b/todo_list.markdown @@ -1,7 +1,7 @@ Tasks: - [FEATURE] Multiple attachments on a single file works. - * [FEATURE] Make deleting attachments work. (It isn't handling auth token issues) + * [BUG] Remove/then add an attachment. The new one is not flagged as 'multiple'. * [BUG] Uploading files with spaces in them breaks things. * [FEATURE] Show be an attachment_manager display helper (for viewing them) or at least document it.