From 6ea91b44169e275dbedd89bb281e2cd5d61e5f80 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9gis=20Hanol?= Date: Sat, 15 Jun 2013 09:54:49 +0200 Subject: [PATCH] remove useless upload topic direct association --- .../discourse/views/composer_view.js | 50 +++++----- app/controllers/uploads_controller.rb | 11 +-- app/models/upload.rb | 5 +- ...0615073305_remove_topic_id_from_uploads.rb | 9 ++ spec/controllers/uploads_controller_spec.rb | 94 +++++++++---------- spec/models/upload_spec.rb | 5 +- 6 files changed, 81 insertions(+), 93 deletions(-) create mode 100644 db/migrate/20130615073305_remove_topic_id_from_uploads.rb diff --git a/app/assets/javascripts/discourse/views/composer_view.js b/app/assets/javascripts/discourse/views/composer_view.js index 18841c306..009d3c18e 100644 --- a/app/assets/javascripts/discourse/views/composer_view.js +++ b/app/assets/javascripts/discourse/views/composer_view.js @@ -44,15 +44,15 @@ Discourse.ComposerView = Discourse.View.extend({ }.property('content.createdPost'), observeReplyChanges: function() { - var _this = this; + var composerView = this; if (this.get('content.hidePreview')) return; Ember.run.next(null, function() { var $wmdPreview, caretPosition; - if (_this.editor) { - _this.editor.refreshPreview(); + if (composerView.editor) { + composerView.editor.refreshPreview(); // if the caret is on the last line ensure preview scrolled to bottom - caretPosition = Discourse.Utilities.caretPosition(_this.wmdInput[0]); - if (!_this.wmdInput.val().substring(caretPosition).match(/\n/)) { + caretPosition = Discourse.Utilities.caretPosition(composerView.wmdInput[0]); + if (!composerView.wmdInput.val().substring(caretPosition).match(/\n/)) { $wmdPreview = $('#wmd-preview'); if ($wmdPreview.is(':visible')) { return $wmdPreview.scrollTop($wmdPreview[0].scrollHeight); @@ -164,53 +164,50 @@ Discourse.ComposerView = Discourse.View.extend({ initEditor: function() { // not quite right, need a callback to pass in, meaning this gets called once, // but if you start replying to another topic it will get the avatars wrong - var $uploadTarget, $wmdInput, editor, saveDraft, selected, template, topic, transformTemplate, - _this = this; + var $wmdInput, editor, composerView = this; this.wmdInput = $wmdInput = $('#wmd-input'); if ($wmdInput.length === 0 || $wmdInput.data('init') === true) return; $LAB.script(assetPath('defer/html-sanitizer-bundle')); Discourse.ComposerView.trigger("initWmdEditor"); - template = Discourse.UserSelector.templateFunction(); + var template = Discourse.UserSelector.templateFunction(); - transformTemplate = Handlebars.compile("{{avatar this imageSize=\"tiny\"}} {{this.username}}"); $wmdInput.data('init', true); $wmdInput.autocomplete({ template: template, dataSource: function(term) { return Discourse.UserSearch.search({ term: term, - topicId: _this.get('controller.controllers.topic.content.id') + topicId: composerView.get('controller.controllers.topic.content.id') }); }, key: "@", transformComplete: function(v) { return v.username; } }); - topic = this.get('topic'); this.editor = editor = Discourse.Markdown.createEditor({ lookupAvatar: function(username) { return Discourse.Utilities.avatarImg({ username: username, size: 'tiny' }); } }); - $uploadTarget = $('#reply-control'); + var $uploadTarget = $('#reply-control'); this.editor.hooks.insertImageDialog = function(callback) { callback(null); - _this.get('controller').send('showImageSelector', _this); + composerView.get('controller').send('showImageSelector', composerView); return true; }; this.editor.hooks.onPreviewRefresh = function() { - return _this.afterRender(); + return composerView.afterRender(); }; this.editor.run(); this.set('editor', this.editor); this.loadingChanged(); - saveDraft = Discourse.debounce((function() { - return _this.get('controller').saveDraft(); + var saveDraft = Discourse.debounce((function() { + return composerView.get('controller').saveDraft(); }), 2000); $wmdInput.keyup(function() { @@ -223,7 +220,7 @@ Discourse.ComposerView = Discourse.View.extend({ $replyTitle.keyup(function() { saveDraft(); // removes the red background once the requirements are met - if (_this.get('controller.content.missingTitleCharacters') <= 0) { + if (composerView.get('controller.content.missingTitleCharacters') <= 0) { $replyTitle.removeClass("requirements-not-met"); } return true; @@ -232,7 +229,7 @@ Discourse.ComposerView = Discourse.View.extend({ // when the title field loses the focus... $replyTitle.blur(function(){ // ...and the requirements are not met (ie. the minimum number of characters) - if (_this.get('controller.content.missingTitleCharacters') > 0) { + if (composerView.get('controller.content.missingTitleCharacters') > 0) { // then, "redify" the background $replyTitle.toggleClass("requirements-not-met", true); } @@ -245,22 +242,21 @@ Discourse.ComposerView = Discourse.View.extend({ $uploadTarget.fileupload({ url: Discourse.getURL('/uploads'), dataType: 'json', - timeout: 20000, - formData: { topic_id: 1234 } + timeout: 20000 }); // submit - this event is triggered for each upload $uploadTarget.on('fileuploadsubmit', function (e, data) { var result = Discourse.Utilities.validateFilesForUpload(data.files); // reset upload status when everything is ok - if (result) _this.setProperties({ uploadProgress: 0, loadingImage: true }); + if (result) composerView.setProperties({ uploadProgress: 0, loadingImage: true }); return result; }); // send - this event is triggered when the upload request is about to start $uploadTarget.on('fileuploadsend', function (e, data) { // hide the "image selector" modal - _this.get('controller').send('closeModal'); + composerView.get('controller').send('closeModal'); // cf. https://github.com/blueimp/jQuery-File-Upload/wiki/API#how-to-cancel-an-upload var jqXHR = data.xhr(); // need to wait for the link to show up in the DOM @@ -279,21 +275,21 @@ Discourse.ComposerView = Discourse.View.extend({ // progress all $uploadTarget.on('fileuploadprogressall', function (e, data) { var progress = parseInt(data.loaded / data.total * 100, 10); - _this.set('uploadProgress', progress); + composerView.set('uploadProgress', progress); }); // done $uploadTarget.on('fileuploaddone', function (e, data) { var upload = data.result; var html = ""; - _this.addMarkdown(html); - _this.set('loadingImage', false); + composerView.addMarkdown(html); + composerView.set('loadingImage', false); }); // fail $uploadTarget.on('fileuploadfail', function (e, data) { // hide upload status - _this.set('loadingImage', false); + composerView.set('loadingImage', false); // deal with meaningful errors first if (data.jqXHR) { switch (data.jqXHR.status) { @@ -321,7 +317,7 @@ Discourse.ComposerView = Discourse.View.extend({ // to finish. return Em.run.later(jQuery, (function() { var replyTitle = $('#reply-title'); - _this.resize(); + composerView.resize(); if (replyTitle.length) { return replyTitle.putCursorAtEnd(); } else { diff --git a/app/controllers/uploads_controller.rb b/app/controllers/uploads_controller.rb index db82d1eed..ceff160e3 100644 --- a/app/controllers/uploads_controller.rb +++ b/app/controllers/uploads_controller.rb @@ -2,16 +2,15 @@ class UploadsController < ApplicationController before_filter :ensure_logged_in def create - params.require(:topic_id) file = params[:file] || params[:files].first - + # only supports images for now return render status: 415, json: failed_json unless file.content_type =~ /^image\/.+/ - - upload = Upload.create_for(current_user.id, file, params[:topic_id]) - + + upload = Upload.create_for(current_user.id, file) + render_serialized(upload, UploadSerializer, root: false) - + rescue FastImage::ImageFetchFailure render status: 422, text: I18n.t("upload.image.fetch_failure") rescue FastImage::UnknownImageType diff --git a/app/models/upload.rb b/app/models/upload.rb index 06858502b..ee1757bb7 100644 --- a/app/models/upload.rb +++ b/app/models/upload.rb @@ -5,7 +5,6 @@ require 'local_store' class Upload < ActiveRecord::Base belongs_to :user - belongs_to :topic has_many :post_uploads has_many :posts, through: :post_uploads @@ -13,7 +12,7 @@ class Upload < ActiveRecord::Base validates_presence_of :filesize validates_presence_of :original_filename - def self.create_for(user_id, file, topic_id) + def self.create_for(user_id, file) # retrieve image info image_info = FastImage.new(file.tempfile, raise_on_failure: true) # compute image aspect ratio @@ -21,7 +20,6 @@ class Upload < ActiveRecord::Base upload = Upload.create!({ user_id: user_id, - topic_id: topic_id, original_filename: file.original_filename, filesize: File.size(file.tempfile), width: width, @@ -53,7 +51,6 @@ end # # id :integer not null, primary key # user_id :integer not null -# topic_id :integer not null # original_filename :string(255) not null # filesize :integer not null # width :integer diff --git a/db/migrate/20130615073305_remove_topic_id_from_uploads.rb b/db/migrate/20130615073305_remove_topic_id_from_uploads.rb new file mode 100644 index 000000000..3be8c407f --- /dev/null +++ b/db/migrate/20130615073305_remove_topic_id_from_uploads.rb @@ -0,0 +1,9 @@ +class RemoveTopicIdFromUploads < ActiveRecord::Migration + def up + remove_column :uploads, :topic_id + end + + def down + add_column :uploads, :topic_id, :interger, null: false, default: -1 + end +end diff --git a/spec/controllers/uploads_controller_spec.rb b/spec/controllers/uploads_controller_spec.rb index bbbef2914..f71342b18 100644 --- a/spec/controllers/uploads_controller_spec.rb +++ b/spec/controllers/uploads_controller_spec.rb @@ -14,64 +14,54 @@ describe UploadsController do context '.create' do - context 'missing params' do - it 'raises an error without the topic_id param' do - -> { xhr :post, :create }.should raise_error(ActionController::ParameterMissing) + let(:logo) do + ActionDispatch::Http::UploadedFile.new({ + filename: 'logo.png', + type: 'image/png', + tempfile: File.new("#{Rails.root}/spec/fixtures/images/logo.png") + }) + end + + let(:logo_dev) do + ActionDispatch::Http::UploadedFile.new({ + filename: 'logo-dev.png', + type: 'image/png', + tempfile: File.new("#{Rails.root}/spec/fixtures/images/logo-dev.png") + }) + end + + let(:text_file) do + ActionDispatch::Http::UploadedFile.new({ + filename: 'LICENSE.txt', + type: 'text/plain', + tempfile: File.new("#{Rails.root}/LICENSE.txt") + }) + end + + let(:files) { [ logo_dev, logo ] } + + context 'with a file' do + it 'is succesful' do + xhr :post, :create, file: logo + response.should be_success + end + + it 'supports only images' do + xhr :post, :create, file: text_file + response.status.should eq 415 end end - context 'correct params' do + context 'with some files' do - let(:logo) do - ActionDispatch::Http::UploadedFile.new({ - filename: 'logo.png', - type: 'image/png', - tempfile: File.new("#{Rails.root}/spec/fixtures/images/logo.png") - }) + it 'is succesful' do + xhr :post, :create, files: files + response.should be_success end - let(:logo_dev) do - ActionDispatch::Http::UploadedFile.new({ - filename: 'logo-dev.png', - type: 'image/png', - tempfile: File.new("#{Rails.root}/spec/fixtures/images/logo-dev.png") - }) - end - - let(:text_file) do - ActionDispatch::Http::UploadedFile.new({ - filename: 'LICENSE.txt', - type: 'text/plain', - tempfile: File.new("#{Rails.root}/LICENSE.txt") - }) - end - - let(:files) { [ logo_dev, logo ] } - - context 'with a file' do - it 'is succesful' do - xhr :post, :create, topic_id: 1234, file: logo - response.should be_success - end - - it 'supports only images' do - xhr :post, :create, topic_id: 1234, file: text_file - response.status.should eq 415 - end - end - - context 'with some files' do - - it 'is succesful' do - xhr :post, :create, topic_id: 1234, files: files - response.should be_success - end - - it 'takes the first file' do - xhr :post, :create, topic_id: 1234, files: files - response.body.should match /logo-dev.png/ - end - + it 'takes the first file' do + xhr :post, :create, files: files + response.body.should match /logo-dev.png/ end end diff --git a/spec/models/upload_spec.rb b/spec/models/upload_spec.rb index 8378da26c..f44c1f679 100644 --- a/spec/models/upload_spec.rb +++ b/spec/models/upload_spec.rb @@ -3,7 +3,6 @@ require 'spec_helper' describe Upload do it { should belong_to :user } - it { should belong_to :topic } it { should have_many :post_uploads } it { should have_many :posts } @@ -14,7 +13,6 @@ describe Upload do context '.create_for' do let(:user_id) { 1 } - let(:topic_id) { 42 } let(:logo) do ActionDispatch::Http::UploadedFile.new({ @@ -24,14 +22,13 @@ describe Upload do }) end - let(:upload) { Upload.create_for(user_id, logo, topic_id) } + let(:upload) { Upload.create_for(user_id, logo) } let(:url) { "http://domain.com" } shared_examples_for "upload" do it "is valid" do upload.user_id.should == user_id - upload.topic_id.should == topic_id upload.original_filename.should == logo.original_filename upload.filesize.should == File.size(logo.tempfile) upload.width.should == 244