Feature #3007

Convert Enumerations to single table inheritance (STI)

Added by Eric Davis over 8 years ago. Updated over 8 years ago.

Status:ClosedStart date:2009-03-18
Priority:NormalDue date:
Assignee:Eric Davis% Done:

100%

Category:-
Target version:0.9.0
Resolution:Fixed

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

enumeration-sti.patch Magnifier - Patch to convert Enumerations to STI - for review (35.8 KB) Eric Davis, 2009-03-25 22:50

enumeration-sti-3.patch Magnifier - Latest conversion of Enumerations to an STI class (42.8 KB) Eric Davis, 2009-04-02 00:45

enumeration-sti-4.patch Magnifier - Patch through d33663afac0b833ea18449047459aa6932c6a04d (42.8 KB) Eric Davis, 2009-04-02 02:16

Associated revisions

Revision 2777
Added by Eric Davis over 8 years ago

Changed Enumerations to use a Single Table Inheritance

  • Added migrations to change Enumerations to an STI relationship
  • Added TimeEntryActivity model (STI)
  • Added DocumentCategory model (STI)
  • Added IssuePriority model (STI)
  • Added Enumeration#get_subclasses to get a list of the subclasses of Enumeration
  • Changed Enumeration to use the STI type field instead of the opt field
  • Changed Enumeration#opt to return the old opt values but with a deprecation warning.
  • Removed Enumeration::OPTIONS
  • Removed the dynamic named_scopes in favor of specific named_scopes. Kept for
    compatibility reasons.
  • Added Enumeration#default so each subclass can easily find it's default record.
  • Fixed Enumeration#default to use the STI scoping with a fake default scope for finding Enumeration's default.
  • Added a 'all' named scope for getting all records in order by position.
  • Added Deprecation warnings to the old named_scopes in Enumerations.
  • Moved various methods off of Enumeration and onto the concrete classes
  • Changed the EnumerationsController to use types
  • Updated the Enumeration list template
  • Added has_many relationships to the Enumeration STI classes.
  • Fixes for tests.

    #3007

History

#1 Updated by Eric Davis over 8 years ago

  • Status changed from New to 7
  • Assignee set to Eric Davis

#2 Updated by Eric Davis over 8 years ago

Starting to work on this in a GitHub branch: http://github.com/edavis10/redmine_branches/tree/sti-enumerations

#3 Updated by Eric Davis over 8 years ago

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

#4 Updated by Eric Davis over 8 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.

#5 Updated by Jean-Philippe Lang over 8 years ago

I didn't test it but the patch looks good. A few things:

  1. 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.
  2. 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.
  3. We have DocumentCategory, IssuePriority. Maybe we should have TimeEntryActivity ?
  4. 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

#6 Updated by Eric Davis over 8 years ago

Jean-Philippe Lang wrote:

  1. 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.

  1. 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.

  1. We have DocumentCategory, IssuePriority. Maybe we should have TimeEntryActivity ?

Do you think Activities would ever be used for things other than a TimeEntry?

  1. 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.

#7 Updated by Eric Davis over 8 years ago

I've updated the patch and the code on GitHub.

  • 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.

#8 Updated by Eric Davis over 8 years ago

Fixed a small bug, when reordering an Enumeration the type would be lost.

#9 Updated by Eric Davis over 8 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.

Also available in: Atom PDF