##// END OF EJS Templates
jro_apps » redmine
RSS
Better handling of attachments when issue validation fails (#10253)....
Jean-Philippe Lang -
r8771:d4e6355eb3f2
parent child
Show More
Side by Side Unified
Context file:
r8771:d4e6355eb3f2 Loading diff...:
Show file before | Show file after | Hide comments
@@ -149,8 +149,8 class IssuesController < ApplicationController
149 149
150 150 def create
151 151 call_hook(:controller_issues_new_before_save, { :params => params, :issue => @issue })
152 @issue.save_attachments(params[:attachments])
152 153 if @issue.save
153 attachments = Attachment.attach_files(@issue, params[:attachments])
154 154 call_hook(:controller_issues_new_after_save, { :params => params, :issue => @issue})
155 155 respond_to do |format|
156 156 format.html {
@@ -181,6 +181,7 class IssuesController < ApplicationController
181 181
182 182 def update
183 183 return unless update_issue_from_params
184 @issue.save_attachments(params[:attachments])
184 185 saved = false
185 186 begin
186 187 saved = @issue.save_issue_with_child_records(params, @time_entry)
Show file before | Show file after | Hide comments
@@ -21,7 +21,7 class Attachment < ActiveRecord::Base
21 21 belongs_to :container, :polymorphic => true
22 22 belongs_to :author, :class_name => "User", :foreign_key => "author_id"
23 23
24 validates_presence_of :container, :filename, :author
24 validates_presence_of :filename, :author
25 25 validates_length_of :filename, :maximum => 255
26 26 validates_length_of :disk_filename, :maximum => 255
27 27 validate :validate_max_file_size
@@ -49,6 +49,15 class Attachment < ActiveRecord::Base
49 49 before_save :files_to_final_location
50 50 after_destroy :delete_from_disk
51 51
52 def container_with_blank_type_check
53 if container_type.blank?
54 nil
55 else
56 container_without_blank_type_check
57 end
58 end
59 alias_method_chain :container, :blank_type_check unless method_defined?(:container_without_blank_type_check)
60
52 61 # Returns an unsaved copy of the attachment
53 62 def copy(attributes=nil)
54 63 copy = self.class.new
@@ -121,15 +130,15 class Attachment < ActiveRecord::Base
121 130 end
122 131
123 132 def project
124 container.project
133 container.try(:project)
125 134 end
126 135
127 136 def visible?(user=User.current)
128 container.attachments_visible?(user)
137 container && container.attachments_visible?(user)
129 138 end
130 139
131 140 def deletable?(user=User.current)
132 container.attachments_deletable?(user)
141 container && container.attachments_deletable?(user)
133 142 end
134 143
135 144 def image?
@@ -149,32 +158,31 class Attachment < ActiveRecord::Base
149 158 File.readable?(diskfile)
150 159 end
151 160
161 # Returns the attachment token
162 def token
163 "#{id}.#{digest}"
164 end
165
166 # Finds an attachment that matches the given token and that has no container
167 def self.find_by_token(token)
168 if token.to_s =~ /^(\d+)\.([0-9a-f]+)$/
169 attachment_id, attachment_digest = $1, $2
170 attachment = Attachment.first(:conditions => {:id => attachment_id, :digest => attachment_digest})
171 if attachment && attachment.container.nil?
172 attachment
173 end
174 end
175 end
176
152 177 # Bulk attaches a set of files to an object
153 178 #
154 179 # Returns a Hash of the results:
155 180 # :files => array of the attached files
156 181 # :unsaved => array of the files that could not be attached
157 182 def self.attach_files(obj, attachments)
158 attached = []
159 if attachments && attachments.is_a?(Hash)
160 attachments.each_value do |attachment|
161 file = attachment['file']
162 next unless file && file.size > 0
163 a = Attachment.create(:container => obj,
164 :file => file,
165 :description => attachment['description'].to_s.strip,
166 :author => User.current)
167 obj.attachments << a
168
169 if a.new_record?
170 obj.unsaved_attachments ||= []
171 obj.unsaved_attachments << a
172 else
173 attached << a
174 end
175 end
176 end
177 {:files => attached, :unsaved => obj.unsaved_attachments}
183 result = obj.save_attachments(attachments, User.current)
184 obj.attach_saved_attachments
185 result
178 186 end
179 187
180 188 def self.latest_attach(attachments, filename)
@@ -183,6 +191,11 class Attachment < ActiveRecord::Base
183 191 }
184 192 end
185 193
194 def self.prune(age=1.day)
195 attachments = Attachment.all(:conditions => ["created_on < ? AND (container_type IS NULL OR container_type = ''", Time.now - age])
196 attachments.each(&:destroy)
197 end
198
186 199 private
187 200
188 201 # Physically deletes the file from the file system
Show file before | Show file after | Hide comments
@@ -680,8 +680,7 class Issue < ActiveRecord::Base
680 680 s
681 681 end
682 682
683 # Saves an issue, time_entry, attachments, and a journal from the parameters
684 # Returns false if save fails
683 # Saves an issue and a time_entry from the parameters
685 684 def save_issue_with_child_records(params, existing_time_entry=nil)
686 685 Issue.transaction do
687 686 if params[:time_entry] && (params[:time_entry][:hours].present? || params[:time_entry][:comments].present?) && User.current.allowed_to?(:log_time, project)
@@ -694,21 +693,13 class Issue < ActiveRecord::Base
694 693 self.time_entries << @time_entry
695 694 end
696 695
697 if valid?
698 attachments = Attachment.attach_files(self, params[:attachments])
696 # TODO: Rename hook
697 Redmine::Hook.call_hook(:controller_issues_edit_before_save, { :params => params, :issue => self, :time_entry => @time_entry, :journal => @current_journal})
698 if save
699 699 # TODO: Rename hook
700 Redmine::Hook.call_hook(:controller_issues_edit_before_save, { :params => params, :issue => self, :time_entry => @time_entry, :journal => @current_journal})
701 begin
702 if save
703 # TODO: Rename hook
704 Redmine::Hook.call_hook(:controller_issues_edit_after_save, { :params => params, :issue => self, :time_entry => @time_entry, :journal => @current_journal})
705 else
706 raise ActiveRecord::Rollback
707 end
708 rescue ActiveRecord::StaleObjectError
709 attachments[:files].each(&:destroy)
710 raise ActiveRecord::StaleObjectError
711 end
700 Redmine::Hook.call_hook(:controller_issues_edit_after_save, { :params => params, :issue => self, :time_entry => @time_entry, :journal => @current_journal})
701 else
702 raise ActiveRecord::Rollback
712 703 end
713 704 end
714 705 end
Show file before | Show file after | Hide comments
@@ -1,3 +1,11
1 <% if defined?(container) && container && container.saved_attachments %>
2 <% container.saved_attachments.each_with_index do |attachment, i| %>
3 <span class="icon icon-attachment" style="display:block; line-height:1.5em;">
4 <%= h(attachment.filename) %> (<%= number_to_human_size(attachment.filesize) %>)
5 <%= hidden_field_tag "attachments[p#{i}][token]", "#{attachment.id}.#{attachment.digest}" %>
6 </span>
7 <% end %>
8 <% end %>
1 9 <span id="attachments_fields">
2 10 <span>
3 11 <%= file_field_tag 'attachments[1][file]', :size => 30, :id => nil, :class => 'file',
Show file before | Show file after | Hide comments
@@ -31,7 +31,7
31 31 <%= wikitoolbar_for 'notes' %>
32 32 <%= call_hook(:view_issues_edit_notes_bottom, { :issue => @issue, :notes => @notes, :form => f }) %>
33 33
34 <p><%=l(:label_attachment_plural)%><br /><%= render :partial => 'attachments/form' %></p>
34 <p><%=l(:label_attachment_plural)%><br /><%= render :partial => 'attachments/form', :locals => {:container => @issue} %></p>
35 35 </fieldset>
36 36 </div>
37 37
Show file before | Show file after | Hide comments
@@ -18,7 +18,7
18 18 </p>
19 19 <% end %>
20 20
21 <p id="attachments_form"><%= label_tag('attachments[1][file]', l(:label_attachment_plural))%><%= render :partial => 'attachments/form' %></p>
21 <p id="attachments_form"><%= label_tag('attachments[1][file]', l(:label_attachment_plural))%><%= render :partial => 'attachments/form', :locals => {:container => @issue} %></p>
22 22
23 23 <% if @issue.safe_attribute? 'watcher_user_ids' -%>
24 24 <p id="watchers_form"><label><%= l(:label_issue_watchers) %></label>
Show file before | Show file after | Hide comments
@@ -210,6 +210,14 class AttachmentsControllerTest < ActionController::TestCase
210 210 set_tmp_attachments_directory
211 211 end
212 212
213 def test_show_file_without_container_should_be_denied
214 attachment = Attachment.create!(:file => uploaded_test_file("testfile.txt", "text/plain"), :author_id => 2)
215
216 @request.session[:user_id] = 2
217 get :show, :id => attachment.id
218 assert_response 403
219 end
220
213 221 def test_download_text_file
214 222 get :download, :id => 4
215 223 assert_response :success
Show file before | Show file after | Hide comments
@@ -1663,6 +1663,69 class IssuesControllerTest < ActionController::TestCase
1663 1663 assert_equal 59, File.size(attachment.diskfile)
1664 1664 end
1665 1665
1666 def test_post_create_with_failure_should_save_attachments
1667 set_tmp_attachments_directory
1668 @request.session[:user_id] = 2
1669
1670 assert_no_difference 'Issue.count' do
1671 assert_difference 'Attachment.count' do
1672 post :create, :project_id => 1,
1673 :issue => { :tracker_id => '1', :subject => '' },
1674 :attachments => {'1' => {'file' => uploaded_test_file('testfile.txt', 'text/plain'), 'description' => 'test file'}}
1675 assert_response :success
1676 assert_template 'new'
1677 end
1678 end
1679
1680 attachment = Attachment.first(:order => 'id DESC')
1681 assert_equal 'testfile.txt', attachment.filename
1682 assert File.exists?(attachment.diskfile)
1683 assert_nil attachment.container
1684
1685 assert_tag 'input', :attributes => {:name => 'attachments[p0][token]', :value => attachment.token}
1686 assert_tag 'span', :content => /testfile.txt/
1687 end
1688
1689 def test_post_create_with_failure_should_keep_saved_attachments
1690 set_tmp_attachments_directory
1691 attachment = Attachment.create!(:file => uploaded_test_file("testfile.txt", "text/plain"), :author_id => 2)
1692 @request.session[:user_id] = 2
1693
1694 assert_no_difference 'Issue.count' do
1695 assert_no_difference 'Attachment.count' do
1696 post :create, :project_id => 1,
1697 :issue => { :tracker_id => '1', :subject => '' },
1698 :attachments => {'p0' => {'token' => attachment.token}}
1699 assert_response :success
1700 assert_template 'new'
1701 end
1702 end
1703
1704 assert_tag 'input', :attributes => {:name => 'attachments[p0][token]', :value => attachment.token}
1705 assert_tag 'span', :content => /testfile.txt/
1706 end
1707
1708 def test_post_create_should_attach_saved_attachments
1709 set_tmp_attachments_directory
1710 attachment = Attachment.create!(:file => uploaded_test_file("testfile.txt", "text/plain"), :author_id => 2)
1711 @request.session[:user_id] = 2
1712
1713 assert_difference 'Issue.count' do
1714 assert_no_difference 'Attachment.count' do
1715 post :create, :project_id => 1,
1716 :issue => { :tracker_id => '1', :subject => 'Saved attachments' },
1717 :attachments => {'p0' => {'token' => attachment.token}}
1718 assert_response 302
1719 end
1720 end
1721
1722 issue = Issue.first(:order => 'id DESC')
1723 assert_equal 1, issue.attachments.count
1724
1725 attachment.reload
1726 assert_equal issue, attachment.container
1727 end
1728
1666 1729 context "without workflow privilege" do
1667 1730 setup do
1668 1731 Workflow.delete_all(["role_id = ?", Role.anonymous.id])
@@ -2301,6 +2364,72 class IssuesControllerTest < ActionController::TestCase
2301 2364 assert mail.body.include?('testfile.txt')
2302 2365 end
2303 2366
2367 def test_put_update_with_failure_should_save_attachments
2368 set_tmp_attachments_directory
2369 @request.session[:user_id] = 2
2370
2371 assert_no_difference 'Journal.count' do
2372 assert_difference 'Attachment.count' do
2373 put :update, :id => 1,
2374 :issue => { :subject => '' },
2375 :attachments => {'1' => {'file' => uploaded_test_file('testfile.txt', 'text/plain'), 'description' => 'test file'}}
2376 assert_response :success
2377 assert_template 'edit'
2378 end
2379 end
2380
2381 attachment = Attachment.first(:order => 'id DESC')
2382 assert_equal 'testfile.txt', attachment.filename
2383 assert File.exists?(attachment.diskfile)
2384 assert_nil attachment.container
2385
2386 assert_tag 'input', :attributes => {:name => 'attachments[p0][token]', :value => attachment.token}
2387 assert_tag 'span', :content => /testfile.txt/
2388 end
2389
2390 def test_put_update_with_failure_should_keep_saved_attachments
2391 set_tmp_attachments_directory
2392 attachment = Attachment.create!(:file => uploaded_test_file("testfile.txt", "text/plain"), :author_id => 2)
2393 @request.session[:user_id] = 2
2394
2395 assert_no_difference 'Journal.count' do
2396 assert_no_difference 'Attachment.count' do
2397 put :update, :id => 1,
2398 :issue => { :subject => '' },
2399 :attachments => {'p0' => {'token' => attachment.token}}
2400 assert_response :success
2401 assert_template 'edit'
2402 end
2403 end
2404
2405 assert_tag 'input', :attributes => {:name => 'attachments[p0][token]', :value => attachment.token}
2406 assert_tag 'span', :content => /testfile.txt/
2407 end
2408
2409 def test_put_update_should_attach_saved_attachments
2410 set_tmp_attachments_directory
2411 attachment = Attachment.create!(:file => uploaded_test_file("testfile.txt", "text/plain"), :author_id => 2)
2412 @request.session[:user_id] = 2
2413
2414 assert_difference 'Journal.count' do
2415 assert_difference 'JournalDetail.count' do
2416 assert_no_difference 'Attachment.count' do
2417 put :update, :id => 1,
2418 :notes => 'Attachment added',
2419 :attachments => {'p0' => {'token' => attachment.token}}
2420 assert_redirected_to '/issues/1'
2421 end
2422 end
2423 end
2424
2425 attachment.reload
2426 assert_equal Issue.find(1), attachment.container
2427
2428 journal = Journal.first(:order => 'id DESC')
2429 assert_equal 1, journal.details.size
2430 assert_equal 'testfile.txt', journal.details.first.value
2431 end
2432
2304 2433 def test_put_update_with_attachment_that_fails_to_save
2305 2434 set_tmp_attachments_directory
2306 2435
Show file before | Show file after | Hide comments
@@ -58,7 +58,33 class IssuesControllerTransactionTest < ActionController::TestCase
58 58
59 59 assert_no_difference 'Journal.count' do
60 60 assert_no_difference 'TimeEntry.count' do
61 assert_no_difference 'Attachment.count' do
61 put :update,
62 :id => issue.id,
63 :issue => {
64 :fixed_version_id => 4,
65 :lock_version => (issue.lock_version - 1)
66 },
67 :notes => 'My notes',
68 :time_entry => { :hours => '2.5', :comments => '', :activity_id => TimeEntryActivity.first.id }
69 end
70 end
71
72 assert_response :success
73 assert_template 'edit'
74 assert_tag 'div', :attributes => {:class => 'conflict'}
75 assert_tag 'input', :attributes => {:name => 'conflict_resolution', :value => 'overwrite'}
76 assert_tag 'input', :attributes => {:name => 'conflict_resolution', :value => 'add_notes'}
77 assert_tag 'input', :attributes => {:name => 'conflict_resolution', :value => 'cancel'}
78 end
79
80 def test_update_stale_issue_should_save_attachments
81 set_tmp_attachments_directory
82 issue = Issue.find(2)
83 @request.session[:user_id] = 2
84
85 assert_no_difference 'Journal.count' do
86 assert_no_difference 'TimeEntry.count' do
87 assert_difference 'Attachment.count' do
62 88 put :update,
63 89 :id => issue.id,
64 90 :issue => {
@@ -74,10 +100,9 class IssuesControllerTransactionTest < ActionController::TestCase
74 100
75 101 assert_response :success
76 102 assert_template 'edit'
77 assert_tag 'div', :attributes => {:class => 'conflict'}
78 assert_tag 'input', :attributes => {:name => 'conflict_resolution', :value => 'overwrite'}
79 assert_tag 'input', :attributes => {:name => 'conflict_resolution', :value => 'add_notes'}
80 assert_tag 'input', :attributes => {:name => 'conflict_resolution', :value => 'cancel'}
103 attachment = Attachment.first(:order => 'id DESC')
104 assert_tag 'input', :attributes => {:name => 'attachments[p0][token]', :value => attachment.token}
105 assert_tag 'span', :content => /testfile.txt/
81 106 end
82 107
83 108 def test_update_stale_issue_without_notes_should_not_show_add_notes_option
Show file before | Show file after | Hide comments
@@ -38,6 +38,10 class AttachmentTest < ActiveSupport::TestCase
38 38 set_tmp_attachments_directory
39 39 end
40 40
41 def test_container_for_new_attachment_should_be_nil
42 assert_nil Attachment.new.container
43 end
44
41 45 def test_create
42 46 a = Attachment.new(:container => Issue.find(1),
43 47 :file => uploaded_test_file("testfile.txt", "text/plain"),
Show file before | Show file after | Hide comments
@@ -32,8 +32,8 module Redmine
32 32 has_many :attachments, options.merge(:as => :container,
33 33 :order => "#{Attachment.table_name}.created_on",
34 34 :dependent => :destroy)
35 attr_accessor :unsaved_attachments
36 35 send :include, Redmine::Acts::Attachable::InstanceMethods
36 before_save :attach_saved_attachments
37 37 end
38 38 end
39 39
@@ -52,6 +52,43 module Redmine
52 52 user.allowed_to?(self.class.attachable_options[:delete_permission], self.project)
53 53 end
54 54
55 def saved_attachments
56 @saved_attachments ||= []
57 end
58
59 def unsaved_attachments
60 @unsaved_attachments ||= []
61 end
62
63 def save_attachments(attachments, author=User.current)
64 if attachments && attachments.is_a?(Hash)
65 attachments.each_value do |attachment|
66 a = nil
67 if file = attachment['file']
68 next unless file && file.size > 0
69 a = Attachment.create(:file => file,
70 :description => attachment['description'].to_s.strip,
71 :author => author)
72 elsif token = attachment['token']
73 a = Attachment.find_by_token(token)
74 end
75 next unless a
76 if a.new_record?
77 unsaved_attachments << a
78 else
79 saved_attachments << a
80 end
81 end
82 end
83 {:files => saved_attachments, :unsaved => unsaved_attachments}
84 end
85
86 def attach_saved_attachments
87 saved_attachments.each do |attachment|
88 self.attachments << attachment
89 end
90 end
91
55 92 module ClassMethods
56 93 end
57 94 end
General Comments 0
You need to be logged in to leave comments. Login now