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

Rescue DB connection error. #153

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

DanielePalombo
Copy link
Contributor

@DanielePalombo DanielePalombo commented Jun 28, 2023

Summary

If the DB connection fails, the initializer shouldn't raise any error.

Checklist

Check out our PR guidelines for more details.

The following are mandatory for all PRs:

The following are not always needed:

  • 📖 I have updated the README to account for my changes.
  • 📑 I have documented new code with YARD.
  • 🛣️ I have opened a PR to update the guides.
  • ✅ I have added automated tests to cover my changes.
  • 📸 I have attached screenshots to demo visual changes.

@kennyadsl
Copy link
Member

@DanielePalombo why should fail when the DB adapter is not present?

@DanielePalombo
Copy link
Contributor Author

DanielePalombo commented Jun 28, 2023

@kennyadsl My fault.

This PR aims to do exactly the opposite! The initializer should not fail if the DB connection fails.

I'm updating the PR and commit description

@DanielePalombo DanielePalombo force-pushed the dp/rescue-db-connection-error branch from 9ead98c to efc16bb Compare June 28, 2023 14:02
If the DB connection fails the initializer shouldn't raise any error.
@DanielePalombo DanielePalombo force-pushed the dp/rescue-db-connection-error branch from efc16bb to 7638d74 Compare June 28, 2023 14:17
Copy link
Member

@elia elia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think rescuing without letting the user know might hide some issues, I suggested a could have alternatives that might work.

Of course if there's no other option we can add this

@@ -24,6 +24,8 @@ class Engine < Rails::Engine
'bolt_config_credentials',
::SolidusBolt::Engine.bolt_config_credentials_hash
)
rescue ActiveRecord::ConnectionNotEstablished
# do nothing
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DanielePalombo since Solidus v3.3 static model prefs can be added passing a string for the class name.

I think at least for the first configuration it can be done that way:

Spree::Config.static_model_preferences.add(
  'SolidusBolt::PaymentMethod',
  'bolt_credentials',
  ::SolidusBolt::Engine.bolt_credentials_hash
)

The second one is more tricky because it needs the database, thoughts on using ActiveRecord::Base.connected? or even specifically SolidusBolt::BoltConfiguration.connected??

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.

3 participants