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

Refactor backend mail method #2643

Closed
wants to merge 7 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@huoxito
Member

huoxito commented Mar 2, 2013

A suggestion to remove the mail_method obj (and table) from Spree and just use Spree::Config instead. I could never understand why Spree would keep a collection of mail methods when all it needs, and actually uses, is a single one. This method should prove it, it's called by both Spree::Core::MailSetting and Spree::Core::MailInterceptor to override the action_mailer configs.

def self.current
  where(:environment => Rails.env).first
end

Another reason to remove the MailMethod model is that the table only has an environment and an active attr, besides the Rails defaults (I believe the active attribute is not used anywhere). All other fields are preferences already.

If this get merged in users could use if statements to define custom settings as per environment. e.g.

  if Rails.env.production?
    config.mail_port = "25"
  else
    config.mail_port = "1025"
  end

Please let me know what you think. The build still has some failing specs but I think that's related to taxons or something else. I made it in four commits just to state clearly what were the changes. I could squash them into one in case you guys intend to merge it in master.

@huoxito

This comment has been minimized.

Show comment
Hide comment
@huoxito

huoxito Mar 2, 2013

Member

Missed MailInterceptor and MailSetting specs, I pushed one more to fix them and move them to /core/specs. I believe that's where they belong.

Member

huoxito commented Mar 2, 2013

Missed MailInterceptor and MailSetting specs, I pushed one more to fix them and move them to /core/specs. I believe that's where they belong.

@radar

This comment has been minimized.

Show comment
Hide comment
@radar

radar Mar 19, 2013

Member

It's done this way so that admins can modify it through the backend. I don't know how many admins would be modifying this information though. I've always thought it would be better suited in an initializer.

Thoughts on this @BDQ or @schof?

Member

radar commented Mar 19, 2013

It's done this way so that admins can modify it through the backend. I don't know how many admins would be modifying this information though. I've always thought it would be better suited in an initializer.

Thoughts on this @BDQ or @schof?

@huoxito

This comment has been minimized.

Show comment
Hide comment
@huoxito

huoxito Mar 19, 2013

Member

It would still allow admin to edit settings through admin UI. But this PR makes that happen using Spree::Config instead of saving records on a spree_mail_methods table. And since it's done through Spree::Config users can also configure this on the initializer. I can't see a reason to have a list of mail method when Spree only uses MailMethod#current everywhere.

I'm aware that if this gets merged in the spree_mail_method table still needs to be dropped eventually and we'll need to update the docs. I'm willing to help with that too just in case.

Member

huoxito commented Mar 19, 2013

It would still allow admin to edit settings through admin UI. But this PR makes that happen using Spree::Config instead of saving records on a spree_mail_methods table. And since it's done through Spree::Config users can also configure this on the initializer. I can't see a reason to have a list of mail method when Spree only uses MailMethod#current everywhere.

I'm aware that if this gets merged in the spree_mail_method table still needs to be dropped eventually and we'll need to update the docs. I'm willing to help with that too just in case.

@radar

View changes

Show outdated Hide outdated core/app/models/spree/mail_method.rb
@radar

This comment has been minimized.

Show comment
Hide comment
@radar

radar Mar 28, 2013

Member

Alright, I've looked over this again. It looks like a step in the right direction. Could you please move the MailMethod class as suggested. That's the last thing to do before this PR will be merged.

Member

radar commented Mar 28, 2013

Alright, I've looked over this again. It looks like a step in the right direction. Could you please move the MailMethod class as suggested. That's the last thing to do before this PR will be merged.

@huoxito

This comment has been minimized.

Show comment
Hide comment
@huoxito

huoxito Mar 28, 2013

Member

@radar done. rebased and remove all references to MailMethod. Please take a look. Running backend spec now, which take ages on my local machine. Hopefully should be good to go.

Member

huoxito commented Mar 28, 2013

@radar done. rebased and remove all references to MailMethod. Please take a look. Running backend spec now, which take ages on my local machine. Hopefully should be good to go.

@huoxito

This comment has been minimized.

Show comment
Hide comment
@huoxito

huoxito Mar 28, 2013

Member

Fixed a backend controller spec related to this change. Got another error I think it's unrelated:

  1. Promotion Adjustments coupon promotions should allow an admin to create a promotion that adds a 'free' item to the cart
    Failure/Error: select2 "Create adjustment", :from => "Add action of type"
    RuntimeError:
    Could not find label by text Add action of type

    /home/washington/projetos/spree/core/lib/spree/testing_support/capybara_ext.rb:73:in `find_label_by_text'

    /home/washington/projetos/spree/core/lib/spree/testing_support/capybara_ext.rb:42:in`select2'

    ./spec/requests/admin/promotion_adjustments_spec.rb:221:in `block (3 levels) in <top (required)>'

Member

huoxito commented Mar 28, 2013

Fixed a backend controller spec related to this change. Got another error I think it's unrelated:

  1. Promotion Adjustments coupon promotions should allow an admin to create a promotion that adds a 'free' item to the cart
    Failure/Error: select2 "Create adjustment", :from => "Add action of type"
    RuntimeError:
    Could not find label by text Add action of type

    /home/washington/projetos/spree/core/lib/spree/testing_support/capybara_ext.rb:73:in `find_label_by_text'

    /home/washington/projetos/spree/core/lib/spree/testing_support/capybara_ext.rb:42:in`select2'

    ./spec/requests/admin/promotion_adjustments_spec.rb:221:in `block (3 levels) in <top (required)>'

@huoxito

This comment has been minimized.

Show comment
Hide comment
@huoxito

huoxito Mar 28, 2013

Member

Also ran frontend specs. Got same similar errors as above. I believe this one is good to go.

Member

huoxito commented Mar 28, 2013

Also ran frontend specs. Got same similar errors as above. I believe this one is good to go.

@radar

This comment has been minimized.

Show comment
Hide comment
@radar

radar Apr 1, 2013

Member

Tentatively adding this to master now. Will run it over the CI server and see what it says.

Member

radar commented Apr 1, 2013

Tentatively adding this to master now. Will run it over the CI server and see what it says.

@radar

This comment has been minimized.

Show comment
Hide comment
@radar

radar Apr 9, 2013

Member

CI server says all is green. Thanks @huoxito. Goodbye, MailMethod.

Member

radar commented Apr 9, 2013

CI server says all is green. Thanks @huoxito. Goodbye, MailMethod.

@radar radar closed this Apr 9, 2013

@huoxito

This comment has been minimized.

Show comment
Hide comment
@huoxito

huoxito Apr 10, 2013

Member

Awesome @radar ! However I've notice it not made it to spree/spree master yet. Tested a merge locally and relized it does not apply cleanly anymore. Would like me to rebase it again on top of master?

Member

huoxito commented Apr 10, 2013

Awesome @radar ! However I've notice it not made it to spree/spree master yet. Tested a merge locally and relized it does not apply cleanly anymore. Would like me to rebase it again on top of master?

@radar

This comment has been minimized.

Show comment
Hide comment
@radar

radar Apr 12, 2013

Member

Hm... I don't know where it went!

Yes please rebase on master.

Member

radar commented Apr 12, 2013

Hm... I don't know where it went!

Yes please rebase on master.

@radar radar reopened this Apr 12, 2013

@huoxito

This comment has been minimized.

Show comment
Hide comment
@huoxito

huoxito Apr 12, 2013

Member

Will do!

Member

huoxito commented Apr 12, 2013

Will do!

huoxito added some commits Mar 1, 2013

Changes mail method form to use Spree::Config
  - Deletes new and index templates from mail methods
  - Removes mail_method model spec. It adds value to the test suite
  - Fixes requests and controllers specs related to mail method
    refactoring
Fixes mail settings and interceptor specs
Moves them to /core/spec/lib since both MailInterceptor and MailSettings
are classes from the /core package
Remove all remaining references to Spree::MailMethod
And the class itself. I believe the spree_mail_methods table should stay
for now so that can core team can talk about a strategy to advise users
the best way to upgrade and docs are updated
@huoxito

This comment has been minimized.

Show comment
Hide comment
@huoxito

huoxito Apr 12, 2013

Member

It's rebased @radar Please merge :)

Looks like not much changed since the last rebase except for this 6eb8a0c Got 2 failing specs on backend which seem unrelated to this changes.

Member

huoxito commented Apr 12, 2013

It's rebased @radar Please merge :)

Looks like not much changed since the last rebase except for this 6eb8a0c Got 2 failing specs on backend which seem unrelated to this changes.

@radar

This comment has been minimized.

Show comment
Hide comment
@radar

radar Apr 17, 2013

Member

Added to my master branch and running over CI. Thanks again @huoxito.

On Fri, Apr 12, 2013 at 11:54 PM, Washington Júnior <
notifications@github.com> wrote:

It's rebased @radar https://github.com/radar Please merge :)

Looks like not much changed since the last rebase except for this 6eb8a0chttps://github.com/spree/spree/commit/6eb8a0c10d6a4bc9abfbc67f0074220914123f63Got 2 failing specs on backend which seem unrelated to this changes.


Reply to this email directly or view it on GitHubhttps://github.com/spree/spree/pull/2643#issuecomment-16294017
.

Ryan Bigg
Community Manager
Spree Commerce, Inc.

Register now for SpreeConf
May 20-21 in Washington, D.C.
http://spreeconf.com

Member

radar commented Apr 17, 2013

Added to my master branch and running over CI. Thanks again @huoxito.

On Fri, Apr 12, 2013 at 11:54 PM, Washington Júnior <
notifications@github.com> wrote:

It's rebased @radar https://github.com/radar Please merge :)

Looks like not much changed since the last rebase except for this 6eb8a0chttps://github.com/spree/spree/commit/6eb8a0c10d6a4bc9abfbc67f0074220914123f63Got 2 failing specs on backend which seem unrelated to this changes.


Reply to this email directly or view it on GitHubhttps://github.com/spree/spree/pull/2643#issuecomment-16294017
.

Ryan Bigg
Community Manager
Spree Commerce, Inc.

Register now for SpreeConf
May 20-21 in Washington, D.C.
http://spreeconf.com

@radar radar closed this in 2037ac9 Apr 22, 2013

@huoxito huoxito deleted the huoxito:refactor-backend-mail-method branch May 22, 2013

enricostano added a commit to Em-AK/openfoodnetwork that referenced this pull request Apr 19, 2017

enricostano added a commit to Em-AK/openfoodnetwork that referenced this pull request May 8, 2017

enricostano added a commit to Em-AK/openfoodnetwork that referenced this pull request May 18, 2017

enricostano added a commit to Em-AK/openfoodnetwork that referenced this pull request May 24, 2017

enricostano added a commit to Em-AK/openfoodnetwork that referenced this pull request Jul 3, 2017

enricostano added a commit to Em-AK/openfoodnetwork that referenced this pull request Jul 17, 2017

enricostano added a commit to Em-AK/openfoodnetwork that referenced this pull request Aug 2, 2017

sauloperez added a commit to coopdevs/openfoodnetwork that referenced this pull request Aug 9, 2017

sauloperez added a commit to coopdevs/openfoodnetwork that referenced this pull request Aug 9, 2017

mkllnk added a commit to Em-AK/openfoodnetwork that referenced this pull request Aug 30, 2017

oeoeaio added a commit to Em-AK/openfoodnetwork that referenced this pull request Sep 8, 2017

oeoeaio added a commit to oeoeaio/openfoodnetwork that referenced this pull request Sep 8, 2017

oeoeaio added a commit to oeoeaio/openfoodnetwork that referenced this pull request Sep 8, 2017

oeoeaio added a commit to oeoeaio/openfoodnetwork that referenced this pull request Sep 8, 2017

oeoeaio added a commit to oeoeaio/openfoodnetwork that referenced this pull request Sep 8, 2017

sauloperez added a commit to coopdevs/openfoodnetwork that referenced this pull request Mar 7, 2018

sauloperez added a commit to coopdevs/openfoodnetwork that referenced this pull request Mar 7, 2018

sauloperez added a commit to coopdevs/openfoodnetwork that referenced this pull request Mar 7, 2018

HugsDaniel added a commit to HugsDaniel/openfoodnetwork that referenced this pull request Jun 4, 2018

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