-
Notifications
You must be signed in to change notification settings - Fork 683
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
Renaming some Controller Concerns #1565
Conversation
…shopify_app into klenotiw/add-log-levels
lib/shopify_app/utils.rb
Outdated
end | ||
|
||
def self.deprecated(message) | ||
send_to_logger(:warn, "DEPRECATED - #{message}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should be using the ActiveSupport Deprecation API for this:
https://andycroll.com/ruby/use-a-deprecation-message/
Co-authored-by: Andy Waite <13400+andyw8@users.noreply.github.com>
Co-authored-by: Andy Waite <13400+andyw8@users.noreply.github.com>
…shopify_app into klenotiw/add-log-levels
Co-authored-by: Andy Waite <13400+andyw8@users.noreply.github.com>
Co-authored-by: Andy Waite <13400+andyw8@users.noreply.github.com>
…shopify_app into klenotiw/add-log-levels
… of the functionality
before_action :login_again_if_different_user_or_shop | ||
around_action :activate_shopify_session | ||
after_action :add_top_level_redirection_headers | ||
ShopifyApp::Logger.deprecated("Authenticated has been renamed to EnsureHasSession") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ShopifyApp::Logger.deprecated("Authenticated has been renamed to EnsureHasSession") | |
ShopifyApp::Logger.deprecated("Authenticated has been renamed to EnsureHasSession") |
I would say 'replaced by' rather than renamed to
here, since if someone is upgrading from an older version then there may also be behavior changes.
this pr will take over #1581 |
What this PR does
We had two controllers concerns that we wanted to rename, but still have our old names be backwards compatible for the time being.
RequireKnownShop => EnsureInstalled
Authenticated => EnsureHasSession
I merged in the commit from the logging PR because I wanted to add some deprecation logs. So sorry for the messed up commit history.
closes #1502
Reviewer's guide to testing
Things to focus on
Checklist
Before submitting the PR, please consider if any of the following are needed:
CHANGELOG.md
if the changes would impact usersREADME.md
, if appropriate./docs
, if necessary