##// 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 def create
150 def create
151 call_hook(:controller_issues_new_before_save, { :params => params, :issue => @issue })
151 call_hook(:controller_issues_new_before_save, { :params => params, :issue => @issue })
152 @issue.save_attachments(params[:attachments])
152 if @issue.save
153 if @issue.save
153 attachments = Attachment.attach_files(@issue, params[:attachments])
154 call_hook(:controller_issues_new_after_save, { :params => params, :issue => @issue})
154 call_hook(:controller_issues_new_after_save, { :params => params, :issue => @issue})
155 respond_to do |format|
155 respond_to do |format|
156 format.html {
156 format.html {
@@ -181,6 +181,7 class IssuesController < ApplicationController
181
181
182 def update
182 def update
183 return unless update_issue_from_params
183 return unless update_issue_from_params
184 @issue.save_attachments(params[:attachments])
184 saved = false
185 saved = false
185 begin
186 begin
186 saved = @issue.save_issue_with_child_records(params, @time_entry)
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 belongs_to :container, :polymorphic => true
21 belongs_to :container, :polymorphic => true
22 belongs_to :author, :class_name => "User", :foreign_key => "author_id"
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 validates_length_of :filename, :maximum => 255
25 validates_length_of :filename, :maximum => 255
26 validates_length_of :disk_filename, :maximum => 255
26 validates_length_of :disk_filename, :maximum => 255
27 validate :validate_max_file_size
27 validate :validate_max_file_size
@@ -49,6 +49,15 class Attachment < ActiveRecord::Base
49 before_save :files_to_final_location
49 before_save :files_to_final_location
50 after_destroy :delete_from_disk
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 # Returns an unsaved copy of the attachment
61 # Returns an unsaved copy of the attachment
53 def copy(attributes=nil)
62 def copy(attributes=nil)
54 copy = self.class.new
63 copy = self.class.new
@@ -121,15 +130,15 class Attachment < ActiveRecord::Base
121 end
130 end
122
131
123 def project
132 def project
124 container.project
133 container.try(:project)
125 end
134 end
126
135
127 def visible?(user=User.current)
136 def visible?(user=User.current)
128 container.attachments_visible?(user)
137 container && container.attachments_visible?(user)
129 end
138 end
130
139
131 def deletable?(user=User.current)
140 def deletable?(user=User.current)
132 container.attachments_deletable?(user)
141 container && container.attachments_deletable?(user)
133 end
142 end
134
143
135 def image?
144 def image?
@@ -149,32 +158,31 class Attachment < ActiveRecord::Base
149 File.readable?(diskfile)
158 File.readable?(diskfile)
150 end
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 # Bulk attaches a set of files to an object
177 # Bulk attaches a set of files to an object
153 #
178 #
154 # Returns a Hash of the results:
179 # Returns a Hash of the results:
155 # :files => array of the attached files
180 # :files => array of the attached files
156 # :unsaved => array of the files that could not be attached
181 # :unsaved => array of the files that could not be attached
157 def self.attach_files(obj, attachments)
182 def self.attach_files(obj, attachments)
158 attached = []
183 result = obj.save_attachments(attachments, User.current)
159 if attachments && attachments.is_a?(Hash)
184 obj.attach_saved_attachments
160 attachments.each_value do |attachment|
185 result
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}
178 end
186 end
179
187
180 def self.latest_attach(attachments, filename)
188 def self.latest_attach(attachments, filename)
@@ -183,6 +191,11 class Attachment < ActiveRecord::Base
183 }
191 }
184 end
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 private
199 private
187
200
188 # Physically deletes the file from the file system
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 s
680 s
681 end
681 end
682
682
683 # Saves an issue, time_entry, attachments, and a journal from the parameters
683 # Saves an issue and a time_entry from the parameters
684 # Returns false if save fails
685 def save_issue_with_child_records(params, existing_time_entry=nil)
684 def save_issue_with_child_records(params, existing_time_entry=nil)
686 Issue.transaction do
685 Issue.transaction do
687 if params[:time_entry] && (params[:time_entry][:hours].present? || params[:time_entry][:comments].present?) && User.current.allowed_to?(:log_time, project)
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 self.time_entries << @time_entry
693 self.time_entries << @time_entry
695 end
694 end
696
695
697 if valid?
696 # TODO: Rename hook
698 attachments = Attachment.attach_files(self, params[:attachments])
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 # TODO: Rename hook
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})
700 Redmine::Hook.call_hook(:controller_issues_edit_after_save, { :params => params, :issue => self, :time_entry => @time_entry, :journal => @current_journal})
701 begin
701 else
702 if save
702 raise ActiveRecord::Rollback
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
712 end
703 end
713 end
704 end
714 end
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 <span id="attachments_fields">
9 <span id="attachments_fields">
2 <span>
10 <span>
3 <%= file_field_tag 'attachments[1][file]', :size => 30, :id => nil, :class => 'file',
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 <%= wikitoolbar_for 'notes' %>
31 <%= wikitoolbar_for 'notes' %>
32 <%= call_hook(:view_issues_edit_notes_bottom, { :issue => @issue, :notes => @notes, :form => f }) %>
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 </fieldset>
35 </fieldset>
36 </div>
36 </div>
37
37
Show file before | Show file after | Hide comments
@@ -18,7 +18,7
18 </p>
18 </p>
19 <% end %>
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 <% if @issue.safe_attribute? 'watcher_user_ids' -%>
23 <% if @issue.safe_attribute? 'watcher_user_ids' -%>
24 <p id="watchers_form"><label><%= l(:label_issue_watchers) %></label>
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 set_tmp_attachments_directory
210 set_tmp_attachments_directory
211 end
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 def test_download_text_file
221 def test_download_text_file
214 get :download, :id => 4
222 get :download, :id => 4
215 assert_response :success
223 assert_response :success
Show file before | Show file after | Hide comments
@@ -1663,6 +1663,69 class IssuesControllerTest < ActionController::TestCase
1663 assert_equal 59, File.size(attachment.diskfile)
1663 assert_equal 59, File.size(attachment.diskfile)
1664 end
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 context "without workflow privilege" do
1729 context "without workflow privilege" do
1667 setup do
1730 setup do
1668 Workflow.delete_all(["role_id = ?", Role.anonymous.id])
1731 Workflow.delete_all(["role_id = ?", Role.anonymous.id])
@@ -2301,6 +2364,72 class IssuesControllerTest < ActionController::TestCase
2301 assert mail.body.include?('testfile.txt')
2364 assert mail.body.include?('testfile.txt')
2302 end
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 def test_put_update_with_attachment_that_fails_to_save
2433 def test_put_update_with_attachment_that_fails_to_save
2305 set_tmp_attachments_directory
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 assert_no_difference 'Journal.count' do
59 assert_no_difference 'Journal.count' do
60 assert_no_difference 'TimeEntry.count' do
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 put :update,
88 put :update,
63 :id => issue.id,
89 :id => issue.id,
64 :issue => {
90 :issue => {
@@ -74,10 +100,9 class IssuesControllerTransactionTest < ActionController::TestCase
74
100
75 assert_response :success
101 assert_response :success
76 assert_template 'edit'
102 assert_template 'edit'
77 assert_tag 'div', :attributes => {:class => 'conflict'}
103 attachment = Attachment.first(:order => 'id DESC')
78 assert_tag 'input', :attributes => {:name => 'conflict_resolution', :value => 'overwrite'}
104 assert_tag 'input', :attributes => {:name => 'attachments[p0][token]', :value => attachment.token}
79 assert_tag 'input', :attributes => {:name => 'conflict_resolution', :value => 'add_notes'}
105 assert_tag 'span', :content => /testfile.txt/
80 assert_tag 'input', :attributes => {:name => 'conflict_resolution', :value => 'cancel'}
81 end
106 end
82
107
83 def test_update_stale_issue_without_notes_should_not_show_add_notes_option
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 set_tmp_attachments_directory
38 set_tmp_attachments_directory
39 end
39 end
40
40
41 def test_container_for_new_attachment_should_be_nil
42 assert_nil Attachment.new.container
43 end
44
41 def test_create
45 def test_create
42 a = Attachment.new(:container => Issue.find(1),
46 a = Attachment.new(:container => Issue.find(1),
43 :file => uploaded_test_file("testfile.txt", "text/plain"),
47 :file => uploaded_test_file("testfile.txt", "text/plain"),
Show file before | Show file after | Hide comments
@@ -32,8 +32,8 module Redmine
32 has_many :attachments, options.merge(:as => :container,
32 has_many :attachments, options.merge(:as => :container,
33 :order => "#{Attachment.table_name}.created_on",
33 :order => "#{Attachment.table_name}.created_on",
34 :dependent => :destroy)
34 :dependent => :destroy)
35 attr_accessor :unsaved_attachments
36 send :include, Redmine::Acts::Attachable::InstanceMethods
35 send :include, Redmine::Acts::Attachable::InstanceMethods
36 before_save :attach_saved_attachments
37 end
37 end
38 end
38 end
39
39
@@ -52,6 +52,43 module Redmine
52 user.allowed_to?(self.class.attachable_options[:delete_permission], self.project)
52 user.allowed_to?(self.class.attachable_options[:delete_permission], self.project)
53 end
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 module ClassMethods
92 module ClassMethods
56 end
93 end
57 end
94 end
General Comments 0
You need to be logged in to leave comments. Login now