Move Mailboxer models into Mailboxer namespace #61

Closed
wants to merge 12 commits into
from

Conversation

Projects
None yet
Contributor

daveworth commented Jun 16, 2012

@Roendal ,

This morning I made the first pass at namespacing the Mailboxer models to avoid conflicts as we discussed in ging/mailboxer#39 yesterday. It turns out that it was not so hard to get all tests back to passing but it definitely does break backward compatibility in an apps using the 0.6.x line of Mailboxer. As such I've bumped the gemspec version to 1.0.0a in an effort to use semantic versioning well.

A few quick notes:

  • Since it's a whole new version, rather than having many migrations I simply merged the migrations into one that contains the final result. This would break folks on the 0.6.x line but shouldn't affect new users.
  • Namespacing of database tables required explicit table names in models but this is a small cost
  • I decided to skip the namespacing of mailers but that might be a reasonable next step.
  • I know this commit is large and daunting but I attempted to make each individual commit very focussed and described reasonably by the commit message. Don't look at the whole diff at once, but rather at each commit in the chain. That should make acceptance / discussion simpler.
  • For the most part the changes are merely database table name changes, prefixing model names with Mailboxer:: and updating scopes.

If you have any questions or think changes should be made before attempting some integration with the mailboxer mainline I am more than open to that. Thanks for taking the time to even consider this.

Owner

Roendal commented Jun 16, 2012

I simply have no words. Many many thanks for the great effort you are doing with Mailboxer.

I will take a deeper look to it tomorrow as I had a hard day and need some sleeping ;)

I will now summon @atd, who is my master and guide, to ask him if he can join the discussion.

Again and again, thanks :)

Contributor

daveworth commented Jun 17, 2012

It is absolutely my pleasure. It was fun to spend some time this morning hacking on some non-client code that I know will help others! Mailboxer is a great, simple gem that will help people add real, and meaningful value to their rails apps with a minimum of pain (which is why I found it)!

Owner

atd commented Jun 18, 2012

Hi @daveworth

Many thanks for your work. Definitely, namespace in engines is a good thing.

I have only the issue of facilitating upgrade to users that have an already deployed application. So I think that releasing an intermediate version with a migration to the new schema is a must.

Contributor

daveworth commented Jun 18, 2012

@atd ,

That is a fantastic idea, and one I had not considered. I will work on that change very soon and submit a modified pull-request with it.

Owner

Roendal commented Jun 18, 2012

Great idea @atd.

I assume that there will be other changes rather than DB issues. Maybe a walkthrough to guide developers to the new version will also be interesting.

Take your time @daveworth, you have done more than enough :)

Contributor

daveworth commented Jun 20, 2012

@atd and @Roendal,

I've been thinking through the best way to approach the 0.x to 1.x data migration problem. My best approach to date is simply to include a section on upgrading in the README which includes the necessary migration code that a user performing the upgrade can copy/paste into their own migration file.

class RenameMailboxerTables < ActiveRecord::Migration
  def change
    rename_table :conversations, :mailboxer_conversations
    rename_table :notifications, :mailboxer_notifications
    rename_table :receipts,      :mailboxer_receipts

    rename_index :notifications, :mailboxer_notifications
    rename_index :receipts,      :mailboxer_receipts
  end
end

Do you think this would suffice?

Owner

atd commented Jun 20, 2012

Mmm, that would require users to create their own migration and copy/paste the solution. I think we could facilitate things a bit more.

What do you think about a generator that creates this migration?

Contributor

daveworth commented Jun 20, 2012

@atd, That's also a very good point. A generator that generates the above migration would probably be a good approach. I have no experience writing generators so this is a good place to start I guess!

Owner

Roendal commented Jun 20, 2012

Good point @atd.

As mailboxer has now rails generate mailboxer:install maybe rails generate mailboxer:upgrade could be interesting.

zolzaya commented Nov 7, 2012

+1

+1 on namespace

indriq commented Nov 23, 2012

+1

archonic commented Dec 6, 2012

I'm trying to implement into an existing application that has a table 'notifications'. I spent most of a day trying to prefix the table names, changing rake tasks and screwing up my db before finding this thread. +10! Please pull =)

Edit: In related news I'm getting this error from running rake db:migrate
Mysql2::Error: Key column 'mailboxer_notification_id' doesn't exist in table: ALTER TABLE mailboxer_receipts ADD CONSTRAINT receipts_on_notification_id FOREIGN KEY (mailboxer_notification_id) REFERENCES mailboxer_notifications(id)

archonic commented Dec 7, 2012

The methods on ActiveRecord::ConnectionAdapters::SchemaStatements include add_foreign_key and drop_foreign_key. Perhaps remove_foreign_key has been depreciated?

http://araddconstraint.rubyforge.org/classes/ActiveRecord/ConnectionAdapters/SchemaStatements.html

As for my error, it seems as if it thinks :name is :column. I don't see any errors in syntax though... pardon my migrations noobness.

Update: I ended up completing the migration manually and adding the migration to schema_migrations. That won't fly for deploying to a different environment though. These never worked for me:

#Receipts
add_foreign_key "mailboxer_receipts", "mailboxer_notifications", :name => "receipts_on_notification_key"
#Messages  
add_foreign_key "mailboxer_notifications", "mailboxer_conversations", :name => "notifications_on_conversation_key"

Anyone else see an issue with them?

Contributor

mikesnare commented Jan 18, 2013

Is there any progress on this? I would like to be able to subclass one or two of the models to add custom scopes, etc, but can't because of the namespace issue. Thanks!

I solved my issue with gem install foreigner.

+1! I will be implementing this in February and namespacing would really help.

+1 please pull

@atd @Roendal Any progress on pulling this in? Anything anyone can do to help?

Owner

Roendal commented Mar 11, 2013

I am no longer a rails developer @danmelnick, so I can't make any progress on it. And I don't think @atd has the time needed to work with this issue. It's a truly a pity.

This issue needs, at least for some time, a developer with some time to spend on it.

@Roendal So does Mailboxer need some new maintainers? The pull request looks fairly complete unless I'm missing something.

Contributor

daveworth commented Mar 11, 2013

in theory this is really close. I too have been slammed for a while and have never gotten this finished 😊

What this should have to complete it is a set of migrations for 3.x that take an existing Mailboxer installation and renames the columns to the right namespace so that the upgrade path is really simple for existing users. @danmelnick an you implement that? It'd be cool to do rake mailboxer:upgrade and have it create the migration automagically.

Owner

Roendal commented Mar 11, 2013

social_stream team and me can handle most of the issues and small updates needed, but It's true that for bigger features and updates some help is needed.

Contributor

apneadiving commented May 23, 2013

great pull request, thanks all

@apneadiving apneadiving referenced this pull request May 27, 2013

Closed

Rails 4 support #154

Contributor

apneadiving commented Jun 4, 2013

I can help with the merge, just would like to know how I could help best.
What is expected from the reviewer? I guess specs are already green?

Contributor

daveworth commented Jun 4, 2013

@apneadiving I don't want to speak for the Mailboxer dev team (most of whom are inactive) but since I did the bulk of the work and haven't circle'd back.. the problem is that I was never able to find a nice way to migrate existing Mailboxer users to the new namespaced schema in a smooth migration. It comes down to my not understanding how to generate migrations to do so. What we would like is a rails generator like rails g mailboxer:upgrade_schema which takes the old schema and renames the tables to the new schema. Do you think you can make that happen in a way that is Rails 3.0, 3.1, and 3.2 + 4.x compatible? I ask because, as I understand it is different for 3.0 and 3.1 from 3.2 and 4.x (I could be wrong... this was a sticking point for me).

Contributor

apneadiving commented Jun 4, 2013

I'd focus on Rails 3 first. If I recall well, two main things changed in migration:

  • class methods replaced with instance methods (needs fix)
  • change usually replaces up and down but the laters are still working

If an ETL script is expected to keep backward compatibility, it would take longer. If this was my gem, I'd simply release a new major version in rc to have at least the new code working.

Backward compatibility is important, for sure, but it should not be another reason to slow the pace. BTW, major bumps are here to warn about breaking changes.

Contributor

daveworth commented Jun 4, 2013

@apneadiving I agree about the major version bump (done in this PR) but making the upgrade path smooth is a huge win in my boat. The goal would be to simply make it easy for existing users to keep their existing data intact and move to the new schema. Is that reasonable?

My understanding of the differences in generating migrations was something to do with Railties but I could be wrong (it's been a while since I've paid attention).

Contributor

apneadiving commented Jun 4, 2013

Of course we should do that but I'd say: let's proceed with baby steps.
Once a pre or rc is ok, it can be improved and the generator created.

Owner

atd commented Jun 4, 2013

Hi guys, I think there is not such a big problem. The migration just can be dropped inside db/migrate with the rest of the migrations. Rails engines already provide the rake mailboxer_engine:install:migrations to copy them to the appilcation

IMO, a rc version must include the smooth upgrade path or it is not a true release candidate

Contributor

apneadiving commented Jun 5, 2013

Yep a pre would be better, or even a branch on mailboxer.

Actually I need the code for myself fast, that's why I suggested to handle this the shortest way possible.

I fear I'll have to go my own way if we can't find a common ground: I cannot afford delay :(

Owner

atd commented Jun 5, 2013

@apneadiving we will look after the interest of all Mailboxer users. A smooth migration is a must for new versions of mailboxer. It is a pity if you cannot afford giving back and contributing to the Mailboxer community, but you can always make your own fork and place it in your Gemfile

Contributor

jcoyne commented Aug 12, 2013

Can this be rebased so that it's ready to merge?

I almost rolled up my sleeves to work on this. :) glad it is already done. I will try to find the time to make the upgrade process easier, if I can find some spare time this week.

Contributor

jcoyne commented Sep 24, 2013

@chrisftw I think @daveworth may have lost interest in this. If you want to open a new PR, with this branch rebased against master, I'd be happy to review and merge it for you.

@apneadiving apneadiving referenced this pull request Oct 14, 2013

Merged

Namespaced mailboxer #200

Contributor

apneadiving commented Oct 15, 2013

Shall we close this one since #200 has been merged?

Contributor

jcoyne commented Oct 15, 2013

Closed by #200

@jcoyne jcoyne closed this Oct 15, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment