Feature #3007
closedConvert Enumerations to single table inheritance (STI)
Added by Eric Davis over 15 years ago. Updated over 15 years ago.
100%
Description
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
Files
enumeration-sti.patch (35.8 KB) enumeration-sti.patch | Patch to convert Enumerations to STI - for review | Eric Davis, 2009-03-25 22:50 | |
enumeration-sti-3.patch (42.8 KB) enumeration-sti-3.patch | Latest conversion of Enumerations to an STI class | Eric Davis, 2009-04-02 00:45 | |
enumeration-sti-4.patch (42.8 KB) enumeration-sti-4.patch | Patch through d33663afac0b833ea18449047459aa6932c6a04d | Eric Davis, 2009-04-02 02:16 |
Updated by Eric Davis over 15 years ago
- Status changed from New to 7
- Assignee set to Eric Davis
Updated by Eric Davis over 15 years ago
Starting to work on this in a GitHub branch: http://github.com/edavis10/redmine_branches/tree/sti-enumerations
Updated by Eric Davis over 15 years ago
- File enumeration-sti.patch enumeration-sti.patch added
- Target version set to 0.9.0
- % Done changed from 0 to 100
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:
# Activity class
def opt
ActiveSupport::Deprecation.warn('...')
return 'ACTI'
end
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 or as the attached patch.
Developed off of r2615
Updated by Eric Davis over 15 years ago
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.
Updated by Jean-Philippe Lang over 15 years ago
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 thanEnumeration.option_name
. Of course, translation files must be adjusted so that classes names are properly translated. - We have
DocumentCategory
,IssuePriority
. Maybe we should haveTimeEntryActivity
? - Maybe you could declare some has_many relations in subclasses to clean up the code, eg:
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
Updated by Eric Davis over 15 years ago
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 thanEnumeration.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 haveTimeEntryActivity
?
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.
Updated by Eric Davis over 15 years ago
- File enumeration-sti-3.patch enumeration-sti-3.patch added
I've updated the patch and the code on GitHub.
- Changed the
Enumeration
unit tests to use the genericEnumeration
class - Added unit tests for the three STI classes
- Renamed
Activity
toTimeEntryActivity
- 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.
Updated by Eric Davis over 15 years ago
- File enumeration-sti-4.patch enumeration-sti-4.patch added
Fixed a small bug, when reordering an Enumeration the type would be lost.
Updated by Eric Davis over 15 years ago
- Status changed from 7 to Closed
- Resolution set to Fixed
Changed the Enumerations in r2777. I'll post in the forums so plugin developers are aware of the change.