From e614562470dfca7b6bc865666be1e73e4422278e Mon Sep 17 00:00:00 2001 From: Florian Staudacher Date: Sun, 22 Sep 2013 17:19:42 +0200 Subject: [PATCH] avoid publishing empty posts (fixes #4401) invalidate empty posts when created by a local user add changelog entry --- Changelog.md | 1 + .../app/views/publisher/uploader_view.js | 3 ++ .../javascripts/app/views/publisher_view.js | 1 + app/models/status_message.rb | 12 ++++++- features/desktop/posts_from_main_page.feature | 1 + features/step_definitions/posts_steps.rb | 4 +++ features/support/publishing_cuke_helpers.rb | 5 +++ spec/models/status_message_spec.rb | 33 +++++++++++-------- 8 files changed, 46 insertions(+), 14 deletions(-) diff --git a/Changelog.md b/Changelog.md index d1b167be670..3280470d7a3 100644 --- a/Changelog.md +++ b/Changelog.md @@ -26,6 +26,7 @@ * Fix date color and alignment in the notifications dropdown [#4502](https://github.com/diaspora/diaspora/issues/4502) * Add a white background to images shown in the lightbox [#4475](https://github.com/diaspora/diaspora/issues/4475) * Refactor getting_started page, test if facebook is available, fix [#4520](https://github.com/diaspora/diaspora/issues/4520) +* Avoid publishing empty posts [#4542](https://github.com/diaspora/diaspora/pull/4542) ## Features * Add oEmbed content to the mobile view [#4343](https://github.com/diaspora/diaspora/pull/4353) diff --git a/app/assets/javascripts/app/views/publisher/uploader_view.js b/app/assets/javascripts/app/views/publisher/uploader_view.js index b41475de640..b09d8dd2106 100644 --- a/app/assets/javascripts/app/views/publisher/uploader_view.js +++ b/app/assets/javascripts/app/views/publisher/uploader_view.js @@ -66,6 +66,7 @@ app.views.PublisherUploader = Backbone.View.extend({ url = response.data.photo.unprocessed_image.url; this._addFinishedPhoto(id, url); + this.trigger('change'); }, // replace the first photo placeholder with the finished uploaded image and @@ -114,6 +115,8 @@ app.views.PublisherUploader = Backbone.View.extend({ // no more photos left... self.options.publisher.el_wrapper.removeClass('with_attachments'); } + + self.trigger('change'); }); } }); diff --git a/app/assets/javascripts/app/views/publisher_view.js b/app/assets/javascripts/app/views/publisher_view.js index e0f24dbca45..9d8749e935a 100644 --- a/app/assets/javascripts/app/views/publisher_view.js +++ b/app/assets/javascripts/app/views/publisher_view.js @@ -93,6 +93,7 @@ app.views.Publisher = Backbone.View.extend({ el: this.$('#file-upload'), publisher: this }); + this.view_uploader.on('change', this.checkSubmitAvailability, this); }, diff --git a/app/models/status_message.rb b/app/models/status_message.rb index 472fbcff1eb..b6a9f3cb5a3 100644 --- a/app/models/status_message.rb +++ b/app/models/status_message.rb @@ -12,6 +12,10 @@ class StatusMessage < Post extract_tags_from :raw_message validates_length_of :text, :maximum => 65535, :message => I18n.t('status_messages.too_long', :count => 65535) + + # don't allow creation of empty status messages + validate :presence_of_content, on: :create, if: proc { |sm| sm.author.local? } + xml_name :status_message xml_attr :raw_message xml_attr :photos, :as => [Photo] @@ -23,7 +27,7 @@ class StatusMessage < Post # a StatusMessage is federated before its photos are so presence_of_content() fails erroneously if no text is present # therefore, we put the validation in a before_destory callback instead of a validation - before_destroy :presence_of_content + before_destroy :absence_of_content attr_accessor :oembed_url attr_accessor :open_graph_url @@ -165,6 +169,12 @@ def address protected def presence_of_content + if text_and_photos_blank? + errors[:base] << "Cannot create a StatusMessage without content" + end + end + + def absence_of_content unless text_and_photos_blank? errors[:base] << "Cannot destory a StatusMessage with text and/or photos present" end diff --git a/features/desktop/posts_from_main_page.feature b/features/desktop/posts_from_main_page.feature index d6fa6d3bf61..ea528a40c06 100644 --- a/features/desktop/posts_from_main_page.feature +++ b/features/desktop/posts_from_main_page.feature @@ -99,6 +99,7 @@ Feature: posting from the main page When I attach the file "spec/fixtures/button.png" to hidden "file" within "#file-upload" And I click to delete the first uploaded photo Then I should not see an uploaded image within the photo drop zone + And I should not be able to submit the publisher Scenario: back out of uploading a picture to a post with text Given I expand the publisher diff --git a/features/step_definitions/posts_steps.rb b/features/step_definitions/posts_steps.rb index 3b7a8e02763..6c374296bdf 100644 --- a/features/step_definitions/posts_steps.rb +++ b/features/step_definitions/posts_steps.rb @@ -18,6 +18,10 @@ all(".stream_element").should be_empty end +Then /^I should not be able to submit the publisher$/ do + expect(publisher_submittable?).to be_false +end + Given /^"([^"]*)" has a public post with text "([^"]*)"$/ do |email, text| user = User.find_by_email(email) user.post(:status_message, :text => text, :public => true, :to => user.aspects) diff --git a/features/support/publishing_cuke_helpers.rb b/features/support/publishing_cuke_helpers.rb index b694a5f9315..90c0482e8d9 100644 --- a/features/support/publishing_cuke_helpers.rb +++ b/features/support/publishing_cuke_helpers.rb @@ -17,6 +17,11 @@ def click_publisher ') end + def publisher_submittable? + submit_btn = find("#publisher input[type=submit]") + !submit_btn[:disabled] + end + def expand_first_post within(".stream_element", match: :first) do find(".expander").click diff --git a/spec/models/status_message_spec.rb b/spec/models/status_message_spec.rb index 28db3a47a44..49db951c2ab 100644 --- a/spec/models/status_message_spec.rb +++ b/spec/models/status_message_spec.rb @@ -101,23 +101,30 @@ end end - it "should have either a message or at least one photo" do - n = FactoryGirl.build(:status_message, :text => nil) -# n.valid?.should be_false + context "emptyness" do + it "needs either a message or at least one photo" do + n = @user.build_post(:status_message, :text => nil) + n.should_not be_valid -# n.text = "" -# n.valid?.should be_false + n.text = "" + n.should_not be_valid - n.text = "wales" - n.valid?.should be_true - n.text = nil + n.text = "wales" + n.should be_valid + n.text = nil - photo = @user.build_post(:photo, :user_file => uploaded_photo, :to => @aspect.id) - photo.save! + photo = @user.build_post(:photo, :user_file => uploaded_photo, :to => @aspect.id) + photo.save! - n.photos << photo - n.valid?.should be_true - n.errors.full_messages.should == [] + n.photos << photo + n.should be_valid + n.errors.full_messages.should == [] + end + + it "doesn't check for content when author is remote (federation...)" do + p = FactoryGirl.build(:status_message, text: nil) + p.should be_valid + end end it 'should be postable through the user' do