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

don't clone deprecated blockly levels #44255

Merged
merged 3 commits into from
Jan 7, 2022
Merged

Conversation

tim-dot-org
Copy link
Contributor

@tim-dot-org tim-dot-org commented Jan 7, 2022

Deprecated blockly levels cause problems when cloned, and should be skipped (here, we return the old blockly level)
Specifically, they cause a validation failure on uniqueness of the level_num field:

# Together, these validations prevent collisions between level keys, including
# level keys which differ only by case, between all 3 categories of levels:
# custom levels, DSLDefined levels, and deprecated blockly levels. For more
# context on these categories and level keys, see:
# https://docs.google.com/document/d/1rS1ekCEVU1Q49ckh2S9lfq0tQo-m-G5KJLiEalAzPts/edit
validates_uniqueness_of :name, case_sensitive: false, conditions: -> {where(level_num: ['custom', nil])}
validates_uniqueness_of :level_num, scope: :game, conditions: -> {where.not(level_num: ['custom', nil])}

See: https://docs.google.com/document/d/1rS1ekCEVU1Q49ckh2S9lfq0tQo-m-G5KJLiEalAzPts/edit#heading=h.2buyj1ahym0m

Testing story

Couldn't repro locally, but tested with these changes in levelbuilder for pre-express and looks good!

@tim-dot-org tim-dot-org requested review from davidsbailey and a team January 7, 2022 17:50
@@ -656,6 +656,9 @@ def clone_with_suffix(new_suffix, editor_experiment: nil)

return Level.find_by_name(new_name) if Level.find_by_name(new_name)

# explicitly don't clone blockly levels (will cause a validation failure on non-unique level_num)
return self if key.start_with?('blockly:')
Copy link
Member

Choose a reason for hiding this comment

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

nit: it would be slightly safer to put this before we check for new_name... in the extremely unlikely chance that we end up with a level named blockly_2022 😛 but also in the murkier case which has come up on levelbuilder where some of the blockly levels do actually have names other than "blockly".

the fix looks right to me. it would be nice to still find a way to verify it, but if that proves difficult then a speculative fix could also be ok. in that case it could be enough to convince yourself that you haven't made anything worse locally, even if it doesn't prove things will be fixed on levelbuilder.

Copy link
Contributor

@bethanyaconnor bethanyaconnor left a comment

Choose a reason for hiding this comment

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

Looks good to me after addressing Dave's comment. Thanks for working through this!

@tim-dot-org
Copy link
Contributor Author

tested using these changes on levelbuilder with pre-express and looks good!

@tim-dot-org tim-dot-org merged commit 1582c5d into staging Jan 7, 2022
@tim-dot-org tim-dot-org deleted the level-clone-skip-blockly branch January 7, 2022 21:47
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