Fix modify and save test, add "updated" to modify #416
Conversation
bio-boris
commented
Aug 6, 2021
•
bio-boris
commented
- Fix modify to update the "updated" timestamp
- Write tests to make sure that the updated timestamp is updated for save and for modify
- Rewrite test_insert_jobs test
bio-boris
changed the title
|
Did I miss something in my review or is this a pre-existing issue? |
|
This is a pre-existing issue that I noticed during #406 (comment) |
| del retrieved_job_mongo["_id"] | ||
| if "updated" not in original_job_json: | ||
| del retrieved_job_json["updated"] | ||
| del retrieved_job_mongo["updated"] |
There was a problem hiding this comment.
Aren't these two ifs going to be always true or always false?
| return super(Job, self).save(*args, **kwargs) | ||
|
|
||
| def modify(self, query=None, **update): | ||
| update["updated"] = datetime.utcnow().timestamp() | ||
| return super(Job, self).modify(query=None, **update) |
There was a problem hiding this comment.
Shouldn't this just pass the query as is, not pass None?
There was a problem hiding this comment.
Assuming I understand this I guess no code uses the query field otherwise tests should've broken
There was a problem hiding this comment.
Yea it should pass the query good call
There was a problem hiding this comment.
how did the tests pass with this bug?
| def test_save_and_modify_updates_timestamp(self): | ||
| job = get_example_job(status=Status.created.value) | ||
| updated_ts = job.updated | ||
| job.save() |
There was a problem hiding this comment.
Its seems a bit odd that saving a job updates the timestamp even if there are no changes, is that what you want?
There was a problem hiding this comment.
Well, updated tracks the last time the job was saved or modified, so seems ok to me. What do you suggest?
There was a problem hiding this comment.
I mean updated to me means that the job was updated, e.g. changed in some way. It doesn't make sense to me for the updated timestamp to change if the job hasn't changed
There was a problem hiding this comment.
Alright, so if we make sure to only save jobs with changes, then we should be alright?
There was a problem hiding this comment.
I'm not sure if its possible to know if a record has a different value or not when saving unless we reload the record or do two saves?
There was a problem hiding this comment.
Alright, so if we make sure to only save jobs with changes, then we should be alright?
I think so, but we should document the behavior clearly
There was a problem hiding this comment.
I'm not sure if its possible to know if a record has a different value or not when saving unless we reload the record or do two saves?
You can do a query on the entire record, but that's probably overkill. It'd be nice if Mongo had a feature where a field would only be updated if at least one of a set of provided fields changed
