cleanup hrefs from external levels #13382
Conversation
| class RemoveHrefFromExternal < ActiveRecord::Migration[5.0] | ||
| def up | ||
| External.all.each{|level| level.update!(properties: level.properties.except('href'))} | ||
| end |
There was a problem hiding this comment.
Does this make the migration irreversible? If so, worth including a no-op down migration.
There was a problem hiding this comment.
It is irreversible.
Is the noop down migration just to make it explicit that this is the case? What would happen if I were to run migrate:down without a noop down migration? (I would have guessed it would be the same behavior)
There was a problem hiding this comment.
Ah, sorry, I meant we want to be able to run the migration both ways even though we're only modifying data in the up migration.
Some migrations raise ActiveRecord::IrreversibleMigration if you run them backwards — I wasn't sure if that was also true for migrations with only an up and not a down method.
| @@ -0,0 +1,9 @@ | |||
| class RemoveHrefFromExternal < ActiveRecord::Migration[5.0] | |||
| def up | |||
| External.all.each{|level| level.update!(properties: level.properties.except('href'))} | |||
There was a problem hiding this comment.
Since there are only ~ 1600 of these, it's not a big deal, but for larger sets find_each is preferred over .all.each No need to change this, just FYI
Support for hrefs in External levels was removed a while ago (https://github.com/code-dot-org/code-dot-org/pull/3762/files).
However, when creating a new external level, you're still provided with
href 'path/to/html/in/asset/folder'This PR
All of our existing external levels have either nil or 'path/to/html/in/asset/folder' as their href. There is one exception that tries to point to curriculum builder, that I assume was JoshC figuring out if that worked (and isn't referenced in any scripts). I'll also check in with him on that level.