Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update receipt.rb to avoid scope conflicts in #340 #341

Closed
wants to merge 1 commit into from

Conversation

souqueta
Copy link

No description provided.

@jcoyne
Copy link
Contributor

jcoyne commented Mar 12, 2015

Sorry, I don't think this change is acceptable. It's only for the case where you've also installed permanent_records gem. This is not a common use case and we can't reasonably support it. I imagine there are other gems out there that might have conflicts Secondly it violates the principal of least surprise. One could reasonably expect that Mailboxer::Receipt.deleted is code that ships with mailboxer.

Here's how you can fix it:

Refactor the Mailboxer::Receipt class so that it only has include statements. Extract all the logic currently in that class to modules. One of the modules will just have the scope statements in it. Submit this as a patch to Mailboxer. e.g.

class Mailboxer::Receipt < ActiveRecord::Base
  include Mailboxer::Relationships
  include Mailboxer::Scopes
  include Mailboxer::Actions
end

In your local app, define the Mailboxer::Receipt class yourself, then it will not be loaded from the gem. You will include all the modules except the one module that defines the scopes. You can define the scopes as needed (and use permanent_records). e.g.

class Mailboxer::Receipt < ActiveRecord::Base
  include Mailboxer::Relationships
  scope :not_deleted, -> { ... }
  # scope :deleted, -> { ... }  This line is commented out because it clashes with permanent_records
  ...
  ...
  include Mailboxer::Actions
end

@jcoyne jcoyne closed this Mar 12, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants