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

Set a default value for Stage lockable and make it non-nullable #14927

Merged
merged 2 commits into from May 9, 2017

Conversation

joshlory
Copy link
Contributor

@joshlory joshlory commented May 9, 2017

I ran into this in 03c1d43. See: https://robots.thoughtbot.com/avoid-the-threestate-boolean-problem.

irb(main):015:0> Script.first.stages.count
  Script Load (0.7ms)  SELECT  `scripts`.* FROM `scripts` ORDER BY `scripts`.`id` ASC LIMIT 1
   (0.5ms)  SELECT COUNT(*) FROM `stages` WHERE `stages`.`script_id` = 1
=> 20
irb(main):013:0> Script.first.stages.where(lockable: true)
  Script Load (0.9ms)  SELECT  `scripts`.* FROM `scripts` ORDER BY `scripts`.`id` ASC LIMIT 1
  Stage Load (0.6ms)  SELECT `stages`.* FROM `stages` WHERE `stages`.`script_id` = 1 AND `stages`.`lockable` = 1 ORDER BY absolute_position ASC
=> #<ActiveRecord::AssociationRelation []>
irb(main):013:0> Script.first.stages.where(lockable: false)
  Script Load (0.9ms)  SELECT  `scripts`.* FROM `scripts` ORDER BY `scripts`.`id` ASC LIMIT 1
  Stage Load (0.6ms)  SELECT `stages`.* FROM `stages` WHERE `stages`.`script_id` = 1 AND `stages`.`lockable` = 0 ORDER BY absolute_position ASC
=> #<ActiveRecord::AssociationRelation []>

First make the column non-nullable (setting a default value for NULL entries), then set the column default in a reversible way. Verified this migration can be rolled back:

$ rake db:migrate
== 20170508234535 ChangeLockableOnStage: migrating ============================
-- change_column_null(:stages, :lockable, false, false)
   -> 0.0583s
-- change_column_default(:stages, :lockable, {:from=>nil, :to=>false})
   -> 0.0180s
== 20170508234535 ChangeLockableOnStage: migrated (0.0765s) ===================

Annotated (1): app/models/stage.rb

$ rake db:rollback STEP=1
Ignoring db/schema_cache.dump because it has expired. The current schema version is 20170508234535, but the one in the cache is 20170505231542.
== 20170508234535 ChangeLockableOnStage: reverting ============================
-- change_column_default(:stages, :lockable, {:from=>false, :to=>nil})
   -> 0.0371s
-- change_column_null(:stages, :lockable, true, false)
   -> 0.0343s
== 20170508234535 ChangeLockableOnStage: reverted (0.0772s) ===================

Annotated (1): app/models/stage.rb

@joshlory joshlory merged commit 7ccc2fb into staging May 9, 2017
@joshlory joshlory deleted the lockable-not-null branch May 9, 2017 02:48
class ChangeLockableOnStage < ActiveRecord::Migration[5.0]
def change
change_column_null :stages, :lockable, false, false
change_column_default :stages, :lockable, from: nil, to: false
Copy link
Contributor

Choose a reason for hiding this comment

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

Q: Should the order of these two be reversed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It shouldn't matter, since the 4th param of change_column_null specifies the default value for NULL entries (false). I believe even if I swap the order, the 4th param is still needed.

@@ -518,7 +518,7 @@ def self.add_script(options, raw_script_levels)
named_level = raw_level.delete(:named_level)
bonus = raw_level.delete(:bonus)
stage_flex_category = raw_level.delete(:stage_flex_category)
stage_lockable = raw_level.delete(:stage_lockable)
stage_lockable = !!raw_level.delete(:stage_lockable)
Copy link
Contributor

Choose a reason for hiding this comment

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

Given the !!, can the comparison to true on line 595 be simplified (removed)?

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.

None yet

3 participants