Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Patch for Issue 1259 -- New #1297

Merged
merged 4 commits into from

3 participants

@seanski

@josevalim,

The revoke behavior now removes the migration as it should.

Regards,

Sean

seanski added some commits
@seanski seanski Added a template to create a migration when the model already exists.…
… Changed the generator code to check if model exists, and if it does, call the new template instead of the standard template.
9629da8
@seanski seanski Changed the order of the devise_generator methods to create the model…
… after the migration to properly use model_exists?, and I added tests to prove the generator works.
179cb2c
@seanski seanski Chagned the copy_devise_migration method to properly handle the :revo…
…ke behavior using @daf's commit: daf/devise@acf7e9e as a guide.
99d539b
@seanski seanski Added a helper to look for an modifying migration. If one is found du…
…ring :revoke, the modifying migration is deleted. If the modifying migration is not found, the creating migration is deleted.
8ad414b
@daf daf commented on the diff
...erators/active_record/templates/migration_existing.rb
((15 lines not shown))
+ t.<%= attribute.type %> :<%= attribute.name %>
+<% end -%>
+ #Uncomment below if timestamps were not included in your original model.
+ t.timestamps
+ end
+ add_index :<%= table_name %>, :email, :unique => true
+ add_index :<%= table_name %>, :reset_password_token, :unique => true
+ # add_index :<%= table_name %>, :confirmation_token, :unique => true
+ # add_index :<%= table_name %>, :unlock_token, :unique => true
+ # add_index :<%= table_name %>, :authentication_token, :unique => true
+ end
+
+ def self.down
+ #By default, we don't want to make any assumption about how to roll back a migration when your
+ #model already existed. Please edit below which fields you would like to remove in this migration.
+ raise ActiveRecord::IrreversibleMigration
@daf
daf added a note

In my attempt at this, I had added "rm_" versions of each of the... well not sure what the word is, but the t.recoverable, t.trackable etc.. that way the down could be just the inverse of the things up top. So if you had t.recoverable in the up, you could put t.rm_recoverable in the down. Easy enough to mirror after editing.

Not sure what to do with the attribute section though, as I'm not sure what those are for.

@seanski
seanski added a note

That wouldn't be a bad thing to do; however, I'm worried what would happen if we had a user model with email, for example. It would be deleted when we the migration is rolled back, and this would not be ideal. I still think it's better for the user to write the rollback code. Maybe we could add the rollback code in the migration but leave it commented out?

@daf
daf added a note

I agree - the existing email thing is very tricky. It's in database_authenticatable aleady and will blow up as it stands currently if the existing model has that field (as many will, I suspect).

I had thought to possibly handle that by detecting the existence of the model, and somehow checking its fields? Then instead of database_authenticatable, something else, like database_authenticatable_existing_email, and the complementary rm would follow suit? Getting very complicated, though...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@josevalim josevalim merged commit 79d89a3 into from
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Aug 25, 2011
  1. @seanski

    Added a template to create a migration when the model already exists.…

    seanski authored
    … Changed the generator code to check if model exists, and if it does, call the new template instead of the standard template.
Commits on Aug 28, 2011
  1. @seanski

    Changed the order of the devise_generator methods to create the model…

    seanski authored
    … after the migration to properly use model_exists?, and I added tests to prove the generator works.
  2. @seanski

    Chagned the copy_devise_migration method to properly handle the :revo…

    seanski authored
    …ke behavior using @daf's commit: daf/devise@acf7e9e as a guide.
Commits on Aug 29, 2011
  1. @seanski

    Added a helper to look for an modifying migration. If one is found du…

    seanski authored
    …ring :revoke, the modifying migration is deleted. If the modifying migration is not found, the creating migration is deleted.
This page is out of date. Refresh to see the latest.
View
24 lib/generators/active_record/devise_generator.rb
@@ -1,6 +1,7 @@
require 'rails/generators/active_record'
require 'generators/devise/orm_helpers'
+
module ActiveRecord
module Generators
class DeviseGenerator < ActiveRecord::Generators::Base
@@ -9,14 +10,27 @@ class DeviseGenerator < ActiveRecord::Generators::Base
include Devise::Generators::OrmHelpers
source_root File.expand_path("../templates", __FILE__)
- def generate_model
- invoke "active_record:model", [name], :migration => false unless model_exists? && behavior == :invoke
- end
-
def copy_devise_migration
- migration_template "migration.rb", "db/migrate/devise_create_#{table_name}"
+ exists = model_exists?
+ unless behavior == :revoke
+ unless exists
+ migration_template "migration.rb", "db/migrate/devise_create_#{table_name}"
+ else
+ migration_template "migration_existing.rb", "db/migrate/add_devise_to_#{table_name}"
+ end
+ else
+ if migration_exists?(table_name)
+ migration_template "migration_existing.rb", "db/migrate/add_devise_to_#{table_name}"
+ else
+ migration_template "migration.rb", "db/migrate/devise_create_#{table_name}"
+ end
+ end
end
+ def generate_model
+ invoke "active_record:model", [name], :migration => false unless model_exists? && behavior == :invoke
+ end
+
def inject_devise_content
inject_into_class(model_path, class_name, model_contents + <<CONTENT) if model_exists?
# Setup accessible (or protected) attributes for your model
View
32 lib/generators/active_record/templates/migration_existing.rb
@@ -0,0 +1,32 @@
+class AddDeviseTo<%= table_name.camelize %> < ActiveRecord::Migration
+ def self.up
+ change_table :<%= table_name %> do |t|
+ t.database_authenticatable :null => false
+ t.recoverable
+ t.rememberable
+ t.trackable
+
+ # t.encryptable
+ # t.confirmable
+ # t.lockable :lock_strategy => :<%= Devise.lock_strategy %>, :unlock_strategy => :<%= Devise.unlock_strategy %>
+ # t.token_authenticatable
+
+<% for attribute in attributes -%>
+ t.<%= attribute.type %> :<%= attribute.name %>
+<% end -%>
+ #Uncomment below if timestamps were not included in your original model.
+ t.timestamps
+ end
+ add_index :<%= table_name %>, :email, :unique => true
+ add_index :<%= table_name %>, :reset_password_token, :unique => true
+ # add_index :<%= table_name %>, :confirmation_token, :unique => true
+ # add_index :<%= table_name %>, :unlock_token, :unique => true
+ # add_index :<%= table_name %>, :authentication_token, :unique => true
+ end
+
+ def self.down
+ #By default, we don't want to make any assumption about how to roll back a migration when your
+ #model already existed. Please edit below which fields you would like to remove in this migration.
+ raise ActiveRecord::IrreversibleMigration
@daf
daf added a note

In my attempt at this, I had added "rm_" versions of each of the... well not sure what the word is, but the t.recoverable, t.trackable etc.. that way the down could be just the inverse of the things up top. So if you had t.recoverable in the up, you could put t.rm_recoverable in the down. Easy enough to mirror after editing.

Not sure what to do with the attribute section though, as I'm not sure what those are for.

@seanski
seanski added a note

That wouldn't be a bad thing to do; however, I'm worried what would happen if we had a user model with email, for example. It would be deleted when we the migration is rolled back, and this would not be ideal. I still think it's better for the user to write the rollback code. Maybe we could add the rollback code in the migration but leave it commented out?

@daf
daf added a note

I agree - the existing email thing is very tricky. It's in database_authenticatable aleady and will blow up as it stands currently if the existing model has that field (as many will, I suspect).

I had thought to possibly handle that by detecting the existence of the model, and somehow checking its fields? Then instead of database_authenticatable, something else, like database_authenticatable_existing_email, and the complementary rm would follow suit? Getting very complicated, though...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ end
+end
View
8 lib/generators/devise/orm_helpers.rb
@@ -14,6 +14,14 @@ def model_contents
def model_exists?
File.exists?(File.join(destination_root, model_path))
end
+
+ def migration_exists?(table_name)
+ Dir.glob("#{File.join(destination_root, migration_path)}/[0-9]*_*.rb").grep(/\d+_add_devise_to_#{table_name}.rb$/).first
+ end
+
+ def migration_path
+ @migration_path ||= File.join("db", "migrate")
+ end
def model_path
@model_path ||= File.join("app", "models", "#{file_path}.rb")
View
13 test/generators/active_record_generator_test.rb
@@ -13,9 +13,22 @@ class ActiveRecordGeneratorTest < Rails::Generators::TestCase
assert_file "app/models/monster.rb", /devise/, /attr_accessible (:[a-z_]+(, )?)+/
assert_migration "db/migrate/devise_create_monsters.rb"
end
+
+ test "update model migration when model exists" do
+ run_generator %w(monster)
+ assert_file "app/models/monster.rb"
+ run_generator %w(monster)
+ assert_migration "db/migrate/add_devise_to_monsters.rb"
+ end
test "all files are properly deleted" do
run_generator %w(monster)
+ run_generator %w(monster)
+ assert_migration "db/migrate/devise_create_monsters.rb"
+ assert_migration "db/migrate/add_devise_to_monsters.rb"
+ run_generator %w(monster), :behavior => :revoke
+ assert_no_migration "db/migrate/add_devise_to_monsters.rb"
+ assert_migration "db/migrate/devise_create_monsters.rb"
run_generator %w(monster), :behavior => :revoke
assert_no_file "app/models/monster.rb"
assert_no_migration "db/migrate/devise_create_monsters.rb"
Something went wrong with that request. Please try again.