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

Fix rails 7 deprecation on ActiveRecord::Base.default_timezone #124

Conversation

jrafanie
Copy link
Member

@jrafanie jrafanie commented Mar 6, 2024

Rails 7 added ActiveRecord.default_timezone and 7.1 will remove it from ActiveRecord::Base.

Extract a memoized method to hide this logic.

Fixes this deprecation in the 7.0 test suite:

DEPRECATION WARNING: ActiveRecord::Base.default_timezone is deprecated and will be removed in Rails 7.1.
Use `ActiveRecord.default_timezone` instead.
 (called from time_now at /home/runner/work/inventory_refresh/inventory_refresh/lib/inventory_refresh/save_collection/saver/base.rb:275)

Comment on lines 275 to 279
if default_timezone_utc?
Time.now.utc
else
Time.zone.now
end
Copy link
Member

Choose a reason for hiding this comment

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

Hm if the default_timezone is UTC then aren't Time.now.utc and Time.zone.now the same thing? Could we simplify this to just Time.zone.now

Copy link
Member Author

Choose a reason for hiding this comment

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

Oooh, I hear you saying, can we delete this?

irb(main):009:0> Time.zone.now.class
=> ActiveSupport::TimeWithZone
irb(main):010:0> Time.now.utc.class
=> Time

Let me see if there is a reason it was written like this.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah if we're concerned about Time vs ActiveSupport::TimeWithZone we could always Time.zone.now.to_time

Copy link
Member Author

Choose a reason for hiding this comment

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

It was localtime originally... maybe it's not needed at all.

❤️

Copy link
Member

Choose a reason for hiding this comment

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

✂️ ✂️

We use ActiveSupport's time extension.  If the zone has been set, such as utc, we'll use
that, otherwise, it will use Time.now, which will be local.
@jrafanie jrafanie force-pushed the avoid_rails7_deprecation_ar_base_default_timezone branch from 400138d to 8e374a3 Compare March 6, 2024 22:43
@miq-bot
Copy link
Member

miq-bot commented Mar 6, 2024

Checked commit jrafanie@8e374a3 with ruby 2.7.8, rubocop 1.56.3, haml-lint 0.51.0, and yamllint
1 file checked, 0 offenses detected
Everything looks fine. 🍰

else
Time.zone.now
end
Time.current
Copy link
Member Author

@jrafanie jrafanie Mar 6, 2024

Choose a reason for hiding this comment

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

@agrare this handles if Time.zone is not set (nil) and returns Time.now which should be localtime if not set. If set to utc, it will return utc time via Time.zone.now.

See: https://github.com/rails/rails/blob/v6.1.7.7/activesupport/lib/active_support/core_ext/time/calculations.rb#L39-L41

@agrare agrare merged commit 46e5701 into ManageIQ:master Mar 7, 2024
9 checks passed
@jrafanie jrafanie deleted the avoid_rails7_deprecation_ar_base_default_timezone branch August 6, 2024 19:27
agrare added a commit that referenced this pull request Aug 6, 2024
Fixed
- Fix rails 7 deprecation on ActiveRecord::Base.default_timezone (#124)
- Fix missing constant OpenStruct in tests (#131)

Added
- Test with ruby 3.2 and rails 7.1 (#122)
- Log destroy result when duplicate record is found (#120)
- Use ruby 3.1 and rails 7 for code coverage (#136)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants