Don't log empty m2m changes (fix #2576) #2584
Conversation
| # Helper method that extracts the json literal from an SQL Values clause. | ||
| # Assumes the JSON appears between the last single quotes. | ||
| # Added to solve issue: #2576 | ||
| def extract_jsonb(sql: str): |
There was a problem hiding this comment.
unused? I would hope we don't need to write a broken-in-edge-cases extractor here, there must be a better way
| with assert_no_database_useless_logging(): | ||
| importer_log = import_enrollments(self.default_excel_content, self.semester, None, None, test_run=True) |
There was a problem hiding this comment.
My understanding was that this doesn't do any actual database modifications, but it just "accidentally" added the empty log entries. If this PR fixes the empty log entries, we should be good to use assert_no_database_modifications (and hopefully we don't need assert_no_database_useless_logging at all, then)
|
|
||
|
|
||
| @contextmanager | ||
| def assert_no_database_useless_logging(*args, **kwargs): |
There was a problem hiding this comment.
I think with proper testing setup this shouldn't be necessary. We should:
- have a unit test testing the case from the issue,
course.programs.add(*[]). This should pass underassert_no_database_modifications - modify the tests for the importer to use
assert_no_database_modifications(which we would have liked to do before, but couldn't, due to the logging entries)
| case "pre_add": | ||
| action_type = FieldActionType.M2M_ADD | ||
| case "pre_clear": | ||
| action_type = FieldActionType.M2M_CLEAR |
There was a problem hiding this comment.
Can you explain why the changes here are necessary, and how this is now supposed to work for the reverse-clear case? My understanding is that if we do a reverse-clear (i.e., A.b_instances.clear()), we want to treat it as N forward removes (i.e., as if B.a_instances.remove() was called on each B that was in A.b_instances). This, in my head, should still work perfectly fine with a generic "If adding or removing an empty set, bail out early" logic below -- and it should even require it: If A.b_instances was already empty, we don't want to trigger logging, but right now, your early-out wouldn't catch this case.
2 participants
See #2576