Merge pull request #35 from oliverguenther/fix/document_bugs

Fix notification and Rails4-related issues

app/models/activity/document_activity_provider.rb

@@ -49,11 +49,11 @@ class Activity::DocumentActivityProvider < Activity::BaseActivityProvider
   end
 
   def event_path(event, activity)
-    url_helpers.project_document_path(url_helper_parameter(event))
+    url_helpers.project_documents_url(url_helper_parameter(event))
   end
 
   def event_url(event, activity)
-    url_helpers.project_document_url(url_helper_parameter(event))
+    url_helpers.project_documents_url(url_helper_parameter(event))
   end
 
   private

app/models/document.rb

@@ -50,9 +50,17 @@ class Document < ActiveRecord::Base
   validates_presence_of :project, :title, :category
   validates_length_of :title, :maximum => 60
 
-  scope :visible, lambda {|*args| { :include => :project,
-                                    :conditions => Project.allowed_to_condition(args.first || User.current, :view_documents) } }
-  scope :with_attachments, includes(:attachments).where("attachments.container_id is not NULL" )
+  scope :visible, lambda {
+    includes(:project)
+      .where(Project.allowed_to_condition(User.current, :view_documents))
+      .references(:projects)
+  }
+
+  scope :with_attachments, lambda {
+    includes(:attachments)
+      .where('attachments.container_id is not NULL')
+      .references(:attachments)
+  }
 
   after_initialize :set_default_category
 

app/models/document_observer.rb

@@ -34,11 +34,9 @@ class DocumentObserver < ActiveRecord::Observer
 
 
   def after_create(document)
+    return unless Setting.notified_events.include?('document_added')
 
-    return unless Notifier.notify?(:document_added)
-
-    users = User.find_all_by_mails(document.recipients)
-    users.each do |user|
+    document.recipients.each do |user|
       DocumentsMailer.document_added(user, document).deliver
     end
   end

app/views/documents/_document.html.erb

@@ -34,5 +34,5 @@ See doc/COPYRIGHT.rdoc for more details.
 <p><em><%= format_time(document.updated_on) %></em></p>
 
 <div class="wiki">
-	<%= textilizable(truncate_lines(document.description)) %>
+	<%= format_text(truncate_lines(document.description)) %>
 </div>

app/views/documents/edit.html.erb

@@ -32,7 +32,7 @@ See doc/COPYRIGHT.rdoc for more details.
 
 <%= toolbar title: Document.model_name.human %>
 
-<%= labelled_tabular_form_for @document, url: project_document_path(@document) do |f| -%>
+<%= labelled_tabular_form_for @document, url: document_path(@document), method: 'PATCH' do |f| -%>
   <%= render partial: "documents/form", locals: { f: f } %>
   <%= styled_button_tag l(:button_save), class: "-highlight -with-icon icon-yes" %>
 <% end %>

app/views/documents_mailer/document_added.html.erb

@@ -30,6 +30,6 @@ See doc/COPYRIGHT.rdoc for more details.
 
 ++#%>
 
-<p><%= link_to(@document.title, project_document_url(@document)) %> (<%= @document.category.name %>)</p>
+<p><%= link_to(@document.title, project_documents_url(@document)) %> (<%= @document.category.name %>)</p>
 
-<%= textilizable @document.description %>
+<%= format_text @document.description %>

app/views/documents_mailer/document_added.text.erb

@@ -31,6 +31,6 @@ See doc/COPYRIGHT.rdoc for more details.
 ++#%>
 
 <%= @document.title %> (<%= @document.category.name %>)
-<%= project_document_url(@document) %>
+<%= project_documents_url(@document) %>
 
 <%= @document.description %>

config/routes.rb

@@ -31,8 +31,8 @@
 
 OpenProject::Application.routes.draw do
 
-  scope 'projects/:project_id' do
-    resources :documents, :shallow => true, :as => :project_documents
+  resources :projects do
+    resources :documents, shallow: true
   end
 end
 

spec/application_helper_spec.rb

@@ -101,13 +101,13 @@ describe ApplicationHelper do
       context "By id and given project" do
         subject { format_text("#{identifier}:document##{document.id}", :project => the_other_project) }
 
-        it { is_expected.to eq("<p><a href=\"/documents/#{document.id}\" class=\"document\">Test document</a></p>") }
+        it { is_expected.to eq("<p><a class=\"document\" href=\"/documents/#{document.id}\">Test document</a></p>") }
       end
 
       context "By name and given project" do
         subject { format_text("#{identifier}:document:\"#{document.title}\"", :project => the_other_project) }
 
-        it { is_expected.to eq("<p><a href=\"/documents/#{document.id}\" class=\"document\">Test document</a></p>") }
+        it { is_expected.to eq("<p><a class=\"document\" href=\"/documents/#{document.id}\">Test document</a></p>") }
       end
 
       context "Invalid link" do

spec/controllers/documents_controller_spec.rb

@@ -165,7 +165,7 @@ LOREM
       }.to change{Document.count}.by -1
 
       expect(response).to redirect_to "/projects/#{project.identifier}/documents"
-      expect{Document.find(document.id)}.to raise_error
+      expect{Document.find(document.id)}.to raise_error ActiveRecord::RecordNotFound
     end
   end
 

spec/models/document_observer_spec.rb

@@ -53,10 +53,9 @@ describe DocumentObserver do
   it "calls the DocumentsMailer, when a new document has been added" do
     document = FactoryGirl.build(:document)
     # make sure, that we have actually someone to notify
-    allow(document).to receive(:recipients).and_return(user.mail)
+    allow(document).to receive(:recipients).and_return([user])
     # ... and notifies are actually sent out
-    allow(Notifier).to receive(:notify?).and_return(true)
-
+    Setting.notified_events = Setting.notified_events << 'document_added'
     expect(DocumentsMailer).to receive(:document_added).and_return(mail)
 
     document.save

spec/models/document_spec.rb

@@ -57,8 +57,8 @@ describe Document do
     end
 
     it "should send out email-notifications" do
-      allow(valid_document).to receive(:recipients).and_return([user.mail])
-      allow(Notifier).to receive(:notify?).with(:document_added).and_return(true)
+      allow(valid_document).to receive(:recipients).and_return([user])
+      Setting.notified_events = Setting.notified_events << 'document_added'
 
       expect{
         valid_document.save
@@ -72,7 +72,7 @@ describe Document do
 
       expect(document.recipients).not_to be_empty
       expect(document.recipients.count).to eql 1
-      expect(document.recipients).to include admin.mail
+      expect(document.recipients.map(&:mail)).to include admin.mail
     end
 
     it "should set a default-category, if none is given" do