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

Remove hacked relations #17545

Merged

Conversation

bdunne
Copy link
Member

@bdunne bdunne commented Jun 7, 2018

if get_option(:schedule_type) == "schedule"
deliver_on = get_option(:schedule_time).utc rescue nil
start_time = get_option(:schedule_time).utc rescue nil
Copy link
Member

Choose a reason for hiding this comment

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

Copying @gmcculloug 's comment (#17520 (comment))

I would recommend that we refactor this line. The classic UI sets get_option(:schedule_time) to a ActiveSupport::TimeWithZone object which supports the utc call.

I suspect that if this is set from the API or some other means it would set the time as a string in which case the code will silently error and use nil. It is also a good opportunity to get rid of the inline rescue as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 Totally agree, but I would prefer to fix it in a different PR since it's an unrelated issue

@@ -1,4 +1,7 @@
class MiqSchedule < ApplicationRecord
include DeprecationMixin
deprecate_attribute :towhat, :resource_type
Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry @jrafanie but there are still some other models with a tow hat 😆



@bdunne bdunne force-pushed the schedule_changes_move_out_of_reserves branch from 152abcc to ec8deec Compare June 13, 2018 21:21
@bdunne bdunne force-pushed the schedule_changes_move_out_of_reserves branch 4 times, most recently from 7ca7614 to 0652dd1 Compare June 15, 2018 16:53
@miq-bot
Copy link
Member

miq-bot commented Jun 21, 2018

This pull request is not mergeable. Please rebase and repush.

@bdunne bdunne force-pushed the schedule_changes_move_out_of_reserves branch from 0652dd1 to 1ff0da7 Compare July 23, 2018 13:46
@bdunne bdunne changed the title [WIP] Remove hacked relations Remove hacked relations Jul 23, 2018
@miq-bot
Copy link
Member

miq-bot commented Jul 24, 2018

Some comments on commits bdunne/manageiq@8829e85~...1ff0da7

spec/models/miq_schedule_spec.rb

  • ⚠️ - 744 - Detected expect_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.
  • ⚠️ - 752 - Detected expect_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.

@miq-bot
Copy link
Member

miq-bot commented Jul 24, 2018

Checked commits bdunne/manageiq@8829e85~...1ff0da7 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
7 files checked, 2 offenses detected

app/models/miq_schedule.rb

app/models/service_template.rb

@Fryguy Fryguy closed this Aug 1, 2018
@Fryguy Fryguy reopened this Aug 1, 2018
@Fryguy Fryguy merged commit 4a9c9d7 into ManageIQ:master Aug 1, 2018
@Fryguy Fryguy added this to the Sprint 92 Ending Aug 13, 2018 milestone Aug 1, 2018
@bdunne bdunne deleted the schedule_changes_move_out_of_reserves branch August 1, 2018 18:00
NickLaMuro added a commit to NickLaMuro/manageiq-api that referenced this pull request Nov 28, 2018
This warning was added as part of:

ManageIQ/manageiq#17545

There are other uses of `towhat` in the repo but it is only a problem
with `miq_schedule` records being created by these factories that can be
fixed here.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants