Vote #64911
完了Convert Enumerations to single table inheritance (STI)
100%
説明
I think Redmine's outgrown it's current Enumeration class and it's time to move Enumerations to a STI class. This should remove a lot of the code around Enumeration options and make the internal API cleaner:
- @Enumerations.activities@ => @Activity.all@
- @Enumerations.priorities@ => @Priority.all@
journals
--------------------------------------------------------------------------------
Starting to work on this in a GitHub branch: http://github.com/edavis10/redmine_branches/tree/sti-enumerations
--------------------------------------------------------------------------------
I think I'm done converting @Enumeration@ to an STI class. I've updated the database, tests, and application code. I tried to keep the current API but add deprecation warnings to give third party developers enough time to update their code. I also left the @opt@ column in, it could probably be removed also if we provide some deprecation wrapper like:
<pre><code class="ruby">
# Activity class
def opt
ActiveSupport::Deprecation.warn('...')
return 'ACTI'
end
</code></pre>
Could I get another set of eyes to review my patches before I commit this to trunk? Please +1 if you reviewed the code and it looks good to commit. The code is on "GitHub":http://github.com/edavis10/redmine_branches/tree/sti-enumerations or as the attached patch.
Developed off of r2615
--------------------------------------------------------------------------------
Also a nice side benefit of this is that a plugin can add a subclass of @Enumeration@ and automagically get all the methods and be added to the Enumeration Administration panel. This might open up the doors for some more integrated plugins.
--------------------------------------------------------------------------------
I didn't test it but the patch looks good. A few things:
# I would really prefer to split the migration: one that adds the column, ones that populates it. So that if an error is raised when updating for some reason, you'll still be able to rerun db:migrate without getting an error because the column to add already exists.
# Since we now have dedicated models, we should better use the standard @.human_name@ rails class method rather than @Enumeration.option_name@. Of course, translation files must be adjusted so that classes names are properly translated.
# We have @DocumentCategory@, @IssuePriority@. Maybe we should have @TimeEntryActivity@ ?
# Maybe you could declare some has_many relations in subclasses to clean up the code, eg:
<pre>
class IssuePriority < Enumeration
has_many :issues, :foreign_key => 'priority_id'
def objects_count
issues.count
end
def transfer_relations(to)
issues.update_all("priority_id = #{to.id}")
end
end
</pre>
--------------------------------------------------------------------------------
Jean-Philippe Lang wrote:
> # I would really prefer to split the migration: one that adds the column, ones that populates it. So that if an error is raised when updating for some reason, you'll still be able to rerun db:migrate without getting an error because the column to add already exists.
Good idea.
> # Since we now have dedicated models, we should better use the standard @.human_name@ rails class method rather than @Enumeration.option_name@. Of course, translation files must be adjusted so that classes names are properly translated.
I agree, with the split @option_name@ really isn't needed except for two views.
> # We have @DocumentCategory@, @IssuePriority@. Maybe we should have @TimeEntryActivity@ ?
Do you think Activities would ever be used for things other than a TimeEntry?
> # Maybe you could declare some has_many relations in subclasses to clean up the code, eg:
That's a good suggestion. I was trying to keep this patch so it's just rearranging the internals but I could see the relationships being useful.
FYI: I've pushed up some updates to GitHub for the EnumerationController. I had a few bugs where the type wasn't being set (can't be mass-assigned). I'll create a new patch for here soon.
--------------------------------------------------------------------------------
I've updated the patch and the code on "GitHub":http://github.com/edavis10/redmine_branches/tree/sti-enumerations.
* Changed the @Enumeration@ unit tests to use the generic @Enumeration@ class
* Added unit tests for the three STI classes
* Renamed @Activity@ to @TimeEntryActivity@
* Split the migration
* Added the @has_many@ relationship to each STI class
* Added @Enumeration#opt@ to return the old value but with a deprecation warning.
Can I get another review? I'd like to commit this soon if possible.
--------------------------------------------------------------------------------
Fixed a small bug, when reordering an Enumeration the type would be lost.
--------------------------------------------------------------------------------
Changed the Enumerations in r2777. I'll post in the forums so plugin developers are aware of the change.
--------------------------------------------------------------------------------