Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stop updating OCDBase.updated_at on every save #146

Merged
merged 2 commits into from
Jan 10, 2023
Merged

Conversation

antidipyramid
Copy link
Collaborator

@antidipyramid antidipyramid commented Dec 16, 2022

Overview

This PR amends the OCDBase model to no longer update updated_at on save. This change is necessary to begin using the last_seen property properly.

Notes

Background

OCDBase.updated_at keeps track of when database items were last updated. Previously, we assumed that this timestamp should be updated automatically on every object save.

This PR

We will now be using a new OCDBase.last_seen to keep track of when an object was last seen in a scrape. Since we can no longer assume that new information was found in a scrape every time an object is saved, OCDBase.updated_at longer updates automatically on save.

Copy link
Contributor

@hancush hancush left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me, @antidipyramid. Would you mind adding a few things to the PR description to leave a clearer audit trail?

  • Describe the overall approach here: What is the distinction between last_seen and last_updated? How will they be kept current? How does this change fit into the approach?
  • Specify whether these changes rely on any other PR/s, and add a reference to it/them, if applicable.
  • Add references to any relevant issue/s.

If these changes rely on other PRs, please also wait to merge until everything is ready to come in at once. We install this package from master, so we don't want to deploy anything until it's ready for primetime.

@antidipyramid
Copy link
Collaborator Author

@hancush No problem.

Since this is just a change to OCDBase, I don't believe this PR depends on any other (but please correct me if I've got that wrong!).

There is, however, a pupa PR that depends on this one.

@hancush
Copy link
Contributor

hancush commented Jan 5, 2023

@antidipyramid I'm thinking if you merge this, then the updated at field will stop updating when objects are saved, since you've changed it to auto_add_now. Does that seem right to you?

@antidipyramid
Copy link
Collaborator Author

antidipyramid commented Jan 5, 2023

@hancush That does seem correct. The only thing is that I believe the new test for the last_seen field in the pupa PR will not pass until the changes in this PR are merged.

@hancush
Copy link
Contributor

hancush commented Jan 5, 2023

@antidipyramid Can you install from this branch, to incorporate the changes?

@antidipyramid
Copy link
Collaborator Author

@hancush As in locally? Yes, I can. When I do that, all tests pass.

@hancush
Copy link
Contributor

hancush commented Jan 5, 2023

@antidipyramid That's good news! Can you update the requirements in your pupa branch to install python-opencivic data from this branch, so the tests there pass?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants