give last_updated_by a valid default value if it's None when export #74
Conversation
If user call set-maintenance but doesn't do anything and the report was originally empty, then the last_updated_by field would be None, this would lead to crash when set-maintenance is called again, since the last_updated_by field couldn't be None. Now, while exporting the report, make it to default value if field last_updated_by is None.
JayZ12138
requested review from
jbruzl,
rajulkumar and
rohanpm
as code owners
JayZ12138
requested review from
chandwanitulsi,
danrodrig,
negillett and
rbikar
midnightercz
left a comment
midnightercz
left a comment
There was a problem hiding this comment.
It's little strange that you don't have to change test_create_export_report, could you check that?
Since the data given in the test has the last_updated_by value. |
rohanpm
left a comment
rohanpm
left a comment
There was a problem hiding this comment.
I'd also prefer seeing the style-only changes excluded from the commit. If they're introduced because black behavior changed, I think a commit/PR dedicated to just fixing the style is the way to go.
Seems like it's because I'm using an older version of black. |
rohanpm
left a comment
rohanpm
left a comment
There was a problem hiding this comment.
I just realized that no autotests had to be updated for these behavior changes. Seems like we are missing some tests we really ought to have, then. Are you willing to look into that as part of this PR?
sure. |
| self, | ||
| entries=new_entries, | ||
| last_updated_by=owner, | ||
| last_updated=datetime.datetime.utcnow(), |
There was a problem hiding this comment.
Could you explain this part a little? The change here was to add/overwrite last_updated on any change. It might be correct, but all of the commit message, PR message and changelog only refer to changes to last_updated_by, a different field.
There was a problem hiding this comment.
Hm, I thought the problem was here for the first time and did this change, but it's not actually.
I'll remove this since it's unrelated change
4 participants
If a user calls set-maintenance but doesn't do anything and the report
was originally empty, then the last_updated_by field would be None,
this would lead to crash when set-maintenance is called again, since
the last_updated_by field couldn't be None.
Now, while exporting the report, make it to default value if the field
last_updated_by is None.