fix: Serialize list problem_check results as JSON #366
Conversation
|
This is in draft while I test the changes in Aspects. |
bmtcril
force-pushed
the
bmtcril/list_problem_check_fix
branch
from
ed4a1b9 to
f0b7034
Compare
Prior to this fix, TinCan would use the repr of the list, which made some responses effectively un-parseable downstream.
bmtcril
force-pushed
the
bmtcril/list_problem_check_fix
branch
from
f0b7034 to
b32afcd
Compare
There was a problem hiding this comment.
Couple of nits, but this LGTM @bmtcril :) 👍
- I tested this by running this branch on my Tutor devstack while sending problem_check events through the LMS, ensuring that they're still read as expected on Aspects.
- I read through the code
-
I checked for accessibility issuesN/A - Includes documentation
event_routing_backends/processors/xapi/event_transformers/problem_interaction_events.py
Outdated
Show resolved
Hide resolved
| @@ -119,6 +119,7 @@ def test_event_transformer(self, event_filename, mocked_uuid4): | |||
| try: | |||
| self.compare_events(actual_transformed_event, expected_event) | |||
| except Exception as e: # pragma: no cover | |||
| print("Comparison failed, writing output to test_output for debugging") | |||
There was a problem hiding this comment.
nit: debugging statement can be removed
| print("Comparison failed, writing output to test_output for debugging") |
There was a problem hiding this comment.
I added this because every time I'm in that code I am confused by what it's doing, and there is nothing in the rest of the test run that references that this functionality exists. It's handy so I want to make sure people know it's there! So I'd like to keep some version of it, but I'm happy to present it in a different way if there's a better one, though.
There was a problem hiding this comment.
It's fine to leave as-is if it's useful; I just have a knee-jerk reaction to print statements :)
|
I've also tested in Aspects with an updated schema and it looks good. |
Prior to this fix, TinCan would use the repr of the list, which made some responses effectively un-parseable downstream.
bmtcril
force-pushed
the
bmtcril/list_problem_check_fix
branch
from
5c58ade to
9847748
Compare
event_routing_backends/processors/xapi/event_transformers/problem_interaction_events.py
Outdated
Show resolved
Hide resolved
Description:
Prior to this fix, TinCan would use the repr of the list, which made some responses effectively un-parseable downstream.
closes #365
Merge checklist:
Post merge:
finished.
Author concerns: This is a bit of a hack around TinCan, and also TinCan hasn't been updated in 3 yrs. We should consider forking it or replacing it. 😬