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

Add creation modified times #100 #193

Merged
merged 20 commits into from
Feb 22, 2024
Merged

Conversation

joelvdavies
Copy link
Collaborator

Description

Adds created_time and updated_time fields to catalogue categories, catalogue items, items, manufacturers and systems.

Draft as tests need updating.

Testing instructions

Add a set up instructions describing how the reviewer should test the code

  • Review code
  • Check Actions build
  • Review changes to test coverage

Agile board tracking

Closes #100

@joelvdavies joelvdavies added the enhancement New feature or request label Feb 16, 2024
@joelvdavies joelvdavies self-assigned this Feb 16, 2024
@joelvdavies joelvdavies force-pushed the add-creation-modified-times-#100 branch from 3422aaf to 72fe4d2 Compare February 19, 2024 12:09
Copy link
Collaborator

@VKTB VKTB left a comment

Choose a reason for hiding this comment

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

I noticed the following before while fixing the service unit tests.

test/unit/services/test_catalogue_item.py Outdated Show resolved Hide resolved
test/unit/services/test_catalogue_item.py Show resolved Hide resolved
test/unit/services/test_item.py Outdated Show resolved Hide resolved
@VKTB VKTB force-pushed the add-creation-modified-times-#100 branch from b001b13 to 781a442 Compare February 20, 2024 16:17
Copy link
Collaborator Author

@joelvdavies joelvdavies left a comment

Choose a reason for hiding this comment

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

Looked through the updated service unit tests, just had a single thing that looked inconsistent, the rest LGTM.

test/unit/services/test_system.py Outdated Show resolved Hide resolved
@joelvdavies joelvdavies marked this pull request as ready for review February 22, 2024 10:04
VKTB
VKTB previously approved these changes Feb 22, 2024
Copy link
Collaborator

@VKTB VKTB left a comment

Choose a reason for hiding this comment

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

I reviewed your changes and they look good to me, thanks!

@joelvdavies
Copy link
Collaborator Author

I reviewed your changes and they look good to me, thanks!

I have also reviewed yours and they look good to me too (I can't approve as its my PR)

Copy link
Contributor

@MatteoGuarnaccia5 MatteoGuarnaccia5 left a comment

Choose a reason for hiding this comment

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

Works well for me and all the tests look fine to me as well

**FULL_CATALOGUE_ITEM_A_INFO,
**{
**FULL_CATALOGUE_ITEM_A_INFO,
"created_time": FULL_CATALOGUE_ITEM_A_INFO["created_time"] - timedelta(days=5),
Copy link
Contributor

Choose a reason for hiding this comment

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

why is there this - timedelta(days=5) in a lot of the tests? I don't understand what its purpose is

Copy link
Collaborator

@VKTB VKTB Feb 22, 2024

Choose a reason for hiding this comment

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

Both create and update times are generated when something is created and only the update time gets updated when something is updated. In the update tests, we return what the entity looks like before it is updated so we set the create and modified time 5 days earlier so that we can then ensure that the modified time gets updated to the mocked datetime. Let me know if this does not make sense and I will talk to you.

@joelvdavies joelvdavies merged commit d689864 into develop Feb 22, 2024
4 checks passed
@joelvdavies joelvdavies deleted the add-creation-modified-times-#100 branch February 22, 2024 17:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add creation and modified times
3 participants