Uh oh!
There was an error while loading. Please reload this page.
Misc fixes to serializers' handling of attempted updates to missing content - #2682
Conversation
5382838 to
d77d490Compare3cd8470 to
802c3a6Compare| # For the combined index, use any related objects' primary key | ||
| combined_index = (idx.pk if hasattr(idx, 'pk') else idx for idx in id_values) | ||
| return tuple(combined_index) |
There was a problem hiding this comment.
I decided to switch to using a tuple in order to make doing stuff like this simpler and less brittle.
Codecov Report
@@ Coverage Diff @@## develop #2682 +/- ##
===========================================
+ Coverage 85.14% 85.16% +0.02%
===========================================
Files 299 299 Lines 15079 15086 +7 ===========================================
+ Hits 12839 12848 +9 + Misses 2240 2238 -2
Continue to review full report at Codecov.
|
| if len(all_validated_data_by_id) != len(updated_keys): | ||
| self.missing_keys = updated_keys.difference( | ||
| set(all_validated_data_by_id.keys()) |
There was a problem hiding this comment.
this was backwards, which meant that missing_keys was always ending up empty.
rtibbles
left a comment
There was a problem hiding this comment.
Tests pass, and new test passes. Code looks good.
| id_values = (self.get_value(data, attr) for attr in id_attr) | ||
| # For the combined index, use any related objects' primary key | ||
| combined_index = (idx.pk if hasattr(idx, 'pk') else idx for idx in id_values) |
There was a problem hiding this comment.
Hrm, I'm surprised this wasn't breaking previously - was it stringifying the content node before?
There was a problem hiding this comment.
@rtibbles:
Looks like the object being returned there, a rest_framework.relations.PKOnlyObject, returns its pk from its __str__ method:
ipdb> [(i, str(i)) for i in tuple(id_values)] [(<rest_framework.relations.PKOnlyObject object at 0x7f0a176501d0>, '4ce295a9caf94b28b18376e0452d2507'),
('5361ba5142ba4695b27256e5b74adfdd', '5361ba5142ba4695b27256e5b74adfdd')]
So I suppose this could be simplified a bit by just doing:
return tuple(str(self.get_value(data, attr)) for attr in id_attr)
The way it is now has the benefit of being explicit, though.
Description
If the front-end tries to update an assessment item that's missing from the back-end, this PR makes sure that a useful error is returned, instead of a confusing one.
This us a followup on #2655.
Issue Addressed
This will make #2679 and #2603 less severe by adding proper error handling for attempts to update missing items.
Steps to Test
Implementation Notes
At a high level, how did you implement this?
remove_id_valuesfrom mutating changes unexpectedlyid_value_lookupmethod for consistent keying of assessment itemsid_value_lookupto use a tuple instead of a string, for simpler conversion from front-endchange["key"]array representation.missing_keysto generate errors for missing items.