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

Adds audit dates in the asset importer #10511

Merged
merged 1 commit into from
Jan 27, 2022

Conversation

inietov
Copy link
Collaborator

@inietov inietov commented Jan 13, 2022

Description

Adds the next_audit_date and last_audit_date to the importer. Not sure if the best approach since I skip the fillable columns... let me know if something is not ideal.

Feature request in internal freshdesk 24194

Type of change

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Test Configuration:

  • PHP version: 7.4.16
  • MySQL version: 8.0.23
  • Webserver version: nginx/1.19.8
  • OS version: Debian 10

Checklist:

@snipe
Copy link
Owner

snipe commented Jan 13, 2022

@uberbrady What are your thoughts here? I feel like maybe if there is a last_audit_date but NO next_audit_date, we should do the calculations that we normally do to determine the next audit date based on their audit settings. Where it could get weird is if the next_audit_date doesn't match the audit settings and users aren't sure why they're seeing the next_audit_date that they're seeing.

@adagioajanes
Copy link
Contributor

@uberbrady What are your thoughts here? I feel like maybe if there is a last_audit_date but NO next_audit_date, we should do the calculations that we normally do to determine the next audit date based on their audit settings. Where it could get weird is if the next_audit_date doesn't match the audit settings and users aren't sure why they're seeing the next_audit_date that they're seeing.

I know you didn't ask me, but I am commenting anyway. I tend to agree that it should just have last_audit_date. The next audit date should be calculated based on the Snipe IT configuration. If you change the audit policy, then all of the next_audit_date values would be invalid and need to be recalculated.

I also like the idea of last_audit_date as it will be much easier to query than trying to go through the audit logs.

@uberbrady
Copy link
Collaborator

The last time I checked on the API, we permit you to manually specify a next audit date, but if you don't, we calculate it for you based on your audit duration and last audit date. I figure the importer ought to be no different.

@snipe
Copy link
Owner

snipe commented Jan 14, 2022

@uberbrady this relates to the importer, not the API

@snipe
Copy link
Owner

snipe commented Jan 14, 2022

If you change the audit policy, then all of the next_audit_date values would be invalid and need to be recalculated.

That's not always going to be the expected behavior here though. If audit policy changed, but existing assets are exempt from those changes, it could get kind of confusing. We don't currently re-calculate ALL asset next_audit_date values when the audit policy is changed (I don't think), so we'd have to make it clear that that's the behavior moving forward if we were to add that.

@uberbrady
Copy link
Collaborator

I know this is about the importer, but the API is at least a standard we can follow. And this isn't any worse than that.

@snipe snipe merged commit a65fb63 into snipe:develop Jan 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants