LevelConceptDifficulty keeps attributes on create #24034
Conversation
Fixes a subtle bug we didn't notice for a long time: Given a Level that has no LevelConceptDifficulty When a LevelConceptDifficulty is created by passing a `level_concept_difficulty` hash to Level's `assign_attributes` method (via ActiveRecord's [`assign_nested_attributes_for_one_to_one_association`](https://apidock.com/rails/ActiveRecord/NestedAttributes/assign_nested_attributes_for_one_to_one_association) method) Then I expect the created LevelConceptDifficulty to have the properties provided in the hash I passed to Level But actually all of the concept properties are nil after creation! The root cause is that `assign_nested_attributes_for_one_to_one_association` calls `assign_attributes` twice when it causes a new LevelConceptDifficulty to be created - once when it initially builds the model instance, and a second time to set the id of the associated Level. This second call would trigger our custom logic in LevelConceptDifficulty's `assign_attributes` method that unsets any concepts not included in the provided attributes... but since the provided attributes in this second call are only `{level_id: 123}` _all_ concepts would be unset, and we'd end up missing properties. As a solution, I'm skipping the custom behavior when creating a new LevelConceptDifficulty record, because in that case we shouldn't have to worry about unsetting properties.
| 'level_concept_difficulty' => {'sequencing' => 3} | ||
| ) | ||
| refute_nil level.level_concept_difficulty | ||
| assert_equal 3, level.level_concept_difficulty.sequencing |
There was a problem hiding this comment.
Out of these two tests, this is the only assertion that was failing prior to my fix.
| level = LevelLoader.load_custom_level(LevelLoader.level_file_path('K-1 Bee 2')) | ||
| refute_nil Level.find_by_name('K-1 Bee 2') | ||
|
|
||
| assert_equal 3, level.level_concept_difficulty.sequencing |
There was a problem hiding this comment.
Wrote this test to see if the seeding process somehow sidestepped this issue; it doesn't. This assertion also failed before my fix.
|
@Hamms anyone else you'd like on this review? And anyone on the product team we should notify if we can confirm that any levels on prod have incorrect concept difficulty props? (maybe Ben?) |
|
Definitely Ben, probably also Kiki and Ryan |
| @@ -212,6 +212,15 @@ def raise_unless_specifies_ideal_level_source(level_class) | |||
| level.properties['initial_dirt'] | |||
| end | |||
|
|
|||
| test 'creating custom level from file sets level_concept_difficulty' do | |||
| Level.find_by_name('K-1 Bee 2')&.destroy | |||
There was a problem hiding this comment.
Note, we already have a dependency on K-1 Bee 2.level in this test file (see the test above this one) so I stuck with that for now; I'm setting up proper level file fixtures in my seeding tests on the other PR though.
|
Update: I wrote a oneoff script to check production for mismatched LCDs. There are exactly three right now: I'll add the oneoff script to this PR. |
|
I've emailed the stakeholders you mentioned. After this fix ships, we can ensure the three affected levels get fixed with a small manual change to their audit logs, which will change their checksums and cause them to be updated next time production seeds levels. Thanks @aoby for your help with the oneoff script and spotting me while I ran it on prod! |
While trying to full validate the changes in #23805 I ran across an existing, subtle bug:
level_concept_difficultyhash to Level'sassign_attributesmethod(via ActiveRecord's
assign_nested_attributes_for_one_to_one_associationmethod)The root cause is that
assign_nested_attributes_for_one_to_one_associationcallsassign_attributestwice when it causes a new LevelConceptDifficulty to be created - once when it initially builds the model instance, and a second time to set the id of the associated Level. This second call would trigger our custom logic in LevelConceptDifficulty'sassign_attributesmethod that unsets any concepts not included in the provided attributes... but since the provided attributes in this second call are only{level_id: 123}all concepts would be unset, and we'd end up missing properties.As a solution, I'm skipping the custom behavior when creating a new LevelConceptDifficulty record, because in that case we shouldn't have to worry about unsetting properties.
Impact of bug
The implications of this bug are probably small, but nonzero: On production, any levels with concept difficulty properties that have been created but never updated in a subsequent DTP are probably missing concept difficulty properties.
As of August 1st, 2018 only three levels on production are currently affected (don't match the LevelConceptDifficulty defined in their .level file):
2-3 Artist 9 REPLACEMENTKTest2Overworld Free Play 20x20