The Refactoring
After thinking about yesterday’s refactor, I noticed that setting the flash
message is going to cause problems for future refactorings since it’s trying to break the MVC constraints. Today I found a way to decouple tracking the failed saves while keeping the flash messages up in the controllers.
Before
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
|
# vendor/plugins/acts_as_attachable/lib/acts_as_attachable.rb
module Redmine
module Acts
module Attachable
def self.included(base)
base.extend ClassMethods
end
module ClassMethods
def acts_as_attachable(options = {})
# ...
send :include, Redmine::Acts::Attachable::InstanceMethods
end
end
module InstanceMethods
# ...
end
end
end
end |
# vendor/plugins/acts_as_attachable/lib/acts_as_attachable.rb
module Redmine
module Acts
module Attachable
def self.included(base)
base.extend ClassMethods
end
module ClassMethods
def acts_as_attachable(options = {})
# ...
send :include, Redmine::Acts::Attachable::InstanceMethods
end
end
module InstanceMethods
# ...
end
end
end
end
1
2
3
4
5
6
7
8
9
|
# app/models/attachment.rb
class Attachment 0
a = Attachment.create(:container => obj,
:file => file,
:description => attachment['description'].to_s.strip,
:author => User.current)
a.new_record? ? (unsaved << a) : (attached <<a> attached, :flash => flash, :unsaved => unsaved}
end
end |
# app/models/attachment.rb
class Attachment 0
a = Attachment.create(:container => obj,
:file => file,
:description => attachment['description'].to_s.strip,
:author => User.current)
a.new_record? ? (unsaved << a) : (attached <<a> attached, :flash => flash, :unsaved => unsaved}
end
end
1
2
3
4
|
# app/controllers/application_controller.rb
class ApplicationController < ActionController::Base
# ...
end |
# app/controllers/application_controller.rb
class ApplicationController < ActionController::Base
# ...
end
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
|
# app/controllers/issues_controller.rb
class IssuesController @project, :issue => @issue, :user => User.current, :spent_on => Date.today)
@time_entry.attributes = params[:time_entry]
@issue.time_entries << @time_entry
end
if @issue.valid?
attachments = Attachment.attach_files(@issue, params[:attachments])
flash[:warning] = attachments[:flash] if attachments[:flash]
attachments[:files].each {|a| @journal.details < 'attachment', :prop_key => a.id, :value => a.filename)}
call_hook(:controller_issues_edit_before_save, { :params => params, :issue => @issue, :time_entry => @time_entry, :journal => @journal})
if @issue.save
if !@journal.new_record?
# Only send notification if something was actually changed
flash[:notice] = l(:notice_successful_update)
end
call_hook(:controller_issues_edit_after_save, { :params => params, :issue => @issue, :time_entry => @time_entry, :journal => @journal})
return true
end
end
# failure, returns false
end
end |
# app/controllers/issues_controller.rb
class IssuesController @project, :issue => @issue, :user => User.current, :spent_on => Date.today)
@time_entry.attributes = params[:time_entry]
@issue.time_entries << @time_entry
end
if @issue.valid?
attachments = Attachment.attach_files(@issue, params[:attachments])
flash[:warning] = attachments[:flash] if attachments[:flash]
attachments[:files].each {|a| @journal.details < 'attachment', :prop_key => a.id, :value => a.filename)}
call_hook(:controller_issues_edit_before_save, { :params => params, :issue => @issue, :time_entry => @time_entry, :journal => @journal})
if @issue.save
if !@journal.new_record?
# Only send notification if something was actually changed
flash[:notice] = l(:notice_successful_update)
end
call_hook(:controller_issues_edit_after_save, { :params => params, :issue => @issue, :time_entry => @time_entry, :journal => @journal})
return true
end
end
# failure, returns false
end
end
After
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
|
# vendor/plugins/acts_as_attachable/lib/acts_as_attachable.rb
module Redmine
module Acts
module Attachable
def self.included(base)
base.extend ClassMethods
end
module ClassMethods
def acts_as_attachable(options = {})
# ...
attr_accessor :unsaved_attachments
after_initialize :initialize_unsaved_attachments
send :include, Redmine::Acts::Attachable::InstanceMethods
end
end
module InstanceMethods
# ...
def initialize_unsaved_attachments
@unsaved_attachments ||= []
end
end
end
end
end |
# vendor/plugins/acts_as_attachable/lib/acts_as_attachable.rb
module Redmine
module Acts
module Attachable
def self.included(base)
base.extend ClassMethods
end
module ClassMethods
def acts_as_attachable(options = {})
# ...
attr_accessor :unsaved_attachments
after_initialize :initialize_unsaved_attachments
send :include, Redmine::Acts::Attachable::InstanceMethods
end
end
module InstanceMethods
# ...
def initialize_unsaved_attachments
@unsaved_attachments ||= []
end
end
end
end
end
1
2
3
4
5
6
7
8
9
|
# app/models/attachment.rb
class Attachment 0
a = Attachment.create(:container => obj,
:file => file,
:description => attachment['description'].to_s.strip,
:author => User.current)
a.new_record? ? (obj.unsaved_attachments << a) : (attached <<a> attached, :unsaved => obj.unsaved_attachments}
end
end |
# app/models/attachment.rb
class Attachment 0
a = Attachment.create(:container => obj,
:file => file,
:description => attachment['description'].to_s.strip,
:author => User.current)
a.new_record? ? (obj.unsaved_attachments << a) : (attached <<a> attached, :unsaved => obj.unsaved_attachments}
end
end
1
2
3
4
5
6
7
|
# app/controllers/application_controller.rb
class ApplicationController < ActionController::Base
# Renders a warning flash if obj has unsaved attachments
def render_attachment_warning_if_needed(obj)
flash[:warning] = l(:warning_attachments_not_saved, obj.unsaved_attachments.size) if obj.unsaved_attachments.present?
end
end |
# app/controllers/application_controller.rb
class ApplicationController < ActionController::Base
# Renders a warning flash if obj has unsaved attachments
def render_attachment_warning_if_needed(obj)
flash[:warning] = l(:warning_attachments_not_saved, obj.unsaved_attachments.size) if obj.unsaved_attachments.present?
end
end
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
|
# app/controllers/issues_controller.rb
class IssuesController @project, :issue => @issue, :user => User.current, :spent_on => Date.today)
@time_entry.attributes = params[:time_entry]
@issue.time_entries << @time_entry
end
if @issue.valid?
attachments = Attachment.attach_files(@issue, params[:attachments])
render_attachment_warning_if_needed(@issue)
attachments[:files].each {|a| @journal.details < 'attachment', :prop_key => a.id, :value => a.filename)}
call_hook(:controller_issues_edit_before_save, { :params => params, :issue => @issue, :time_entry => @time_entry, :journal => @journal})
if @issue.save
if !@journal.new_record?
# Only send notification if something was actually changed
flash[:notice] = l(:notice_successful_update)
end
call_hook(:controller_issues_edit_after_save, { :params => params, :issue => @issue, :time_entry => @time_entry, :journal => @journal})
return true
end
end
# failure, returns false
end
end |
# app/controllers/issues_controller.rb
class IssuesController @project, :issue => @issue, :user => User.current, :spent_on => Date.today)
@time_entry.attributes = params[:time_entry]
@issue.time_entries << @time_entry
end
if @issue.valid?
attachments = Attachment.attach_files(@issue, params[:attachments])
render_attachment_warning_if_needed(@issue)
attachments[:files].each {|a| @journal.details < 'attachment', :prop_key => a.id, :value => a.filename)}
call_hook(:controller_issues_edit_before_save, { :params => params, :issue => @issue, :time_entry => @time_entry, :journal => @journal})
if @issue.save
if !@journal.new_record?
# Only send notification if something was actually changed
flash[:notice] = l(:notice_successful_update)
end
call_hook(:controller_issues_edit_after_save, { :params => params, :issue => @issue, :time_entry => @time_entry, :journal => @journal})
return true
end
end
# failure, returns false
end
end
Review
There’s a lot going on here, so I’m going to try to break it down into smaller chunks.
-
acts_as_attachable.rb
had a new accessor added, an array to track unsaved_attachments
.
-
Attachment#attach_files
had all of it’s flash tracking removed and in it’s place it will store attachments that didn’t save to the object’s unsaved_attachments
.
- In the controllers (
IssuesController
): instead of checking the response hash from Attachment#attach_files
for a flash message, it’s checking if there is anything in object#unsaved_attachments
and displaying the flash.
- ApplicationController had a utility method
#render_attachment_warning_if_needed
to abstract the “display a flash” logic. This was needed because there are 5 controllers using Attachment#attach_files
and I didn’t want to duplicate the code in each one.
Now I think I’ll be able to finish refactoring IssuesController#issue_update
by pushing the logic down to the model.
Reference commit